Moved injection of AudioDecoderFactory into voe::Channel.

Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory to be sent along.

BUG=webrtc:5805
Committed: https://crrev.com/e352578bc8f98bf66523580c92f85fbfac6ee333
Cr-Commit-Position: refs/heads/master@{#12893}

[ossu, 2016-05-18 13:11:24]

Description was changed from

==========
Moved injection of AudioDecoderFactory into voe::Channel.

BUG=webrtc:5801
==========

to

==========
Moved injection of AudioDecoderFactory into voe::Channel.

BUG=webrtc:5801
==========

[ossu, 2016-05-18 13:11:24]

ossu@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[ossu, 2016-05-18 13:13:05]

Description was changed from

==========
Moved injection of AudioDecoderFactory into voe::Channel.

BUG=webrtc:5801
==========

to

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5801
==========

[ossu, 2016-05-18 13:13:24]

Next step!

[ossu, 2016-05-18 13:33:03]

https://codereview.webrtc.org/1992763002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/include/neteq.h (right):

https://codereview.webrtc.org/1992763002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/include/neteq.h:20: #incldue
"webrtc/base/scoped_ref_ptr.h"
Aw damnit! I can't spel. :(
Got carried away trying to clean up the change list and didn't double-check
before uploading.

[ossu, 2016-05-18 16:09:25]

https://codereview.webrtc.org/1992763002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1992763002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:25: #include
"webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h"
This one should go.

[ossu, 2016-05-19 13:03:41]

Description was changed from

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5801
==========

to

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5805
==========

[ossu, 2016-05-19 14:31:54]

Patchset #2 (id:20001) has been deleted

[ossu, 2016-05-19 15:46:48]

Added a couple of patchsets that clean up small mistakes that crept in while
pulling apart the bigger change list.

The last one (#3) removes a change in audio_coding_module_impl.h. It's not very
easy to see, since it just disappears from the file list of patch set 3.

[kwiberg-webrtc, 2016-05-24 12:35:35]

lgtm

This introduces a new required Config field to one of the ACM Create functions.
Will that require a deprecation dance?

[hlundin-webrtc, 2016-05-24 12:39:27]

On 2016/05/24 12:35:35, kwiberg-webrtc wrote:
> lgtm
> 
> This introduces a new required Config field to one of the ACM Create
functions.
> Will that require a deprecation dance?

Since the DecoderFactory is only used inside of NetEq (right?), why not add it
to the NetEq::Config and avoid the deprecation dance?

[kwiberg-webrtc, 2016-05-24 12:41:33]

On 2016/05/24 12:39:27, hlundin-webrtc wrote:
> On 2016/05/24 12:35:35, kwiberg-webrtc wrote:
> > lgtm
> > 
> > This introduces a new required Config field to one of the ACM Create
> functions.
> > Will that require a deprecation dance?
> 
> Since the DecoderFactory is only used inside of NetEq (right?), why not add it
> to the NetEq::Config and avoid the deprecation dance?

Because we want to inject it from the outside. That means it has to cross
difficult barriers such as the ACM interface somehow.

[ossu, 2016-05-24 12:48:02]

On 2016/05/24 12:39:27, hlundin-webrtc wrote:
> On 2016/05/24 12:35:35, kwiberg-webrtc wrote:
> > lgtm
> > 
> > This introduces a new required Config field to one of the ACM Create
> functions.
> > Will that require a deprecation dance?
> 
> Since the DecoderFactory is only used inside of NetEq (right?), why not add it
> to the NetEq::Config and avoid the deprecation dance?

I initially tried putting the factory into NetEq::Config, but there were some
problems.
Since NetEqImpl takes a NetEq::Config and a DecoderDatabase - which itself is
created with an AudioDecoderFactory -
we would end up with a possible ambiguity. This was apparent in the tests. With
the factory as a parameter to NetEq::Create, it's possible to put it where it
needs to go, and only there.

[ossu, 2016-05-24 12:51:59]

On 2016/05/24 12:35:35, kwiberg-webrtc wrote:
> lgtm
> 
> This introduces a new required Config field to one of the ACM Create
functions.
> Will that require a deprecation dance?

Hmm. Maybe all versions of Create should default to creating a
BuiltinAudioDecoderFactory() for now?
Then, we could deprecate any and all fallbacks to the builtin factory in one
fell swoop, once we've put everything else in place.

[kwiberg-webrtc, 2016-05-24 12:55:18]

On 2016/05/24 12:51:59, ossu wrote:
> On 2016/05/24 12:35:35, kwiberg-webrtc wrote:
> > lgtm
> > 
> > This introduces a new required Config field to one of the ACM Create
> functions.
> > Will that require a deprecation dance?
> 
> Hmm. Maybe all versions of Create should default to creating a
> BuiltinAudioDecoderFactory() for now?
> Then, we could deprecate any and all fallbacks to the builtin factory in one
> fell swoop, once we've put everything else in place.

Yes---if they get null, create it. Then PSA that you'll remove that two weeks
from now.

[ossu, 2016-05-24 13:11:33]

Patchset #4 (id:80001) has been deleted

[ossu, 2016-05-24 13:13:51]

On 2016/05/24 12:55:18, kwiberg-webrtc wrote:
> On 2016/05/24 12:51:59, ossu wrote:
> > On 2016/05/24 12:35:35, kwiberg-webrtc wrote:
> > > lgtm
> > > 
> > > This introduces a new required Config field to one of the ACM Create
> > functions.
> > > Will that require a deprecation dance?
> > 
> > Hmm. Maybe all versions of Create should default to creating a
> > BuiltinAudioDecoderFactory() for now?
> > Then, we could deprecate any and all fallbacks to the builtin factory in one
> > fell swoop, once we've put everything else in place.
> 
> Yes---if they get null, create it. Then PSA that you'll remove that two weeks
> from now.

Added fallback to builtin factory. Please take a look before I try to get this
to apply to master (possibly rebase?).

[kwiberg-webrtc, 2016-05-24 13:17:11]

lgtm

[ossu, 2016-05-24 13:28:58]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-05-24 13:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1992763002/#ps120001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-05-24 13:29:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992763002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992763002/120001

[commit-bot: I haz the power, 2016-05-24 13:30:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 13:30:18]

Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7742)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7725)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10063)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/9986)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15276)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13926)
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/14888)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3423)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9641)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/15390)

[ossu, 2016-05-24 14:38:52]

Patchset #6 (id:140001) has been deleted

[ossu, 2016-05-24 14:39:04]

Patchset #5 (id:120001) has been deleted

[hlundin-webrtc, 2016-05-25 06:58:17]

henrik.lundin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-05-25 06:58:17]

lgtm with one nit.

Aslo, I see why you couldn't add the factory to NetEq::Config – I yield.

https://codereview.webrtc.org/1992763002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc (right):

https://codereview.webrtc.org/1992763002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/tools/neteq_rtpplay.cc:479:
NetEq::Create(config, webrtc::CreateBuiltinAudioDecoderFactory());
You should be able to drop webrtc:: here, since I moved all of the code inside
that namespace.

[ossu, 2016-05-25 09:14:52]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-05-25 09:14:52]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org,
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1992763002/#ps180001 (title:
"Adressed nit.")

[commit-bot: I haz the power, 2016-05-25 09:14:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992763002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992763002/180001

[commit-bot: I haz the power, 2016-05-25 09:26:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 09:26:49]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5833)

[ossu, 2016-05-25 09:33:52]

ossu@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org

[ossu, 2016-05-25 09:35:08]

Added Tina as reviewer. I need owner signoff specifically on these two files:
    webrtc/modules/utility/source/coder.cc
    webrtc/voice_engine/channel.cc

Thanks!

[tlegrand-webrtc, 2016-05-25 14:20:09]

On 2016/05/25 09:35:08, ossu wrote:
> Added Tina as reviewer. I need owner signoff specifically on these two files:
>     webrtc/modules/utility/source/coder.cc
>     webrtc/voice_engine/channel.cc
> 
> Thanks!

Reviewed the two files: LGTM

[ossu, 2016-05-25 14:25:59]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-05-25 14:26:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1992763002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1992763002/180001

[commit-bot: I haz the power, 2016-05-25 14:37:47]

Description was changed from

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5805
==========

to

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5805
==========

[commit-bot: I haz the power, 2016-05-25 14:37:49]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-05-25 14:37:57]

Description was changed from

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5805
==========

to

==========
Moved injection of AudioDecoderFactory into voe::Channel.

Channel's API remains unchanged, but the creation of a
BuiltinAudioDecoderFactory is now in Channel. The next step would be to amend
Channel's API (through CreateChannel, I believe) to allow an AudioDecoderFactory
to be sent along.

BUG=webrtc:5805

Committed: https://crrev.com/e352578bc8f98bf66523580c92f85fbfac6ee333
Cr-Commit-Position: refs/heads/master@{#12893}
==========

[commit-bot: I haz the power, 2016-05-25 14:37:58]

Patchset 6 (id:??) landed as
https://crrev.com/e352578bc8f98bf66523580c92f85fbfac6ee333
Cr-Commit-Position: refs/heads/master@{#12893}