Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

Added notry due to android_dbg being broken.

NOTRY=True
BUG=webrtc:5805
Committed: https://crrev.com/29b1a8d7ec15f0f3fa6cf5f814f6c7a6bb3dc972
Cr-Commit-Position: refs/heads/master@{#13112}

[ossu, 2016-05-19 16:07:37]

Description was changed from

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

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

to

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

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

[ossu, 2016-05-19 16:07:37]

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

[ossu, 2016-05-19 16:08:48]

This is the last in the set of CLs splitting up
https://codereview.webrtc.org/1949533002/.

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/nullwebrt...
File webrtc/media/engine/nullwebrtcvideoengine_unittest.cc (right):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/nullwebrt...
webrtc/media/engine/nullwebrtcvideoengine_unittest.cc:37:
WebRtcMediaEngineNullVideo engine(nullptr, nullptr, nullptr, nullptr);
This is just lovely! :)

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (left):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:100: :
WebRtcVideoEngine2Test(nullptr, field_trials) {}
While updating the tests to my changes, I found that this test doesn't use the
voice engine, so I removed it.

https://codereview.webrtc.org/1991233004/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (left):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:278:
rtc::scoped_refptr<AudioDecoderFactory> decoder_factory_;
This is already removed by patch set 3 of
https://codereview.webrtc.org/1992763002/.

[kwiberg-webrtc, 2016-05-30 10:04:27]

lgtm, with comments

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2_unittest.cc (left):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2_unittest.cc:100: :
WebRtcVideoEngine2Test(nullptr, field_trials) {}
On 2016/05/19 16:08:48, ossu wrote:
> While updating the tests to my changes, I found that this test doesn't use the
> voice engine, so I removed it.

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.h:49: explicit WebRtcVoiceEngine(
I don't think the constructor needs to be explicit now that it takes a non-1
number of arguments.

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:3547: // we don't want it to
every create a decoder at this stage.
grammar?

https://codereview.webrtc.org/1991233004/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (left):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:278:
rtc::scoped_refptr<AudioDecoderFactory> decoder_factory_;
On 2016/05/19 16:08:48, ossu wrote:
> This is already removed by patch set 3 of
> https://codereview.webrtc.org/1992763002/.

Acknowledged.

[ossu, 2016-05-30 12:49:24]

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

[ossu, 2016-05-30 12:49:25]

Addressed comments. Added Tina for owner approval.

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.h:49: explicit WebRtcVoiceEngine(
On 2016/05/30 10:04:27, kwiberg-webrtc wrote:
> I don't think the constructor needs to be explicit now that it takes a non-1
> number of arguments.

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1991233004/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:3547: // we don't want it to
every create a decoder at this stage.
On 2016/05/30 10:04:27, kwiberg-webrtc wrote:
> grammar?

Acknowledged.

[the sun, 2016-05-30 19:09:52]

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

[the sun, 2016-05-30 19:09:53]

lgtm % nits

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:97: // This is where we'd like to set the
decoder factory to use. However, since
A TODO(ossu) seems appropriate here I think.

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:103: RTC_CHECK(config.decoder_factory ==
channel_proxy_->GetAudioDecoderFactory());
RTC_CHECK_EQ

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2179: engine_->decoder_factory_)));
engine(), or change all the other places using "engine()"

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1077: }
nit: blank line

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:83: const
rtc::scoped_refptr<AudioDecoderFactory>& GetAudioDecoderFactory() const;
virtual - this class should be fully mockable.

[ossu, 2016-06-02 15:47:47]

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:97: // This is where we'd like to set the
decoder factory to use. However, since
On 2016/05/30 19:09:53, the sun wrote:
> A TODO(ossu) seems appropriate here I think.

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:103: RTC_CHECK(config.decoder_factory ==
channel_proxy_->GetAudioDecoderFactory());
On 2016/05/30 19:09:53, the sun wrote:
> RTC_CHECK_EQ

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2179: engine_->decoder_factory_)));
On 2016/05/30 19:09:53, the sun wrote:
> engine(), or change all the other places using "engine()"

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1077: }
On 2016/05/30 19:09:53, the sun wrote:
> nit: blank line

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel_proxy.h:83: const
rtc::scoped_refptr<AudioDecoderFactory>& GetAudioDecoderFactory() const;
On 2016/05/30 19:09:53, the sun wrote:
> virtual - this class should be fully mockable.

Oh, right!

[tlegrand-webrtc, 2016-06-03 06:03:23]

On 2016/06/02 15:47:47, ossu wrote:
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
> File webrtc/audio/audio_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
> webrtc/audio/audio_receive_stream.cc:97: // This is where we'd like to set the
> decoder factory to use. However, since
> On 2016/05/30 19:09:53, the sun wrote:
> > A TODO(ossu) seems appropriate here I think.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
> webrtc/audio/audio_receive_stream.cc:103: RTC_CHECK(config.decoder_factory ==
> channel_proxy_->GetAudioDecoderFactory());
> On 2016/05/30 19:09:53, the sun wrote:
> > RTC_CHECK_EQ
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
> File webrtc/media/engine/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
> webrtc/media/engine/webrtcvoiceengine.cc:2179: engine_->decoder_factory_)));
> On 2016/05/30 19:09:53, the sun wrote:
> > engine(), or change all the other places using "engine()"
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> File webrtc/voice_engine/channel.cc (right):
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> webrtc/voice_engine/channel.cc:1077: }
> On 2016/05/30 19:09:53, the sun wrote:
> > nit: blank line
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> File webrtc/voice_engine/channel_proxy.h (right):
> 
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> webrtc/voice_engine/channel_proxy.h:83: const
> rtc::scoped_refptr<AudioDecoderFactory>& GetAudioDecoderFactory() const;
> On 2016/05/30 19:09:53, the sun wrote:
> > virtual - this class should be fully mockable.
> 
> Oh, right!

LG, but the trybots doesn't seem to like this CL. Can you take a look at that
first?

[ossu, 2016-06-03 08:36:20]

On 2016/06/03 06:03:23, tlegrand-webrtc wrote:
> On 2016/06/02 15:47:47, ossu wrote:
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
> > File webrtc/audio/audio_receive_stream.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
> > webrtc/audio/audio_receive_stream.cc:97: // This is where we'd like to set
the
> > decoder factory to use. However, since
> > On 2016/05/30 19:09:53, the sun wrote:
> > > A TODO(ossu) seems appropriate here I think.
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/audio/audio_receiv...
> > webrtc/audio/audio_receive_stream.cc:103: RTC_CHECK(config.decoder_factory
==
> > channel_proxy_->GetAudioDecoderFactory());
> > On 2016/05/30 19:09:53, the sun wrote:
> > > RTC_CHECK_EQ
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
> > File webrtc/media/engine/webrtcvoiceengine.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/media/engine/webrt...
> > webrtc/media/engine/webrtcvoiceengine.cc:2179: engine_->decoder_factory_)));
> > On 2016/05/30 19:09:53, the sun wrote:
> > > engine(), or change all the other places using "engine()"
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> > File webrtc/voice_engine/channel.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> > webrtc/voice_engine/channel.cc:1077: }
> > On 2016/05/30 19:09:53, the sun wrote:
> > > nit: blank line
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> > File webrtc/voice_engine/channel_proxy.h (right):
> > 
> >
>
https://codereview.webrtc.org/1991233004/diff/20001/webrtc/voice_engine/chann...
> > webrtc/voice_engine/channel_proxy.h:83: const
> > rtc::scoped_refptr<AudioDecoderFactory>& GetAudioDecoderFactory() const;
> > On 2016/05/30 19:09:53, the sun wrote:
> > > virtual - this class should be fully mockable.
> > 
> > Oh, right!
> 
> LG, but the trybots doesn't seem to like this CL. Can you take a look at that
> first?

On closer inspection, there seems to be local tests that I've failed to run and
thus to update. I'm on it!

[ossu, 2016-06-09 15:36:39]

Alright, I've updated the failing tests with injection of audio decoder
factories. PTAL, specifically on the 4 -> 5 patch set delta.

[tlegrand-webrtc, 2016-06-13 06:55:41]

On 2016/06/09 15:36:39, ossu wrote:
> Alright, I've updated the failing tests with injection of audio decoder
> factories. PTAL, specifically on the 4 -> 5 patch set delta.

LGTM, but please have Karl take another look too.

[kwiberg-webrtc, 2016-06-13 13:06:01]

lgtm, but see comments

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/call/call_unittes...
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/call/call_unittes...
webrtc/call/call_unittest.cc:26: : voice_engine_(decoder_factory) {
Since this constructor accepts one argument, it should be explicit.

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/fakeme...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/fakeme...
webrtc/media/base/fakemediaengine.h:742: audio_decoder_factory)
This now takes two arguments, so it doesn't need to be explicit.

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/mediae...
File webrtc/media/base/mediaengine.h (right):

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/mediae...
webrtc/media/base/mediaengine.h:135: : voice_(adm, audio_decoder_factory) {}
Doesn't need to be explicit.

[ossu, 2016-06-13 13:28:39]

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/call/call_unittes...
File webrtc/call/call_unittest.cc (right):

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/call/call_unittes...
webrtc/call/call_unittest.cc:26: : voice_engine_(decoder_factory) {
On 2016/06/13 13:06:00, kwiberg-webrtc wrote:
> Since this constructor accepts one argument, it should be explicit.

It's only used in this test, but you're technically correct - the best kind of
correct! :)

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/fakeme...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/fakeme...
webrtc/media/base/fakemediaengine.h:742: audio_decoder_factory)
On 2016/06/13 13:06:00, kwiberg-webrtc wrote:
> This now takes two arguments, so it doesn't need to be explicit.

Acknowledged.

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/mediae...
File webrtc/media/base/mediaengine.h (right):

https://codereview.webrtc.org/1991233004/diff/100001/webrtc/media/base/mediae...
webrtc/media/base/mediaengine.h:135: : voice_(adm, audio_decoder_factory) {}
On 2016/06/13 13:06:00, kwiberg-webrtc wrote:
> Doesn't need to be explicit.

Acknowledged.

[ossu, 2016-06-13 13:28:55]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-06-13 13:28:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kwiberg@webrtc.org, tina.legrand@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1991233004/#ps120001 (title:
"Parental Advisory: Explicit Content")

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

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

[commit-bot: I haz the power, 2016-06-13 13:36:47]

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

[commit-bot: I haz the power, 2016-06-13 13:36:49]

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

[ossu, 2016-06-13 14:32:41]

Description was changed from

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

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

to

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

Added notry due to android_dbg being broken.

NOTRY=True 
BUG=webrtc:5805
==========

[ossu, 2016-06-13 14:32:50]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-06-13 14:32:53]

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

[commit-bot: I haz the power, 2016-06-13 14:34:55]

Description was changed from

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

Added notry due to android_dbg being broken.

NOTRY=True 
BUG=webrtc:5805
==========

to

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

Added notry due to android_dbg being broken.

NOTRY=True 
BUG=webrtc:5805
==========

[commit-bot: I haz the power, 2016-06-13 14:34:57]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-13 14:35:11]

Description was changed from

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

Added notry due to android_dbg being broken.

NOTRY=True 
BUG=webrtc:5805
==========

to

==========
Moved creation of AudioDecoderFactory to inside PeerConnectionFactory.

CreatePeerConnectionFactory does not yet expose the ability to set the
factory from the outside.

Added notry due to android_dbg being broken.

NOTRY=True
BUG=webrtc:5805

Committed: https://crrev.com/29b1a8d7ec15f0f3fa6cf5f814f6c7a6bb3dc972
Cr-Commit-Position: refs/heads/master@{#13112}
==========

[commit-bot: I haz the power, 2016-06-13 14:35:12]

Patchset 7 (id:??) landed as
https://crrev.com/29b1a8d7ec15f0f3fa6cf5f814f6c7a6bb3dc972
Cr-Commit-Position: refs/heads/master@{#13112}