Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

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

Description was changed from

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

to

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

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

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

[ossu, 2016-05-18 16:12:27]

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

https://codereview.webrtc.org/1993783002/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 already have been removed in previous CL.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_base.h:37: #include <memory>
This one isn't necessary.

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

Description was changed from

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

to

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

[kwiberg-webrtc, 2016-05-25 09:19:17]

lgtm

https://codereview.webrtc.org/1993783002/diff/1/webrtc/media/engine/fakewebrt...
File webrtc/media/engine/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/media/engine/fakewebrt...
webrtc/media/engine/fakewebrtcvoiceengine.h:233: (webrtc::AudioDeviceModule *
adm,
Spurios space.

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

https://codereview.webrtc.org/1993783002/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"
On 2016/05/18 16:12:27, ossu wrote:
> This one should already have been removed in previous CL.

Acknowledged.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:184: const
rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory);
Do you plan to remove the original CreateChannel function? A comment to that
effect would be useful, in that case.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
File webrtc/voice_engine/channel_manager.cc (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
webrtc/voice_engine/channel_manager.cc:56: return
CreateChannel(CreateBuiltinAudioDecoderFactory());
Forward straight to the two-argument CreateChannel?

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
File webrtc/voice_engine/channel_manager.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
webrtc/voice_engine/channel_manager.h:110: const
rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory);
Again, a comment about the impending removal of the original two functions?

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_base.h:37: #include <memory>
On 2016/05/18 16:12:27, ossu wrote:
> This one isn't necessary.

Acknowledged.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_base.h:137: decoder_factory = nullptr) = 0;
Not your fault, but ew! Default values for arguments in a virtual function!

[ossu, 2016-05-26 12:09:53]

https://codereview.webrtc.org/1993783002/diff/1/webrtc/media/engine/fakewebrt...
File webrtc/media/engine/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/media/engine/fakewebrt...
webrtc/media/engine/fakewebrtcvoiceengine.h:233: (webrtc::AudioDeviceModule *
adm,
On 2016/05/25 09:19:16, kwiberg-webrtc wrote:
> Spurios space.

Acknowledged.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:184: const
rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory);
On 2016/05/25 09:19:16, kwiberg-webrtc wrote:
> Do you plan to remove the original CreateChannel function? A comment to that
> effect would be useful, in that case.

I'm not sure, really. Since a long-term goal is to get rid of Channel
completely, I think it will retain its API until we've achieved feature-parity
in Audio{Send,Receive}Stream.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
File webrtc/voice_engine/channel_manager.cc (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
webrtc/voice_engine/channel_manager.cc:56: return
CreateChannel(CreateBuiltinAudioDecoderFactory());
On 2016/05/25 09:19:16, kwiberg-webrtc wrote:
> Forward straight to the two-argument CreateChannel?

I think this is fine. Means we only need to change in one place if, for example,
the way we handle the internal config_ changes in the other version.

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
File webrtc/voice_engine/channel_manager.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/channel_m...
webrtc/voice_engine/channel_manager.h:110: const
rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory);
On 2016/05/25 09:19:16, kwiberg-webrtc wrote:
> Again, a comment about the impending removal of the original two functions?

Yeah, as with the other ones, I think we'll keep it like this. Or do you think
moving away from Channel is so far away that deprecating the old interfaces
necessary?

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
File webrtc/voice_engine/include/voe_base.h (right):

https://codereview.webrtc.org/1993783002/diff/1/webrtc/voice_engine/include/v...
webrtc/voice_engine/include/voe_base.h:137: decoder_factory = nullptr) = 0;
On 2016/05/25 09:19:16, kwiberg-webrtc wrote:
> Not your fault, but ew! Default values for arguments in a virtual function!

Yeah. :/
Fortunately, they're all "no-value" defaults (i.e. nullptr), rather than some
cleverly thought-out value that some subclass might override or need to copy
exactly.

[ossu, 2016-05-26 15:01:52]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-05-26 15:01:52]

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/1993783002/#ps40001 (title:
"Rebass! How low can you go?")

[commit-bot: I haz the power, 2016-05-26 15:01:58]

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

[commit-bot: I haz the power, 2016-05-26 15:06:09]

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

[commit-bot: I haz the power, 2016-05-26 15:06:11]

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

[ossu, 2016-05-26 15:17:19]

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

[ossu, 2016-05-26 15:17:20]

Adding Tina for owner approval.

[tlegrand, 2016-05-27 14:01:11]

On 2016/05/26 15:17:20, ossu wrote:
> Adding Tina for owner approval.

lgtm

[ossu, 2016-05-27 14:11:30]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-05-27 14:11:33]

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

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

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

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

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

[ossu, 2016-05-30 12:40:30]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-05-30 12:40:34]

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

[commit-bot: I haz the power, 2016-05-30 12:47:04]

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

[commit-bot: I haz the power, 2016-05-30 12:47:05]

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

[ossu, 2016-05-30 13:06:15]

On 2016/05/27 14:01:11, tlegrand wrote:
> On 2016/05/26 15:17:20, ossu wrote:
> > Adding Tina for owner approval.
> 
> lgtm

I'm sorry, but it seems your LGTM might not've registered, since it was not
using the @webrtc.org account (i.e. not the account that matches an OWNERS
entry). Could you please LGTM from that one, or should I just land it manually?

[tlegrand-webrtc, 2016-05-30 13:08:20]

On 2016/05/30 13:06:15, ossu wrote:
> On 2016/05/27 14:01:11, tlegrand wrote:
> > On 2016/05/26 15:17:20, ossu wrote:
> > > Adding Tina for owner approval.
> > 
> > lgtm
> 
> I'm sorry, but it seems your LGTM might not've registered, since it was not
> using the @webrtc.org account (i.e. not the account that matches an OWNERS
> entry). Could you please LGTM from that one, or should I just land it
manually?

Sorry, my bad!
LGTM

[ossu, 2016-05-30 13:08:47]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-05-30 13:08:57]

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

[commit-bot: I haz the power, 2016-05-30 15:11:32]

Description was changed from

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

to

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

[commit-bot: I haz the power, 2016-05-30 15:11:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-30 15:11:40]

Description was changed from

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

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

to

==========
Moved CreateBuiltinDecoderFactory out to VoEBaseImpl.

VoEBase is plumbed to optionally take an AudioDecoderFactory, or create
a builtin factory if none is provided.

Retained the CreateChannel interfaces in Channel and ChannelManager
and added variants for injecting an AudioDecoderFactory. The
"old-style" variants call CreateBuiltinAudioDecoderFactory to get a
factory to use.

(Just realized this means each channel uses a separate factory with the
old-style calls. Probably ok.)

BUG=webrtc:5805

Committed: https://crrev.com/5f7cfa50e515634f84198f5f4a713f78e8e2019b
Cr-Commit-Position: refs/heads/master@{#12961}
==========

[commit-bot: I haz the power, 2016-05-30 15:11:41]

Patchset 3 (id:??) landed as
https://crrev.com/5f7cfa50e515634f84198f5f4a713f78e8e2019b
Cr-Commit-Position: refs/heads/master@{#12961}

[the sun, 2016-05-30 18:46:47]

solenberg@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[the sun, 2016-05-30 18:46:48]

https://codereview.webrtc.org/1993783002/diff/40001/webrtc/voice_engine/voe_b...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/1993783002/diff/40001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:384: decoder_factory_ =
CreateBuiltinAudioDecoderFactory();
I think since the VoEBase::Init() interface has been changed, this is the *only*
place we should have a dependency on CreateBuiltinAudioDecoderFactory(). It can
be propagated down from here and we shouldn't need it in Channel or
ChannelManager. Right?

[ossu, 2016-05-31 08:54:18]

https://codereview.webrtc.org/1993783002/diff/40001/webrtc/voice_engine/voe_b...
File webrtc/voice_engine/voe_base_impl.cc (right):

https://codereview.webrtc.org/1993783002/diff/40001/webrtc/voice_engine/voe_b...
webrtc/voice_engine/voe_base_impl.cc:384: decoder_factory_ =
CreateBuiltinAudioDecoderFactory();
On 2016/05/30 18:46:48, the sun wrote:
> I think since the VoEBase::Init() interface has been changed, this is the
*only*
> place we should have a dependency on CreateBuiltinAudioDecoderFactory(). It
can
> be propagated down from here and we shouldn't need it in Channel or
> ChannelManager. Right?

Now, the last time we spoke about this you wanted CreatChannel to retain its old
interface as well. The only way to do that is to create a builtin factory there,
if the caller hasn't supplied one. Or am I misunderstanding your question?

[the sun, 2016-05-31 09:32:29]

On 2016/05/31 08:54:18, ossu wrote:
>
https://codereview.webrtc.org/1993783002/diff/40001/webrtc/voice_engine/voe_b...
> File webrtc/voice_engine/voe_base_impl.cc (right):
> 
>
https://codereview.webrtc.org/1993783002/diff/40001/webrtc/voice_engine/voe_b...
> webrtc/voice_engine/voe_base_impl.cc:384: decoder_factory_ =
> CreateBuiltinAudioDecoderFactory();
> On 2016/05/30 18:46:48, the sun wrote:
> > I think since the VoEBase::Init() interface has been changed, this is the
> *only*
> > place we should have a dependency on CreateBuiltinAudioDecoderFactory(). It
> can
> > be propagated down from here and we shouldn't need it in Channel or
> > ChannelManager. Right?
> 
> Now, the last time we spoke about this you wanted CreatChannel to retain its
old
> interface as well. The only way to do that is to create a builtin factory
there,
> if the caller hasn't supplied one. Or am I misunderstanding your question?

I was talking about VoEBase::CreateChannel(), sorry about the confusion. It
should be possible to leave that as is now that we take the decoder factory in
VoEBase::Init(), or am I wrong?