Add CreatePeerConnectionFactory overloads that take audio codec factory args

Add CreatePeerConnectionFactory overloads that take audio codec factory args

BUG=5805
Review-Url: https://codereview.webrtc.org/2653343003
Cr-Commit-Position: refs/heads/master@{#16371}
Committed: https://chromium.googlesource.com/external/webrtc/+/1e4e8cb43d85a32ac7d0b33392acf7116714e53a

[kwiberg-webrtc, 2017-01-26 01:51:25]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 01:51:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-26 01:59:42]

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

[commit-bot: I haz the power, 2017-01-26 01:59:42]

Dry run: Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[kwiberg-webrtc, 2017-01-26 12:35:46]

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

[kwiberg-webrtc, 2017-01-26 12:35:48]

This isn't done yet (see the red bots), but you're welcome to try to land this
in case you start to block on it.

[ossu, 2017-01-26 12:53:53]

Cool, thanks!

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:834: // TODO(ossu): Remove these and define
a real builtin audio encoder factory
I'd prefer if you put this in audio_encoder_factory.h. It's slightly less
confusing than having it declared here.

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/pc/peerconnectionf...
File webrtc/pc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/pc/peerconnectionf...
webrtc/pc/peerconnectionfactory.h:131: rtc::scoped_refptr<AudioEncoderFactory>
audio_encoder_factory_;
I expected with the API change we'd just drop the AudioEncoderFactory in the
PeerConnectionFactory constructor, for now, since it has no implementation. Did
I misunderstand?

[kwiberg-webrtc, 2017-01-30 13:11:08]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 13:11:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[kwiberg-webrtc, 2017-01-30 13:15:21]

New patch set posted.

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:834: // TODO(ossu): Remove these and define
a real builtin audio encoder factory
On 2017/01/26 12:53:53, ossu wrote:
> I'd prefer if you put this in audio_encoder_factory.h. It's slightly less
> confusing than having it declared here.

(We discussed this off-line, and agreed that I would leave it where it is, since
CreatePeerConnectionFactory is the only user. Soon enough, this dummy will be
replaced with a real implementation in the proper file.)

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/pc/peerconnectionf...
File webrtc/pc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/pc/peerconnectionf...
webrtc/pc/peerconnectionfactory.h:131: rtc::scoped_refptr<AudioEncoderFactory>
audio_encoder_factory_;
On 2017/01/26 12:53:53, ossu wrote:
> I expected with the API change we'd just drop the AudioEncoderFactory in the
> PeerConnectionFactory constructor, for now, since it has no implementation.
Did
> I misunderstand?

No, I just chose a slightly different place to drop it than you expected.

I made a new patch set that removes this member variable, but still takes a
webrtc::AudioEncoderFactory argument in the PeerConnectionFactory constructors.

[commit-bot: I haz the power, 2017-01-30 13:19:20]

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

[commit-bot: I haz the power, 2017-01-30 13:19:21]

Dry run: Try jobs failed on following builders:
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/9079)

[ossu, 2017-01-30 13:58:02]

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:834: // TODO(ossu): Remove these and define
a real builtin audio encoder factory
On 2017/01/30 13:15:21, kwiberg-webrtc wrote:
> On 2017/01/26 12:53:53, ossu wrote:
> > I'd prefer if you put this in audio_encoder_factory.h. It's slightly less
> > confusing than having it declared here.
> 
> (We discussed this off-line, and agreed that I would leave it where it is,
since
> CreatePeerConnectionFactory is the only user. Soon enough, this dummy will be
> replaced with a real implementation in the proper file.)

I'm not sure that's what I meant but it's not super important, since it's only
temporary.

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:838: CreateBuiltinAudioEncoderFactory() {
I do find this particularly weird, though. I'd prefer if you just put a nullptr
there directly, but maybe this makes it easier to extend? That is, if every
call-site uses this call it should "just work" once
CreateBuiltinAudioEncoderFactory() is actually implemented.

https://codereview.webrtc.org/2653343003/diff/40001/webrtc/pc/peerconnectionf...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2653343003/diff/40001/webrtc/pc/peerconnectionf...
webrtc/pc/peerconnectionfactory.cc:142: if (!signaling_thread_) {
Perhaps add a TODO or similar comment here, indicating that we're intentionally
dropping audio_encoder_factory and point to bug webrtc:5806 (the injectable
audio encoder one). Assign the TODO to me, if you want. :)

[kwiberg-webrtc, 2017-01-30 20:55:26]

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:834: // TODO(ossu): Remove these and define
a real builtin audio encoder factory
On 2017/01/30 13:58:02, ossu wrote:
> On 2017/01/30 13:15:21, kwiberg-webrtc wrote:
> > On 2017/01/26 12:53:53, ossu wrote:
> > > I'd prefer if you put this in audio_encoder_factory.h. It's slightly less
> > > confusing than having it declared here.
> > 
> > (We discussed this off-line, and agreed that I would leave it where it is,
> since
> > CreatePeerConnectionFactory is the only user. Soon enough, this dummy will
be
> > replaced with a real implementation in the proper file.)
> 
> I'm not sure that's what I meant but it's not super important, since it's only
> temporary.

Oh, sorry. So we agreed, but not on the same thing? :-/

Top Google suggestions for "blame it on" are "the boogie", "the rain", "the
night", and "me". I'm betting it's one of the first three in this case.

https://codereview.webrtc.org/2653343003/diff/20001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:838: CreateBuiltinAudioEncoderFactory() {
On 2017/01/30 13:58:02, ossu wrote:
> I do find this particularly weird, though. I'd prefer if you just put a
nullptr
> there directly, but maybe this makes it easier to extend? That is, if every
> call-site uses this call it should "just work" once
> CreateBuiltinAudioEncoderFactory() is actually implemented.

Yes, exactly. It's just as simple for me to write
"CreateBuiltinAudioEncoderFactory()" as "nullptr", and if I do the former I
don't leave you with the unenviable job of figuring out exactly which 10 of the
10000 occurrences of "nullptr" need to be replaced by
"CreateBuiltinAudioEncoderFactory()".

In other words, you may not like it, but it's for your own good!

https://codereview.webrtc.org/2653343003/diff/40001/webrtc/pc/peerconnectionf...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2653343003/diff/40001/webrtc/pc/peerconnectionf...
webrtc/pc/peerconnectionfactory.cc:142: if (!signaling_thread_) {
On 2017/01/30 13:58:02, ossu wrote:
> Perhaps add a TODO or similar comment here, indicating that we're
intentionally
> dropping audio_encoder_factory and point to bug webrtc:5806 (the injectable
> audio encoder one). Assign the TODO to me, if you want. :)

Done.

[kwiberg-webrtc, 2017-01-30 21:26:46]

The CQ bit was checked by kwiberg@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 21:26:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-30 22:39:53]

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

[commit-bot: I haz the power, 2017-01-30 22:39:55]

Dry run: This issue passed the CQ dry run.

[kwiberg-webrtc, 2017-01-30 22:49:30]

Two new patch sets (plus a rebase) uploaded—this is ready for a final review
now, provided you still don't feel like putting your foot down regarding the
location of AudioEncoderFactory.

[ossu, 2017-01-31 09:37:29]

lgtm

No, not going to put my foot down about anything. :)

I was sure you had changed some overloads that were used internally, but the bot
seems happy with the changes and so am I.

[kwiberg-webrtc, 2017-01-31 09:44:47]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2017-01-31 09:44:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-31 09:48:09]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485855887213060,
"parent_rev": "7ce109acd37ff1981e6b65eb96500700513743a5", "commit_rev":
"1e4e8cb43d85a32ac7d0b33392acf7116714e53a"}

[commit-bot: I haz the power, 2017-01-31 09:48:13]

Description was changed from

==========
Add CreatePeerConnectionFactory overloads that take audio codec factory args

BUG=5805
==========

to

==========
Add CreatePeerConnectionFactory overloads that take audio codec factory args

BUG=5805

Review-Url: https://codereview.webrtc.org/2653343003
Cr-Commit-Position: refs/heads/master@{#16371}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1e4e8cb43d85a32ac7d0b3339...
==========

[commit-bot: I haz the power, 2017-01-31 09:48:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/1e4e8cb43d85a32ac7d0b3339...