Issue 2854123003: Build WebRTC with data channel only.

Zhi Huang

Patchset #3 (id:40001) has been deleted

3 years, 7 months ago (2017-05-03 03:02:16 UTC) #1

Zhi Huang

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

3 years, 7 months ago (2017-05-03 03:40:26 UTC) #4

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-03 03:40:34 UTC) #5

Zhi Huang

zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org

3 years, 7 months ago (2017-05-03 03:41:59 UTC) #6

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-03 03:42:04 UTC) #8

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-03 03:42:04 UTC) #9

pthatcher1

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

3 years, 7 months ago (2017-05-03 18:05:52 UTC) #10

pthatcher1

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

3 years, 7 months ago (2017-05-03 18:05:52 UTC) #11

pthatcher1

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h#newcode65 webrtc/call/call.h:65: #endif Would it be easier to just not include ...

3 years, 7 months ago (2017-05-03 18:05:54 UTC) #12

pthatcher1

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h#newcode65 webrtc/call/call.h:65: #endif Would it be easier to just not include ...

3 years, 7 months ago (2017-05-03 18:05:54 UTC) #13

Taylor Brandstetter

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn#newcode137 webrtc/media/BUILD.gn:137: "engine/webrtcvideoencoderfactory.h", What would it take to move these files ...

3 years, 7 months ago (2017-05-03 22:50:57 UTC) #14

Zhi Huang

Description was changed from ========== Reduce the binary size by removing the video/audio stack. BUG=None ...

3 years, 7 months ago (2017-05-04 01:03:30 UTC) #16

Zhi Huang

Description was changed from ========== Reduce the binary size by removing the video/audio stack. The ...

3 years, 7 months ago (2017-05-04 01:06:31 UTC) #17

Zhi Huang

Description was changed from ========== The application can build WebRTC with data channel only by ...

3 years, 7 months ago (2017-05-04 01:07:05 UTC) #18

Zhi Huang

zhihuang@webrtc.org changed reviewers: + kjellander@webrtc.org

3 years, 7 months ago (2017-05-04 01:08:01 UTC) #19

Zhi Huang

Please take another look. Thanks. https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h#newcode65 webrtc/call/call.h:65: #endif On 2017/05/03 18:05:53, ...

3 years, 7 months ago (2017-05-04 01:08:03 UTC) #20

Please take another look. Thanks.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h
File webrtc/call/call.h (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h#newco...
webrtc/call/call.h:65: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> Would it be easier to just not include this file if HAVE_MEDIA is false?

This is the base class of internal::Call and this header is part of the
"call_interface" target. The MediaController and WebRtcSession etc. are
depending on it. If removing this, there would be more ifdef in those files.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/call/call.h#newco...
webrtc/call/call.h:71: static const int kDefaultStartBitrateBps = -1;
On 2017/05/03 18:05:53, pthatcher1 wrote:
> It's confusing that we have an ifdef in two places for this (one in the .h and
> one in the .cc).  Can you just put both one the .cc?

This is because call.h is an abstract class and it is part of the
"call_interface" build target while the call.cc is part of "call" target. When
building WebRTC without media, we don't build the "call" target so I have to
give the const member a value to avoid link errors. I can move both into .h file
but not sure if it is better to set this value in the abstract class.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:137: "engine/webrtcvideoencoderfactory.h",
On 2017/05/03 22:50:57, Taylor Brandstetter wrote:
> What would it take to move these files into the "if (rtc_enable_media)"? It
> would be nice if we could do that, since then we'd have one build target
that's
> only SCTP stuff.

The dependency chain is:
peerconnection_integrationtests -> fakevideoencoerfactory ->
webrtcvideoencoderfactroy.

They will just affect the tests. We'll be fine assuming the tests are never
build in the data-only mode. Do you think we can assume that?

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:176: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> Would it be easier to just not include this file if HAVE_MEDIA is false?

Oh, I just realized that I've already excluded this file. None of these changes
are needed. :)

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/BUILD.gn#newco...
webrtc/pc/BUILD.gn:307: "sctputils_unittest.cc",
On 2017/05/03 22:50:57, Taylor Brandstetter wrote:
> Does sctputils_unittest.cc need to be here?

No, it doesn't. Actually, if we want to avoid the duplication of build targets,
these unittests targets don't have to be changed.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1617: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> Would it be easier to move the VoiceChannel and VideoChannel into their own
file
> and not include those if HAVE_MEDIA is false?

We have a dependency chain:
peerconnection -> audiortpreceiver -> voicechannel:basechannel. We still need
some ifdef to exclude the video channel. Plus the effort of splitting the file,
it feels to me that this way is eaiser. What do you think?

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.cc:157: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> Would it be easier to just not call InitMediaEngine_w in
ChannelManager::Init()?

I guess we still need to do something like this:

#ifdef HAVE_MEDIA
  initialized_ = worker_thread_->Invoke<bool>(
      RTC_FROM_HERE, Bind(&ChannelManager::InitMediaEngine_w, this));
#else
  initialized_ = true;
#endif

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:403: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> We might want an RtcEventLog even if we don't have media.  How much size does
it
> add?
> 
> Maybe have a separate HAVE_RTC_EVENT_LOG.

With RtcEvenLog which depends on the rtc_rtcp module, the size would be ~2.77Mb.
Before adding an additional build flag, one thing I need to figure out is that
if the internal application have the control on these flag since they are not
using gn. If they don't, I think it would better to avoid adding new flags. What
do you think?

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectionfactory.cc:412: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> Why not just not call CreateMediaEngine_w from
> PeerConnectionFactory::Initialize()?

The idea here is trying to avoid compiling and linking the method
WebRtcMediaFactory::Create(). It seems that not calling it from Initialize()
doesn't help.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/sdk/android/src/j...
File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/peerconnection_jni.cc:1548: #endif
On 2017/05/03 18:05:53, pthatcher1 wrote:
> Won't the encoder and decoder factories by null so you don't need to do this
> (here and below)?

The idea here is trying to avoid linking the method SetEGLContext();

kjellander_webrtc

It looks a bit weird with rtc_media target checking if(rtc_enable_media) etc. Maybe we can find ...

3 years, 7 months ago (2017-05-04 14:01:46 UTC) #21

the sun

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

3 years, 7 months ago (2017-05-04 14:28:57 UTC) #22

the sun

https://codereview.webrtc.org/2854123003/diff/160001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2854123003/diff/160001/webrtc/pc/mediasession.cc#newcode1275 webrtc/pc/mediasession.cc:1275: channel_manager->GetSupportedDataCodecs(&data_codecs_); Should this really be excluded? https://codereview.webrtc.org/2854123003/diff/160001/webrtc/pc/peerconnection.cc File webrtc/pc/peerconnection.cc ...

3 years, 7 months ago (2017-05-04 14:28:58 UTC) #23

Zhi Huang

On 2017/05/04 14:01:46, kjellander_webrtc wrote: > It looks a bit weird with rtc_media target checking ...

3 years, 7 months ago (2017-05-04 21:28:10 UTC) #25

Zhi Huang

Please take another look. Thanks! https://codereview.webrtc.org/2854123003/diff/160001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/2854123003/diff/160001/webrtc/pc/mediasession.cc#newcode1275 webrtc/pc/mediasession.cc:1275: channel_manager->GetSupportedDataCodecs(&data_codecs_); On 2017/05/04 14:28:58, ...

3 years, 7 months ago (2017-05-04 21:28:53 UTC) #26

pthatcher1

This looks pretty good to me. https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn#newcode137 webrtc/media/BUILD.gn:137: "engine/webrtcvideoencoderfactory.h", On 2017/05/04 ...

3 years, 7 months ago (2017-05-05 01:45:13 UTC) #27

This looks pretty good to me.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:137: "engine/webrtcvideoencoderfactory.h",
On 2017/05/04 01:08:02, Zhi Huang wrote:
> On 2017/05/03 22:50:57, Taylor Brandstetter wrote:
> > What would it take to move these files into the "if (rtc_enable_media)"? It
> > would be nice if we could do that, since then we'd have one build target
> that's
> > only SCTP stuff.
> 
> The dependency chain is:
> peerconnection_integrationtests -> fakevideoencoerfactory ->
> webrtcvideoencoderfactroy.
> 
> They will just affect the tests. We'll be fine assuming the tests are never
> build in the data-only mode. Do you think we can assume that?

If they do, it will be really easy to know and fix it (the build would be
broken), so I think it would be OK to make that assumption.

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1617: #endif
On 2017/05/04 01:08:03, Zhi Huang wrote:
> On 2017/05/03 18:05:53, pthatcher1 wrote:
> > Would it be easier to move the VoiceChannel and VideoChannel into their own
> file
> > and not include those if HAVE_MEDIA is false?
> 
> We have a dependency chain:
> peerconnection -> audiortpreceiver -> voicechannel:basechannel. We still need
> some ifdef to exclude the video channel. Plus the effort of splitting the
file,
> it feels to me that this way is eaiser. What do you think?

It seems like we could do something where we make pure interfaces that
PeerConnection relies on and then we wouldn't need this file at all. We would
just need to change ChannelManager to return nulls (or not use ChannelManager at
all).  

Is it worth it?  To just remove this ifdef, probably not.  But if it saves more
space, maybe it is worth it.  It might be worth trying something really quick to
see if it does save space (change VoiceChannel, VideoChannel, and RtpDataChannel
into interfaces and don't compile in any impls).

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/120001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectionfactory.cc:412: #endif
On 2017/05/04 01:08:03, Zhi Huang wrote:
> On 2017/05/03 18:05:53, pthatcher1 wrote:
> > Why not just not call CreateMediaEngine_w from
> > PeerConnectionFactory::Initialize()?
> 
> The idea here is trying to avoid compiling and linking the method
> WebRtcMediaFactory::Create(). It seems that not calling it from Initialize()
> doesn't help.

Ah, duh.  You're right.

kjellander_webrtc

https://codereview.webrtc.org/2854123003/diff/200001/webrtc/BUILD.gn File webrtc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/200001/webrtc/BUILD.gn#newcode129 webrtc/BUILD.gn:129: if (rtc_enable_media) { Naming this rtc_enable_audio_video is better but ...

3 years, 7 months ago (2017-05-05 06:30:07 UTC) #28

pthatcher1

On 2017/05/05 06:30:07, kjellander_webrtc wrote: > https://codereview.webrtc.org/2854123003/diff/200001/webrtc/BUILD.gn > File webrtc/BUILD.gn (right): > > https://codereview.webrtc.org/2854123003/diff/200001/webrtc/BUILD.gn#newcode129 > ...

3 years, 7 months ago (2017-05-05 21:31:34 UTC) #29

Zhi Huang

Description was changed from ========== The application can build WebRTC with data channel only by ...

3 years, 7 months ago (2017-05-10 01:04:32 UTC) #42

kjellander_webrtc

I don't have time to finish the review right now but you cannot trust the ...

3 years, 7 months ago (2017-05-10 14:20:38 UTC) #43

Zhi Huang

zhihuang@webrtc.org changed reviewers: + sakal@webrtc.org

3 years, 7 months ago (2017-05-10 21:00:34 UTC) #45

Zhi Huang

Please take another look. Thanks. https://codereview.webrtc.org/2854123003/diff/460001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/460001/webrtc/media/BUILD.gn#newcode114 webrtc/media/BUILD.gn:114: rtc_static_library("rtc_media_base") { On 2017/05/10 ...

3 years, 7 months ago (2017-05-10 21:00:36 UTC) #46

Please take another look. Thanks.

https://codereview.webrtc.org/2854123003/diff/460001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/460001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:114: rtc_static_library("rtc_media_base") {
On 2017/05/10 14:20:37, kjellander_webrtc wrote:
> Can we avoid the duplication here and split out the common parts into smaller
> targets (rtc_source_set) ?

Yeah, I can do it here but I need "check_includes = false".

There are some similar cases in other places. I didn't extract the common part
for libjingle_peerconnection_jni target because there are not too many
duplicated files and I'm not sure if it is a good idea to disable the include
check there as well.

https://codereview.webrtc.org/2854123003/diff/460001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:129: "base/adaptedvideotracksource.cc",
On 2017/05/10 14:20:37, kjellander_webrtc wrote:
> I thought duplicating the same sources in multiple targets like this was a
build
> error or at least warning, but it doesn't seem to be the case. I guess it's
> because the rtc_media_base_datachannel_only target is not built by default,
> since it doesn't seem to be a part of the dependency chain down from the
> //:default target in
> https://chromium.googlesource.com/external/webrtc/+/master/BUILD.gn
> 
> This is a fortunate side-effect of the work I did in
> https://bugs.chromium.org/p/webrtc/issues/detail?id=6440 but it also makes
these
> targets actually be built if you specify their names explicitly on the command
> line (if you specify nothing, only  //:default is built)

Yes, this is very fortunate. 

But we probably want the _datachannel_only targets and tests to be built on the
trybots so that they won't be accidentally broken.

Then the question is how can we avoid building same file in different targets
since we need to build those files with #ifdef using different "defines" sets to
get different behavior.

Taylor Brandstetter

This seems like the smallest possible diff that accomplishes the most size savings, I'm impressed. ...

3 years, 7 months ago (2017-05-11 04:43:12 UTC) #47

sakal

Only looked at webrtc/sdk/android/ https://codereview.webrtc.org/2854123003/diff/500001/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/500001/webrtc/sdk/android/BUILD.gn#newcode25 webrtc/sdk/android/BUILD.gn:25: rtc_static_library("libjingle_peerconnection_datachannel_only_jni") { Can we just ...

3 years, 7 months ago (2017-05-11 11:06:12 UTC) #48

magjed_webrtc

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

3 years, 7 months ago (2017-05-11 13:02:01 UTC) #49

magjed_webrtc

https://codereview.webrtc.org/2854123003/diff/500001/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/500001/webrtc/sdk/android/BUILD.gn#newcode25 webrtc/sdk/android/BUILD.gn:25: rtc_static_library("libjingle_peerconnection_datachannel_only_jni") { This approach does not look composable. Can ...

3 years, 7 months ago (2017-05-11 13:02:02 UTC) #50

Zhi Huang

https://codereview.webrtc.org/2854123003/diff/500001/webrtc/call/call.h File webrtc/call/call.h (right): https://codereview.webrtc.org/2854123003/diff/500001/webrtc/call/call.h#newcode68 webrtc/call/call.h:68: static const int kDefaultStartBitrateBps = 300000; On 2017/05/11 04:43:11, ...

3 years, 7 months ago (2017-05-12 20:05:34 UTC) #52

Zhi Huang

Please take a look. https://codereview.webrtc.org/2854123003/diff/560001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/560001/webrtc/pc/BUILD.gn#newcode108 webrtc/pc/BUILD.gn:108: rtc_static_library("libjingle_peerconnection_audio") { Use this when ...

3 years, 7 months ago (2017-05-18 03:55:39 UTC) #53

sakal_google.com

sakal@google.com changed reviewers: + sakal@google.com

3 years, 7 months ago (2017-05-18 13:19:36 UTC) #54

sakal_google.com

I think this approach looks good in principle. We should figure out a better naming ...

3 years, 7 months ago (2017-05-18 13:19:37 UTC) #55

sakal_google.com

sakal@google.com changed reviewers: - sakal@google.com

3 years, 7 months ago (2017-05-18 13:22:40 UTC) #56

pthatcher

pthatcher@google.com changed reviewers: + pthatcher@google.com

3 years, 7 months ago (2017-05-18 17:50:24 UTC) #57

pthatcher

https://codereview.webrtc.org/2854123003/diff/560001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/560001/webrtc/pc/BUILD.gn#newcode108 webrtc/pc/BUILD.gn:108: rtc_static_library("libjingle_peerconnection_audio") { Perhaps a better name would be "audio". ...

3 years, 7 months ago (2017-05-18 17:50:27 UTC) #58

Taylor Brandstetter

This looks good as a proof of concept. Though if we decide to go further ...

3 years, 7 months ago (2017-05-18 17:57:06 UTC) #59

Zhi Huang

Please take another look. Thanks! https://codereview.webrtc.org/2854123003/diff/560001/webrtc/media/base/codec_video_nullimpl.cc File webrtc/media/base/codec_video_nullimpl.cc (right): https://codereview.webrtc.org/2854123003/diff/560001/webrtc/media/base/codec_video_nullimpl.cc#newcode16 webrtc/media/base/codec_video_nullimpl.cc:16: return false; On 2017/05/18 ...

3 years, 7 months ago (2017-05-23 03:40:36 UTC) #62

sakal

https://codereview.webrtc.org/2854123003/diff/640001/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/640001/webrtc/sdk/android/BUILD.gn#newcode34 webrtc/sdk/android/BUILD.gn:34: rtc_static_library("webrtc_jni_common") { nit: Why do this targets begin with ...

3 years, 7 months ago (2017-05-23 09:33:19 UTC) #63

Zhi Huang

Please take another look. Thanks! https://codereview.webrtc.org/2854123003/diff/640001/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/640001/webrtc/sdk/android/BUILD.gn#newcode34 webrtc/sdk/android/BUILD.gn:34: rtc_static_library("webrtc_jni_common") { On 2017/05/23 ...

3 years, 7 months ago (2017-05-23 18:49:46 UTC) #64

Zhi Huang

Description was changed from ========== The application can build WebRTC with data channel only by ...

3 years, 7 months ago (2017-05-23 18:51:59 UTC) #65

Zhi Huang

Hello everyone, Do you have any more comments on this?

3 years, 7 months ago (2017-05-25 18:12:27 UTC) #66

sakal

I think think this is good enough to land and more refactoring can be done ...

3 years, 7 months ago (2017-05-26 08:16:13 UTC) #67

magjed_webrtc

Thank you for taking the time to start cleaning up peerconnection_jni.cc! I think this approach ...

3 years, 7 months ago (2017-05-26 09:43:13 UTC) #68

the sun

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn#newcode48 webrtc/media/BUILD.gn:48: rtc_source_set("rtc_media_data_base") { "..._data_base" sounds like "database" was misspelled. Suggest ...

3 years, 6 months ago (2017-05-29 13:55:32 UTC) #69

kjellander_webrtc

This is really exciting progress! I have mainly minor comments - it's pretty close to ...

3 years, 6 months ago (2017-05-29 20:59:16 UTC) #70

Taylor Brandstetter

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/common_video/h264/profile_level_id.h File webrtc/common_video/h264/profile_level_id.h (right): https://codereview.webrtc.org/2854123003/diff/680001/webrtc/common_video/h264/profile_level_id.h#newcode13 webrtc/common_video/h264/profile_level_id.h:13: Should there be a TODO to delete this file? ...

3 years, 6 months ago (2017-05-30 17:30:53 UTC) #71

Zhi Huang

Please take another look. Thanks. Sorry for making this CL so large. https://codereview.webrtc.org/2854123003/diff/680001/webrtc/common_video/h264/profile_level_id.h File webrtc/common_video/h264/profile_level_id.h ...

3 years, 6 months ago (2017-05-31 00:03:29 UTC) #76

Please take another look. Thanks.
Sorry for making this CL so large.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/common_video/h264...
File webrtc/common_video/h264/profile_level_id.h (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/common_video/h264...
webrtc/common_video/h264/profile_level_id.h:13: 
On 2017/05/30 17:30:52, Taylor Brandstetter wrote:
> Should there be a TODO to delete this file?

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:48: rtc_source_set("rtc_media_data_base") {
On 2017/05/29 13:55:32, the sun wrote:
> "..._data_base" sounds like "database" was misspelled. Suggest flipping these
> names to "rtc_media_base_data" etc.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:82: "base/videoadapter.cc",
On 2017/05/29 13:55:32, the sun wrote:
> The fact that there are things here in the "data" target concerning video and
> audio is confusing. A comment would be good, or a separate sub target. It is
> conceivable to want to build without data support as well.

Agreed. I'll leave a comment and a TODO here for now. It seems that it needs
more work to split them further without disabling the include header check.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:258: # Enabling GN check triggers cyclic dependency error:
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> Is this comment still correct? I suspect you'll get another cycle if you
enable
> check_includes. 

We can enable the check here. The circular dependency only exists in
rtc_media_base_data.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/media/BUILD.gn#ne...
webrtc/media/BUILD.gn:313: # Enabling GN check triggers cyclic dependency error:
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> Is this comment still correct? I suspect you'll get another cycle if you
enable
> check_includes.  Please update it accordingly.

We can enable the check here. The circular dependency only exists in
rtc_media_base_data.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn#newco...
webrtc/pc/BUILD.gn:95: rtc_static_library("webrtc_null_audio") {
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> How about using rtc instead of webrtc in all these target names? It's what we
> use in many places.

Sounds good to me.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn#newco...
webrtc/pc/BUILD.gn:95: rtc_static_library("webrtc_null_audio") {
On 2017/05/30 17:30:52, Taylor Brandstetter wrote:
> It would be useful to have a comment explaining how the "modular build
targets"
> are expected to be used. 

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn#newco...
webrtc/pc/BUILD.gn:447: check_includes = false  # TODO(kjellander): Remove
(bugs.webrtc.org/6828)
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> Is it possible to avoid this?

Yep. :)

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn#newco...
webrtc/pc/BUILD.gn:506: 
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> Remove blank line.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/audiofactory.cc
File webrtc/pc/audiofactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/audiofactory.c...
webrtc/pc/audiofactory.cc:19: rtc::scoped_refptr<AudioEncoderFactory>
CreateBuiltinAudioEncoderFactoryImpl() {
On 2017/05/30 17:30:53, Taylor Brandstetter wrote:
> I still don't understand why the extra level of indirection is needed; we now
> have CreateBuiltinAudioEncoderFactoryImpl which calls
> CreateBuiltinAudioEncoderFactory which calls
> CreateBuiltinAudioEncoderFactoryInternal.

I'll remove these methods.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/channelmanager.cc
File webrtc/pc/channelmanager.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/channelmanager...
webrtc/pc/channelmanager.cc:94: "the media_engine_ is unset.";
On 2017/05/30 17:30:53, Taylor Brandstetter wrote:
> Will this occur during normal circumstances in a data channel-only build? If
so,
> the "warning" log level (or even "info") may be too severe.

I'll remove these.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/mediafactory.cc
File webrtc/pc/mediafactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/mediafactory.c...
webrtc/pc/mediafactory.cc:48: }
On 2017/05/30 17:30:53, Taylor Brandstetter wrote:
> I think a level of indirection can be removed here too, by defining
> Call::Create, RtcEventLog::Create and WebRtcMediaEngineFactory::Create
directly
> here. I don't think this file should ever end up linked with a file that has
> their actual implementations.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:2344: << "Failed to create the Call because the
media engine is unset.";
On 2017/05/30 17:30:53, Taylor Brandstetter wrote:
> Other comment about log levels applies here too.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnection_datachannelonly_unittest.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection_datachannelonly_unittest.cc:152: #ifdef HAVE_SCTP
On 2017/05/30 17:30:53, Taylor Brandstetter wrote:
> I think this #ifdef can be removed; there's no reason to run this test without
> SCTP support.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/peerconnection...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/peerconnection...
webrtc/pc/peerconnectionfactory.cc:73: return
CreatePeerConnectionFactory(CreateBuiltinAudioEncoderFactoryImpl(),
On 2017/05/29 13:55:32, the sun wrote:
> I think we could reduce the build target clutter and size of this CL a bit
with
> a slightly different approach:
> 
> Instead of adding these functions which are "compile time polymorphic", remove
> the "CreatePeerConnectionFactory(...)" functions from this file (and the PCF
> interface) and keep just the one which is most flexible. Then create separate
> build targets to implement these convenience methods, one for the standard
> functions which add audio+video+data, another one which calls new
> rtc::RefCountedObject<PeerConnectionFactory>(...) with empty scoped_refptr:s
for
> the audio codec factories, etc.
> 
> It looks like CreateMediaEngine() could be forwarded to PCF as a factory
method
> in the same way.

If I understand correctly, basically you are suggesting that instead of having
helper functions like CreateBuiltinAudioEncoderFactoryImpl, we can have
different implementations of this constructor in different targets and the user
can choose the constructors by linking against different targets.

This should make it cleaner here.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD.gn
File webrtc/sdk/android/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:57: rtc_static_library("webrtc_audio_jni") {
On 2017/05/26 08:16:13, sakal wrote:
> nit: I don't like webrtc_ prefix in any of the names. I don't think it serves
> any purpose and is just redundant.

Done.
I just not good at naming things. :)

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:57: rtc_static_library("webrtc_audio_jni") {
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> On 2017/05/26 08:16:13, sakal wrote:
> > nit: I don't like webrtc_ prefix in any of the names. I don't think it
serves
> > any purpose and is just redundant.
> 
> +1
> let's at least use rtc_ instead, or nothing at all

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:126: "//webrtc:webrtc_common",
On 2017/05/29 20:59:15, kjellander_webrtc wrote:
> There's no reason to use relative mixed with absolute (but the pattern was
> already established).
> Since this code is not used from Chromium - could you convert all these paths
to
> be absolute? Same below.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD...
webrtc/sdk/android/BUILD.gn:235:
rtc_shared_library("libjingle_peerconnection_datachannelonly_so") {
On 2017/05/30 17:30:53, Taylor Brandstetter wrote:
> Again, would be useful to have a comment that summarizes the various build
> targets, and explains that this is an example of how the "null" targets are
> used.

This is only for testing and it would be removed eventually. I'll add comments
in other places.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/j...
File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/peerconnection_jni.cc:1334: JOW(jlong,
PeerConnectionFactory_nativeCreateVideoSource)
On 2017/05/26 09:43:13, magjed_webrtc wrote:
> I would like to move this to video_jni.cc. I don't think you even need to add
a
> null implementation in null_video_jni.cc, as long as this function isn't
called
> for non-video builds (you will get a runtime UnsatisfiedLinkError exception in
> Java if you try to call it).

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/peerconnection_jni.cc:1353: JOW(void,
PeerConnectionFactory_nativeInitializeVideoCapturer)
On 2017/05/26 09:43:13, magjed_webrtc wrote:
> Same here, move to video_jni.cc.

I didn't move this method entirely to video_jni.cc because it relies on the
global j_application_context. If we do that, we may need to move more things to
video_jni.cc. It feels to me that this work can be done in a separate CL. Does
it make sense to you?

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/peerconnection_jni.cc:1384: JOW(jlong,
PeerConnectionFactory_nativeCreateVideoTrack)(
On 2017/05/26 09:43:13, magjed_webrtc wrote:
> Move to video_jni.cc.

Done.

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/peerconnection_jni.cc:1453: JOW(void,
PeerConnectionFactory_nativeSetVideoHwAccelerationOptions)(
On 2017/05/26 09:43:13, magjed_webrtc wrote:
> Move to video_jni.cc.

Done.

Zhi Huang

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/680001/webrtc/pc/BUILD.gn#newcode95 webrtc/pc/BUILD.gn:95: rtc_static_library("webrtc_null_audio") { On 2017/05/29 20:59:15, kjellander_webrtc wrote: > How ...

3 years, 6 months ago (2017-05-31 03:55:38 UTC) #79

magjed_webrtc

webrtc/sdk/android lgtm https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/jni/peerconnection_jni.cc File webrtc/sdk/android/src/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/src/jni/peerconnection_jni.cc#newcode1353 webrtc/sdk/android/src/jni/peerconnection_jni.cc:1353: JOW(void, PeerConnectionFactory_nativeInitializeVideoCapturer) On 2017/05/31 00:03:29, Zhi Huang ...

3 years, 6 months ago (2017-05-31 09:40:59 UTC) #80

kjellander_webrtc

lgtm from a build/dependency perspective. There's minor cleanup that can be done but it doesn't ...

3 years, 6 months ago (2017-06-01 05:34:31 UTC) #81

Zhi Huang

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

3 years, 6 months ago (2017-06-01 20:35:55 UTC) #82

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 20:36:01 UTC) #83

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 20:37:58 UTC) #84

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-01 20:38:03 UTC) #85

Zhi Huang

https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD.gn File webrtc/sdk/android/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/680001/webrtc/sdk/android/BUILD.gn#newcode235 webrtc/sdk/android/BUILD.gn:235: rtc_shared_library("libjingle_peerconnection_datachannelonly_so") { On 2017/06/01 05:34:30, kjellander_webrtc wrote: > On ...

3 years, 6 months ago (2017-06-02 05:16:43 UTC) #87

kjellander_webrtc

Responded to comments. I also mailed you about the linux_internal bot errors and how to ...

3 years, 6 months ago (2017-06-02 06:46:18 UTC) #88

Zhi Huang

Hello Fredrik, Do you have any more comments on this? It also need your approval ...

3 years, 6 months ago (2017-06-02 18:25:08 UTC) #90

the sun

Yes, more comments, but it is looking better each patch set! :) https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h File webrtc/logging/rtc_event_log/rtc_event_log.h ...

3 years, 6 months ago (2017-06-05 14:27:15 UTC) #91

Yes, more comments, but it is looking better each patch set! :)

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
webrtc/logging/rtc_event_log/rtc_event_log.h:194: // The platform_file is open
and needs to be closed.
The best solution would be to declare the interface in rtc_event_log.h, then
have two separate .h files for the Create() methods - one for the null impl, one
for the "true" impl. The two .h and their corresponding .cc files would then
live in separate build targets and there would be no need for doing work in this
.h file.

This is a minor comment in this context, though.

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/media/BUILD.gn#n...
webrtc/media/BUILD.gn:40: "../api/audio_codecs:audio_codecs_api",
How is base/audiosource.h dependent on audio codecs? I don't see anything such
in it.

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:103: rtc_static_library("pc_null_audio") {
This is much better, but there's still a little too much magic for my taste.

First, the file name "nullaudiofactory.cc" does not sound like it would contain
implementations of CreatePeerConnectionFactory() (neither does audiofactory.cc).

Second, we can do this without the "build-time polymorphism". :) If we removed
all but one of the CreatePeerConnectionFactory() declarations from
peerconnectioninterface.h and instead put them in separate .h/.cc/targets, the
client could just depend on the target which declares the CreatePCF() function
it needs. The resulting structure would be a simple tree. The CreatePCF() which
still lives in peerconnectioninterface.h is the one where everything can be
plugged in.

How the Create() methods should be clustered is not obvious to me, but note that
if we'd already had the above structure, a client with a different need could
have just rolled their own create method, pulling from the available
implementations of factories, loggers, etc.

Zhi Huang

zhihuang@webrtc.org changed reviewers: + skvlad@webrtc.org

3 years, 6 months ago (2017-06-06 03:09:49 UTC) #92

Zhi Huang

Responded the comments. https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h File webrtc/logging/rtc_event_log/rtc_event_log.h (right): https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h#newcode194 webrtc/logging/rtc_event_log/rtc_event_log.h:194: // The platform_file is open and ...

3 years, 6 months ago (2017-06-06 03:09:51 UTC) #93

Responded the comments.

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
webrtc/logging/rtc_event_log/rtc_event_log.h:194: // The platform_file is open
and needs to be closed.
On 2017/06/05 14:27:15, the sun wrote:
> The best solution would be to declare the interface in rtc_event_log.h, then
> have two separate .h files for the Create() methods - one for the null impl,
one
> for the "true" impl. The two .h and their corresponding .cc files would then
> live in separate build targets and there would be no need for doing work in
this
> .h file.
> 
> This is a minor comment in this context, though.

I agree that separating the interface and the subclass implementation is a
better solution.

I think what we need here is moving the implementation of RtcEventLogNullImpl
out of target "rtc_event_log_impl" so that we can depend on this without
introducing other unnecessary stuff. We can do this by either moving the
nullimpl to rtc_event_log_api or making a separate target like
"rtc_event_log_null_impl". 

Firstly, I don't think we can move the declaration of RtcEventLogNullImpl out of
this header because the internal project is depending on the RtcEventLogNullImpl
through this header and we don't want the interface header to depend on the
implementation header either. So we cannot easily have a separate target for the
impl and we need to move the impl to api target instead. 

Beside what I have done here, what we can do is having an rtceventlognullimpl.cc
which implements the RtcEventLogNullImpl and making the "rtc_event_log_api"
target depend on this .cc file to avoid linking error. I can do that if that
looks better to you.

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/media/BUILD.gn#n...
webrtc/media/BUILD.gn:40: "../api/audio_codecs:audio_codecs_api",
On 2017/06/05 14:27:15, the sun wrote:
> How is base/audiosource.h dependent on audio codecs? I don't see anything such
> in it.

I'll remove this.

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:103: rtc_static_library("pc_null_audio") {
On 2017/06/05 14:27:15, the sun wrote:
> This is much better, but there's still a little too much magic for my taste.
> 
> First, the file name "nullaudiofactory.cc" does not sound like it would
contain
> implementations of CreatePeerConnectionFactory() (neither does
audiofactory.cc).
> 
> Second, we can do this without the "build-time polymorphism". :) If we removed
> all but one of the CreatePeerConnectionFactory() declarations from
> peerconnectioninterface.h and instead put them in separate .h/.cc/targets, the
> client could just depend on the target which declares the CreatePCF() function
> it needs. The resulting structure would be a simple tree. The CreatePCF()
which
> still lives in peerconnectioninterface.h is the one where everything can be
> plugged in.
> 
> How the Create() methods should be clustered is not obvious to me, but note
that
> if we'd already had the above structure, a client with a different need could
> have just rolled their own create method, pulling from the available
> implementations of factories, loggers, etc.

Sorry that I didn't understand your previous suggestion correctly before.

I think the "build-time polymorphism" is what we need to make this approach
scalable. If we move the implementation of CreatePeerConnectionFactory method to
separate targets then we would need more special targets for different
functionalities (audio_only, video_only, audio_data etc.) in the future.

For an instance, once we have injectable video encoder/decoder in the future, we
would need four implementations in four build targets for the method
CreatePCF():

  CreatePCF(create_builtin_audio(), create_builtin_video()),
  CreatePCF(create_builtin_audio(), nullptr),
  CreatePCF(nullptr, create_builtin_video()),
  CreatePCF(nullptr. nullptr).

But with current approach, we only need an extra "null_audio" target which make
the create_builtin_audio() return a nullptr and an extra "null_video" target
which make create_builtin_video() return a nullptr.

Beside, wouldn't removing method declarations from peerconnectioninterface break
the downstream applications?

skvlad

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h File webrtc/logging/rtc_event_log/rtc_event_log.h (right): https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h#newcode194 webrtc/logging/rtc_event_log/rtc_event_log.h:194: // The platform_file is open and needs to be ...

3 years, 6 months ago (2017-06-06 17:49:21 UTC) #94

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
File webrtc/logging/rtc_event_log/rtc_event_log.h (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
webrtc/logging/rtc_event_log/rtc_event_log.h:194: // The platform_file is open
and needs to be closed.
On 2017/06/06 03:09:51, Zhi Huang wrote:
> On 2017/06/05 14:27:15, the sun wrote:
> > The best solution would be to declare the interface in rtc_event_log.h, then
> > have two separate .h files for the Create() methods - one for the null impl,
> one
> > for the "true" impl. The two .h and their corresponding .cc files would then
> > live in separate build targets and there would be no need for doing work in
> this
> > .h file.
> > 
> > This is a minor comment in this context, though.
> 
> I agree that separating the interface and the subclass implementation is a
> better solution.
> 
> I think what we need here is moving the implementation of RtcEventLogNullImpl
> out of target "rtc_event_log_impl" so that we can depend on this without
> introducing other unnecessary stuff. We can do this by either moving the
> nullimpl to rtc_event_log_api or making a separate target like
> "rtc_event_log_null_impl". 
> 
> Firstly, I don't think we can move the declaration of RtcEventLogNullImpl out
of
> this header because the internal project is depending on the
RtcEventLogNullImpl
> through this header and we don't want the interface header to depend on the
> implementation header either. So we cannot easily have a separate target for
the
> impl and we need to move the impl to api target instead. 
> 
> Beside what I have done here, what we can do is having an
rtceventlognullimpl.cc
> which implements the RtcEventLogNullImpl and making the "rtc_event_log_api"
> target depend on this .cc file to avoid linking error. I can do that if that
> looks better to you.

I think putting the function inline into the header is a good tradeoff to avoid
introducing another file for the null implementation (which would be a small but
breaking change for existing users). Especially since all our other functions
are already inline.

the sun

On 2017/06/06 17:49:21, skvlad wrote: > https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h > File webrtc/logging/rtc_event_log/rtc_event_log.h (right): > > https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_event_log/rtc_event_log.h#newcode194 > ...

3 years, 6 months ago (2017-06-06 19:21:32 UTC) #95

On 2017/06/06 17:49:21, skvlad wrote:
>
https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
> File webrtc/logging/rtc_event_log/rtc_event_log.h (right):
> 
>
https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/logging/rtc_even...
> webrtc/logging/rtc_event_log/rtc_event_log.h:194: // The platform_file is open
> and needs to be closed.
> On 2017/06/06 03:09:51, Zhi Huang wrote:
> > On 2017/06/05 14:27:15, the sun wrote:
> > > The best solution would be to declare the interface in rtc_event_log.h,
then
> > > have two separate .h files for the Create() methods - one for the null
impl,
> > one
> > > for the "true" impl. The two .h and their corresponding .cc files would
then
> > > live in separate build targets and there would be no need for doing work
in
> > this
> > > .h file.
> > > 
> > > This is a minor comment in this context, though.
> > 
> > I agree that separating the interface and the subclass implementation is a
> > better solution.
> > 
> > I think what we need here is moving the implementation of
RtcEventLogNullImpl
> > out of target "rtc_event_log_impl" so that we can depend on this without
> > introducing other unnecessary stuff. We can do this by either moving the
> > nullimpl to rtc_event_log_api or making a separate target like
> > "rtc_event_log_null_impl". 
> > 
> > Firstly, I don't think we can move the declaration of RtcEventLogNullImpl
out
> of
> > this header because the internal project is depending on the
> RtcEventLogNullImpl
> > through this header and we don't want the interface header to depend on the
> > implementation header either. So we cannot easily have a separate target for
> the
> > impl and we need to move the impl to api target instead. 
> > 
> > Beside what I have done here, what we can do is having an
> rtceventlognullimpl.cc
> > which implements the RtcEventLogNullImpl and making the "rtc_event_log_api"
> > target depend on this .cc file to avoid linking error. I can do that if that
> > looks better to you.
> 
> I think putting the function inline into the header is a good tradeoff to
avoid
> introducing another file for the null implementation (which would be a small
but
> breaking change for existing users). Especially since all our other functions
> are already inline.

Agree that this tradeoff makes sense.

the sun

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn#newcode103 webrtc/pc/BUILD.gn:103: rtc_static_library("pc_null_audio") { On 2017/06/06 03:09:51, Zhi Huang wrote: > ...

3 years, 6 months ago (2017-06-06 19:30:34 UTC) #96

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:103: rtc_static_library("pc_null_audio") {
On 2017/06/06 03:09:51, Zhi Huang wrote:
> On 2017/06/05 14:27:15, the sun wrote:
> > This is much better, but there's still a little too much magic for my taste.
> > 
> > First, the file name "nullaudiofactory.cc" does not sound like it would
> contain
> > implementations of CreatePeerConnectionFactory() (neither does
> audiofactory.cc).
> > 
> > Second, we can do this without the "build-time polymorphism". :) If we
removed
> > all but one of the CreatePeerConnectionFactory() declarations from
> > peerconnectioninterface.h and instead put them in separate .h/.cc/targets,
the
> > client could just depend on the target which declares the CreatePCF()
function
> > it needs. The resulting structure would be a simple tree. The CreatePCF()
> which
> > still lives in peerconnectioninterface.h is the one where everything can be
> > plugged in.
> > 
> > How the Create() methods should be clustered is not obvious to me, but note
> that
> > if we'd already had the above structure, a client with a different need
could
> > have just rolled their own create method, pulling from the available
> > implementations of factories, loggers, etc.
> 
> Sorry that I didn't understand your previous suggestion correctly before.
>
> I think the "build-time polymorphism" is what we need to make this approach
> scalable. If we move the implementation of CreatePeerConnectionFactory method
to
> separate targets then we would need more special targets for different
> functionalities (audio_only, video_only, audio_data etc.) in the future.
> 
> For an instance, once we have injectable video encoder/decoder in the future,
we
> would need four implementations in four build targets for the method
> CreatePCF():
> 
>   CreatePCF(create_builtin_audio(), create_builtin_video()),
>   CreatePCF(create_builtin_audio(), nullptr),
>   CreatePCF(nullptr, create_builtin_video()),
>   CreatePCF(nullptr. nullptr).

The point I'd like us to get to is to *not* maintain all these variants in the
webrtc tree, but to make it easy enough for clients to pull in things from the
api/ smorgasbord and plug together themselves. We'll keep a few commonly used
variants, and they can then serve as examples.

Less complexity, less work, more flexibility, for more clients.

> But with current approach, we only need an extra "null_audio" target which
make
> the create_builtin_audio() return a nullptr and an extra "null_video" target
> which make create_builtin_video() return a nullptr.
> 
> Beside, wouldn't removing method declarations from peerconnectioninterface
break
> the downstream applications?

Maybe temporarily. What we could do to avoid breakage:

1. peerconnectioninterface.h in a new target which is also considered public.
2. Keep only the largest-superset incarnation of CreatePCF in it.
3. Keep existing CreatePCF in the current target, which then depends on the new
target
4. Add new targets for the commonly used CreatePCF methods we decide that we
need.

Zhi Huang

On 2017/06/06 19:30:34, the sun wrote: > https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn > File webrtc/pc/BUILD.gn (right): > > https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn#newcode103 ...

3 years, 6 months ago (2017-06-06 22:22:12 UTC) #97

On 2017/06/06 19:30:34, the sun wrote:
> https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn
> File webrtc/pc/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2854123003/diff/1020001/webrtc/pc/BUILD.gn#newc...
> webrtc/pc/BUILD.gn:103: rtc_static_library("pc_null_audio") {
> On 2017/06/06 03:09:51, Zhi Huang wrote:
> > On 2017/06/05 14:27:15, the sun wrote:
> > > This is much better, but there's still a little too much magic for my
taste.
> > > 
> > > First, the file name "nullaudiofactory.cc" does not sound like it would
> > contain
> > > implementations of CreatePeerConnectionFactory() (neither does
> > audiofactory.cc).
> > > 
> > > Second, we can do this without the "build-time polymorphism". :) If we
> removed
> > > all but one of the CreatePeerConnectionFactory() declarations from
> > > peerconnectioninterface.h and instead put them in separate .h/.cc/targets,
> the
> > > client could just depend on the target which declares the CreatePCF()
> function
> > > it needs. The resulting structure would be a simple tree. The CreatePCF()
> > which
> > > still lives in peerconnectioninterface.h is the one where everything can
be
> > > plugged in.
> > > 
> > > How the Create() methods should be clustered is not obvious to me, but
note
> > that
> > > if we'd already had the above structure, a client with a different need
> could
> > > have just rolled their own create method, pulling from the available
> > > implementations of factories, loggers, etc.
> > 
> > Sorry that I didn't understand your previous suggestion correctly before.
> >
> > I think the "build-time polymorphism" is what we need to make this approach
> > scalable. If we move the implementation of CreatePeerConnectionFactory
method
> to
> > separate targets then we would need more special targets for different
> > functionalities (audio_only, video_only, audio_data etc.) in the future.
> > 
> > For an instance, once we have injectable video encoder/decoder in the
future,
> we
> > would need four implementations in four build targets for the method
> > CreatePCF():
> > 
> >   CreatePCF(create_builtin_audio(), create_builtin_video()),
> >   CreatePCF(create_builtin_audio(), nullptr),
> >   CreatePCF(nullptr, create_builtin_video()),
> >   CreatePCF(nullptr. nullptr).
> 
> The point I'd like us to get to is to *not* maintain all these variants in the
> webrtc tree, but to make it easy enough for clients to pull in things from the
> api/ smorgasbord and plug together themselves. We'll keep a few commonly used
> variants, and they can then serve as examples.
> 
> Less complexity, less work, more flexibility, for more clients.

I agree that this would work for c++ users. 

But for Android users, it would require us to build a set of parallel wrapper
targets for every single use case since they don't have much control on the
native code.

I don't see why current approach is less flexible. It would also allow the user
to plug in the components based on they requirements.

Maybe I missed something. It would be great if you can make a little snippet. :)

> 
> > But with current approach, we only need an extra "null_audio" target which
> make
> > the create_builtin_audio() return a nullptr and an extra "null_video" target
> > which make create_builtin_video() return a nullptr.
> > 
> > Beside, wouldn't removing method declarations from peerconnectioninterface
> break
> > the downstream applications?
> 
> Maybe temporarily. What we could do to avoid breakage:
> 
> 1. peerconnectioninterface.h in a new target which is also considered public.
> 2. Keep only the largest-superset incarnation of CreatePCF in it.
> 3. Keep existing CreatePCF in the current target, which then depends on the
new
> target
> 4. Add new targets for the commonly used CreatePCF methods we decide that we
> need.

Zhi Huang

Please take another look. Thanks. There might be places that need to be clean up ...

3 years, 6 months ago (2017-06-12 05:59:28 UTC) #101

Zhi Huang

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

3 years, 6 months ago (2017-06-12 06:15:02 UTC) #102

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-12 06:15:14 UTC) #103

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-12 06:22:24 UTC) #104

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-12 06:22:26 UTC) #105

the sun

Generally looks good - biggest thing is there are still link-time layered function implementations that ...

3 years, 6 months ago (2017-06-12 20:58:41 UTC) #106

the sun

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/createpeerconnectionfactory.cc File webrtc/pc/createpeerconnectionfactory.cc (right): https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/createpeerconnectionfactory.cc#newcode93 webrtc/pc/createpeerconnectionfactory.cc:93: std::unique_ptr<CallFactoryInterface> call_factory = CreateCallFactory(); On 2017/06/12 20:58:41, the sun ...

3 years, 6 months ago (2017-06-12 21:33:26 UTC) #107

Zhi Huang

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/api/mediastreaminterface.h File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/api/mediastreaminterface.h#newcode27 webrtc/api/mediastreaminterface.h:27: #include "webrtc/base/ratetracker.h" On 2017/06/12 20:58:40, the sun wrote: > ...

3 years, 6 months ago (2017-06-12 22:18:58 UTC) #108

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/api/mediastreami...
File webrtc/api/mediastreaminterface.h (right):

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/api/mediastreami...
webrtc/api/mediastreaminterface.h:27: #include "webrtc/base/ratetracker.h"
On 2017/06/12 20:58:40, the sun wrote:
> How does this belong here? It should be included where it is used.

Many internal projects depend on these header through "mediachannel.h" which we
need to remove to break circular dependenies.  Can we just keep it for now? I'll
 leave a TODO to remove those once the headers are added to downstream apps.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/api/peerconnecti...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/api/peerconnecti...
webrtc/api/peerconnectioninterface.h:1134: CreateModularPeerConnectionFactory(
On 2017/06/12 20:58:41, the sun wrote:
> Can we do without this variant? I'd ultimately like just one create method
here
> (the rest are "convenience" methods).

I guess we could.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:187: # The target "create_pc_factory_datachannelonly" is an
example.
On 2017/06/12 20:58:41, the sun wrote:
> I'd think create_pc_factory is example enough and the implementation(s) used
in
> the JNI layer are "application" specific.

Acknowledged.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:218: rtc_static_library("create_pc_factory_datachannelonly")
{
On 2017/06/12 20:58:41, the sun wrote:
> Is it possible to move these variants to the JNI code? IIUC we have "link time
> polymorphism" again here. I understand we need it for the Java layer, to
support
> the different configs, but lets keep it there then.

So you are suggesting to compile this file in the jni target?  I think that
would work if the JNI owner is OK with it.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:485: rtc_test("peerconnection_datachannelonly_unittests") {
On 2017/06/12 20:58:41, the sun wrote:
> You shouldn't need a separate target for this - the test could be part of the
> default pc test suite. If all link time polymorphism is gone, the compiler
will
> instead be used to verify how PCF is instantiated (and we could conceivably
have
> different configurations at once, within the same process).

Acknowledged.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/createpeercon...
File webrtc/pc/createpeerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/createpeercon...
webrtc/pc/createpeerconnectionfactory.cc:93:
std::unique_ptr<CallFactoryInterface> call_factory = CreateCallFactory();
On 2017/06/12 21:33:26, the sun wrote:
> On 2017/06/12 20:58:41, the sun wrote:
> > Is there any way we could avoid the extra interface for the call factory and
> > just send in a lambda, function pointer, etc? Agree that requiring the
caller
> of
> > CreateModularPCF() to have created the Call on the right thread isn't a good
> > idea.
> 
> You could use
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/functi...

Sorry, I didn't get it. Are you suggestion passing in something like a function
ptr instead of the call_factory so that we don't need to call
call_factory.CreateCall() and we should replace all the "call_factory" with a
function ptr then?

Is it OK to use a factory here? It feels natural to me to pass in a factory when
creating a peerconnectionfactory.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/peerconnectio...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnectionfactory.cc:41:
rtc::scoped_refptr<PeerConnectionFactoryInterface> CreatePeerConnectionFactory(
On 2017/06/12 20:58:41, the sun wrote:
> Does this need to be here? I find it odd that it calls a function defined in
> createpeerconnectionfactory.cc

I'll move these impl to  createpeerconnectionfactory.cc.

https://codereview.webrtc.org/2854123003/diff/1160001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnectionfactory.cc:356: std::unique_ptr<RtcEventLog>
event_log(new RtcEventLogNullImpl());
On 2017/06/12 20:58:41, the sun wrote:
> Could you send in the event log directly instead of the factory? A client not
> wanting it would call "new RtcEventLogNullImpl()" in their CreatePCF call.

I think the peerconnectionfactory need a RtcEventLogFactory because each
PeerConnection need their own RtcEventLog object. Same reason for having a
CallFactory there.

Zhi Huang

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

3 years, 6 months ago (2017-06-13 06:33:22 UTC) #112

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-13 06:33:28 UTC) #113

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-13 06:38:57 UTC) #114

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-13 06:38:59 UTC) #115

kjellander_webrtc

lgtm from a build perspective % updating CL description and fixing the failing PeerConnectionFactoryTestInternal.CreatePCUsingInternalModules test. ...

3 years, 6 months ago (2017-06-13 08:04:51 UTC) #116

the sun

lgtm https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/BUILD.gn#newcode187 webrtc/pc/BUILD.gn:187: # The target "create_pc_factory_datachannelonly" is an example. nit: ...

3 years, 6 months ago (2017-06-13 14:34:39 UTC) #118

the sun

Oh, and THANKS for your patience! On 2017/06/13 14:34:39, the sun wrote: > lgtm > ...

3 years, 6 months ago (2017-06-13 14:36:13 UTC) #119

magjed_webrtc

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc File webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc (right): https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc#newcode1 webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc:1: /* This is a file I haven't seen before ...

3 years, 6 months ago (2017-06-13 15:14:31 UTC) #120

Zhi Huang

On 2017/06/13 15:14:31, magjed_webrtc wrote: > https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc > File webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc > (right): > > https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc#newcode1 ...

3 years, 6 months ago (2017-06-13 17:26:21 UTC) #121

On 2017/06/13 15:14:31, magjed_webrtc wrote:
>
https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
> File webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc
> (right):
> 
>
https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
> webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc:1:
/*
> This is a file I haven't seen before and it's not something I would like to
have
> since it doesn't scale. I thought we agreed to do link-time polymorphism for
> Android since we don't expose passing in modules at the moment. So I would
like
> you to add an audio_jni.cc file and a null_audio_jni.cc file similar to the
> existing video jni files. And in these files you implement different versions
of
> CreateAudioEncoderFactory() and similar for all the other audio modules that
are
> arguments to CreateModularPeerConnectionFactory. In peerconnection_jni.cc, you
> have only one way to call CreateModularPeerConnectionFactory(), but where all
> arguments are replaced with these link-time polymorphism functions.
> 
> This link-time polymorphism solution is for Android only, and I understand you
> want to do it differently for C++. In the future, we would like to move away
> from the link-time polymorphism for Android as well once we can pass in
> corresponding Java modules.
> 
> Also, can you split out the changes for webrtc/sdk/android into a separate CL?
> It's incredibly hard to follow and review this huge CL. Also, it will be
easier
> for you if you have to revert part of the changes in the future if you split
> this huge CL up into multiple CLs.

Thanks, everyone!

For the different CreatePCF method, I think we still need them since the
declarations are still in the peerconnectioninterface.h for backward
compatibility, but I can move them to null_audio_jni though, thanks for pointing
this out Magus.

For linux_internal, I need to make another internal CL to modified the
configuration. I've test this internally already.

I'll land the native change first and make another CL for android.

skvlad

lgtm for logging On 2017/06/13 17:26:21, Zhi Huang wrote: > On 2017/06/13 15:14:31, magjed_webrtc wrote: ...

3 years, 6 months ago (2017-06-13 17:43:14 UTC) #122

lgtm for logging

On 2017/06/13 17:26:21, Zhi Huang wrote:
> On 2017/06/13 15:14:31, magjed_webrtc wrote:
> >
>
https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
> > File
webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc
> > (right):
> > 
> >
>
https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
> > webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc:1:
> /*
> > This is a file I haven't seen before and it's not something I would like to
> have
> > since it doesn't scale. I thought we agreed to do link-time polymorphism for
> > Android since we don't expose passing in modules at the moment. So I would
> like
> > you to add an audio_jni.cc file and a null_audio_jni.cc file similar to the
> > existing video jni files. And in these files you implement different
versions
> of
> > CreateAudioEncoderFactory() and similar for all the other audio modules that
> are
> > arguments to CreateModularPeerConnectionFactory. In peerconnection_jni.cc,
you
> > have only one way to call CreateModularPeerConnectionFactory(), but where
all
> > arguments are replaced with these link-time polymorphism functions.
> > 
> > This link-time polymorphism solution is for Android only, and I understand
you
> > want to do it differently for C++. In the future, we would like to move away
> > from the link-time polymorphism for Android as well once we can pass in
> > corresponding Java modules.
> > 
> > Also, can you split out the changes for webrtc/sdk/android into a separate
CL?
> > It's incredibly hard to follow and review this huge CL. Also, it will be
> easier
> > for you if you have to revert part of the changes in the future if you split
> > this huge CL up into multiple CLs.
> 
> Thanks, everyone!
> 
> For the different CreatePCF method, I think we still need them since the
> declarations are still in the peerconnectioninterface.h for backward
> compatibility, but I can move them to null_audio_jni though, thanks for
pointing
> this out Magus.
> 
> For linux_internal, I need to make another internal CL to modified the
> configuration. I've test this internally already.
> 
> I'll land the native change first and make another CL for android.

Taylor Brandstetter

Sorry it took me a while to review this again in its entirety. So, my ...

3 years, 6 months ago (2017-06-14 01:54:16 UTC) #123

Zhi Huang

zhihuang@webrtc.org changed reviewers: + zstein@webrtc.org

3 years, 6 months ago (2017-06-14 06:56:58 UTC) #127

Zhi Huang

PTAL. https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/BUILD.gn#newcode187 webrtc/pc/BUILD.gn:187: # The target "create_pc_factory_datachannelonly" is an example. On ...

3 years, 6 months ago (2017-06-14 06:57:01 UTC) #128

PTAL.

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:187: # The target "create_pc_factory_datachannelonly" is an
example.
On 2017/06/13 14:34:38, the sun wrote:
> nit: comment referes to nonexistent target

Done.

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/peerconnection.h
File webrtc/pc/peerconnection.h (right):

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnection.h:68: Call* call);
On 2017/06/13 14:34:39, the sun wrote:
> Shouldn't this be a unique_ptr too?

I just want to match the return type of Call::Create().
But using unique_ptr looks better.

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/peerconnectio...
File webrtc/pc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnectionfactory.h:108: // Deprecated. Use the method with
media_engine, call_factory_, and
On 2017/06/13 14:34:39, the sun wrote:
> This comment would be great in peerconnectioninterface.h, for the CreatePCF
> methods

Agree. I would like to keep the comments here as well for the native code
developers.

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
File webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc
(right):

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc:1: /*
On 2017/06/13 15:14:31, magjed_webrtc wrote:
> This is a file I haven't seen before and it's not something I would like to
have
> since it doesn't scale. I thought we agreed to do link-time polymorphism for
> Android since we don't expose passing in modules at the moment. So I would
like
> you to add an audio_jni.cc file and a null_audio_jni.cc file similar to the
> existing video jni files. And in these files you implement different versions
of
> CreateAudioEncoderFactory() and similar for all the other audio modules that
are
> arguments to CreateModularPeerConnectionFactory. In peerconnection_jni.cc, you
> have only one way to call CreateModularPeerConnectionFactory(), but where all
> arguments are replaced with these link-time polymorphism functions.
> 
> This link-time polymorphism solution is for Android only, and I understand you
> want to do it differently for C++. In the future, we would like to move away
> from the link-time polymorphism for Android as well once we can pass in
> corresponding Java modules.
> 
> Also, can you split out the changes for webrtc/sdk/android into a separate CL?
> It's incredibly hard to follow and review this huge CL. Also, it will be
easier
> for you if you have to revert part of the changes in the future if you split
> this huge CL up into multiple CLs.

Agree. It would be much better to put them to null_audio_jni.cc

BTW, I would like to keep the datachannelonly_so target as an example. It also
can help us to make sure the modular build won't be broken accidentally.

https://codereview.webrtc.org/2854123003/diff/1240001/webrtc/sdk/android/src/...
webrtc/sdk/android/src/jni/createpeerconnectionfactory_datachannelonly.cc:20:
rtc::scoped_refptr<PeerConnectionFactoryInterface> CreatePeerConnectionFactory(
On 2017/06/13 14:34:39, the sun wrote:
> Do you really need all of these functions? Isn't the JNI just calling one of
> them?

You are right. I'll try to remove the unused ones.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/api/peerconnecti...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/api/peerconnecti...
webrtc/api/peerconnectioninterface.h:1026: //
On 2017/06/14 01:54:15, Taylor Brandstetter wrote:
> I don't think we need to deprecate CreatePeerConnectionFactory; we can keep
both
> around for both "basic" and "advanced" users.

Acknowledged.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/api/peerconnecti...
webrtc/api/peerconnectioninterface.h:1150: // own them.
On 2017/06/14 01:54:15, Taylor Brandstetter wrote:
> This logic related to threads is pretty confusing. I'd prefer if we could
change
> it to just "if any thread is null, one will be created internally as needed."

I think your suggestion is not equivalent to the original logic.

The original logic is creating and own all of then or creating none of them.
This is why we only have one flag |own_ptr|.

Would it be slightly better to do:

  if (!network_thread || !signaling_thread) {
...
}

Or maybe we can just have two ctors and two CreateModularPCF() which would be
more clear I think.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/call/callfactory...
File webrtc/call/callfactoryinterface.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/call/callfactory...
webrtc/call/callfactoryinterface.h:19: 
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> This and event_log_factory_interface.h could use some comments explaining why
> they exist. Also, shouldn't they go in the api/ directory?

Done.

I'm confused about where this interface should go because it seems that the STO
teams is working on moving the files from api/call/ to call/. Can we keep it
here for now and make it part of call_interface target?
https://codereview.webrtc.org/2550273003

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/call/callfactory...
webrtc/call/callfactoryinterface.h:27: std::unique_ptr<CallFactoryInterface>
CreateCallFactory();
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> nit: newline before '}'

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/common_video/h26...
File webrtc/common_video/h264/profile_level_id.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/common_video/h26...
webrtc/common_video/h264/profile_level_id.h:16: // TODO(zhihuang): Delete this
file once all the dependencies use the
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> nit: Applications that depend on us would be "dependents", not "dependencies"

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/media/BUILD.gn#n...
webrtc/media/BUILD.gn:315: 
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> Given the current approach of this CL, some of these build targets now seem
> unnecessary. I think all we really need for the time being are rtc_media,
> rtc_audio_video, rtc_data, and rtc_media_base. Could the "common_video"
> dependency be pushed up to "rtc_audio_video"?

Done. Now we only have rtc_media_base.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:186: # methods with different dependencies based on their
requirements.
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> This comment is somewhat confusing without the context of this CL.

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:215: # TODO(zhihuang): Remove this once the downstream
dependencies start using the
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> Again, I don't think we want to deprecate this yet. And "dependencies" vs
> "dependents".

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/createpeercon...
File webrtc/pc/createpeerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/createpeercon...
webrtc/pc/createpeerconnectionfactory.cc:23: class MediaEngineCreator {
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> Is this just because "rtc::Bind" requires a class? It seems like there must be
a
> better way.
> 
> Anyone else know of one?

I just wanna have something work here.

Maybe we can leave this at it is because once your CL is landed, we don't need
to create MediaEngine on the worker thread any more.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/createpeercon...
webrtc/pc/createpeerconnectionfactory.cc:65: worker_thread.release(), nullptr
/*signaling_thread*/,
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> So, this results in ownership of |worker_thread| being transferred,
specifically
> because |network_thread| is null? This is the part of
> CreateModularPeerConnectionFactory that's confusing. 

Again, I think this won't be an issue once your CL is landed and we can create
the MediaEngine on the signaling thread.

> Also, it would be nice if
> the code duplication between this and
CreatePeerConnectionFactoryWithAudioMixer
> could be avoided.

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/peerconnection.h
File webrtc/pc/peerconnection.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnection.h:64: explicit PeerConnection(PeerConnectionFactory*
factory);
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> Can we get rid of this constructor? I don't think anything but tests should be
> constructing a PeerConnection directly.

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/peerconnectio...
File webrtc/pc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnectionfactory.cc:25: // WebRTC build targets.
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> Really, the solution here is to put the interface and implementation in
separate
> build targets. Should add a TODO for that.

Done.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/peerconnectio...
File webrtc/pc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnectionfactory.h:109: // event_log_factory instead.
On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> Same as other comment: is there anything preventing us from getting rid of the
> other constructors in this CL?

If no other applications call this method directly, then we can remove it. I'll
try to remove it to see what happens internally.

Zhi Huang

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

3 years, 6 months ago (2017-06-14 06:57:49 UTC) #129

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-14 06:57:57 UTC) #130

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-14 07:07:46 UTC) #131

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-14 07:07:47 UTC) #132

Zhi Huang

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

3 years, 6 months ago (2017-06-14 19:14:55 UTC) #133

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-14 19:15:05 UTC) #134

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-14 19:24:25 UTC) #135

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-14 19:24:27 UTC) #136

Taylor Brandstetter

lgtm, with just some trivial remaining comments. It would be nice if the stuff related ...

3 years, 6 months ago (2017-06-15 00:35:02 UTC) #137

lgtm, with just some trivial remaining comments. It would be nice if the stuff
related to threads could be resolved, but if
https://codereview.webrtc.org/2934103002/ ends up taking multiple rounds of
review, we should just go ahead with this CL and leave it as a TODO to be fixed
soon afterwards.

Also, remember to fill out the CL description before landing, with enough detail
that people who haven't been involved in all the discussions will understand
what we're doing. You could start by copying the explanation from the comment
above CreateModularPeerConnectionFactory.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/api/peerconnecti...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/api/peerconnecti...
webrtc/api/peerconnectioninterface.h:1150: // own them.
On 2017/06/14 06:57:01, Zhi Huang wrote:
> On 2017/06/14 01:54:15, Taylor Brandstetter wrote:
> > This logic related to threads is pretty confusing. I'd prefer if we could
> change
> > it to just "if any thread is null, one will be created internally as
needed."
> 
> I think your suggestion is not equivalent to the original logic.
> 
> The original logic is creating and own all of then or creating none of them.
> This is why we only have one flag |own_ptr|.
> 
> Would it be slightly better to do:
> 
>   if (!network_thread || !signaling_thread) {
> ...
> }
> 
> Or maybe we can just have two ctors and two CreateModularPCF() which would be
> more clear I think.
> 

We already talked about this in person, but for the benefit of other code
reviewers... The trouble now is that there are three possibilities:

1. Passing in null pointer; thread created automatically
2. Passing in a pointer to a thread that the caller owns
3. Passing in a pointer to a thread that the PC factory should own

#3 is needed because WebRtcMediaEngine needs to be constructed on the worker
thread. So CreatePeerConnectionFactory needs to create the worker thread itself,
create the media engine on it, then pass both into PeerConnectionFactory. That's
why landing this CL would be good: https://codereview.webrtc.org/2934103002/

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/call/callfactory...
File webrtc/call/callfactoryinterface.h (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/call/callfactory...
webrtc/call/callfactoryinterface.h:19: 
On 2017/06/14 06:57:01, Zhi Huang wrote:
> On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> > This and event_log_factory_interface.h could use some comments explaining
why
> > they exist. Also, shouldn't they go in the api/ directory?
> 
> Done.
> 
> I'm confused about where this interface should go because it seems that the
STO
> teams is working on moving the files from api/call/ to call/. Can we keep it
> here for now and make it part of call_interface target?
> https://codereview.webrtc.org/2550273003

Don't worry about moving the files for now. I'll plan on fleshing out the
"slim/modular" plan and working out where files go while I'm in STO.

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/createpeercon...
File webrtc/pc/createpeerconnectionfactory.cc (right):

https://codereview.webrtc.org/2854123003/diff/1260001/webrtc/pc/createpeercon...
webrtc/pc/createpeerconnectionfactory.cc:23: class MediaEngineCreator {
On 2017/06/14 06:57:01, Zhi Huang wrote:
> On 2017/06/14 01:54:16, Taylor Brandstetter wrote:
> > Is this just because "rtc::Bind" requires a class? It seems like there must
be
> a
> > better way.
> > 
> > Anyone else know of one?
> 
> I just wanna have something work here.
> 
> Maybe we can leave this at it is because once your CL is landed, we don't need
> to create MediaEngine on the worker thread any more.

Acknowledged.

https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/BUILD.gn
File webrtc/pc/BUILD.gn (right):

https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/BUILD.gn#newc...
webrtc/pc/BUILD.gn:186: # requrements. See comment in
peerconnectionfactoryinterface.h.
Just noticed my own typo, "requrements"

https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/peerconnectio...
File webrtc/pc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/peerconnectio...
webrtc/pc/peerconnectionfactory.h:132: std::unique_ptr<rtc::Thread>
owned_signaling_thread_;
|owned_signaling_thread_| isn't used, because rather than the signaling thread
being created/destroyed, it's wrapped/unwrapped, which is kept track of by
|wraps_current_thread_|.

Taylor Brandstetter

https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/BUILD.gn#newcode187 webrtc/pc/BUILD.gn:187: rtc_static_library("create_pc_factory") { If you change this to rtc_source_set, the ...

3 years, 6 months ago (2017-06-15 07:31:04 UTC) #138

kjellander_webrtc

Replying to question https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/BUILD.gn File webrtc/pc/BUILD.gn (right): https://codereview.webrtc.org/2854123003/diff/1340001/webrtc/pc/BUILD.gn#newcode187 webrtc/pc/BUILD.gn:187: rtc_static_library("create_pc_factory") { On 2017/06/15 07:31:04, Taylor ...

3 years, 6 months ago (2017-06-15 09:08:50 UTC) #139

Taylor Brandstetter

Here's a CL that fixes the build issue (hopefully; just started CQ dry run), and ...

3 years, 6 months ago (2017-06-15 17:35:03 UTC) #140

Zhi Huang

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

3 years, 6 months ago (2017-06-15 18:29:39 UTC) #143

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-15 18:29:45 UTC) #144

Zhi Huang

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

3 years, 6 months ago (2017-06-15 18:38:38 UTC) #145

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-15 18:38:47 UTC) #146

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-15 19:05:24 UTC) #147

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 6 months ago (2017-06-15 19:05:26 UTC) #148

Zhi Huang

Description was changed from ========== Detailed description will be added. BUG=webrtc:7613 ========== to ========== Add ...

3 years, 6 months ago (2017-06-15 19:41:14 UTC) #149

Zhi Huang

Description was changed from ========== Add support to build WebRTC without audio and video. This ...

3 years, 6 months ago (2017-06-15 19:42:04 UTC) #150

Zhi Huang

Description was changed from ========== Support building WebRTC without audio and video. This CL make ...

3 years, 6 months ago (2017-06-15 19:46:17 UTC) #151

Zhi Huang

Description was changed from ========== Support building WebRTC without audio and video. This CL makes ...

3 years, 6 months ago (2017-06-15 19:47:21 UTC) #152

Zhi Huang

Description was changed from ========== Support building WebRTC without audio and video. This CL makes ...

3 years, 6 months ago (2017-06-15 19:47:41 UTC) #153

Zhi Huang

Description was changed from ========== Support building WebRTC without audio and video. This CL makes ...

3 years, 6 months ago (2017-06-15 19:48:06 UTC) #154

Zhi Huang

Description was changed from ========== Support building WebRTC without audio and video. This CL makes ...

3 years, 6 months ago (2017-06-15 19:49:08 UTC) #155

Zhi Huang

The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org, skvlad@webrtc.org, magjed@webrtc.org, solenberg@webrtc.org, ...

3 years, 6 months ago (2017-06-15 19:49:50 UTC) #157

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-15 19:50:04 UTC) #158

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1400001, "attempt_start_ts": 1497556190012340, "parent_rev": "beae92c784b8a6bfe2479f012a1be89ff3a955bb", "commit_rev": "38ede130420ee741f97214a1d87dbee227056554"}

3 years, 6 months ago (2017-06-15 19:52:34 UTC) #159

commit-bot: I haz the power

Description was changed from ========== Support building WebRTC without audio and video. This CL makes ...

3 years, 6 months ago (2017-06-15 19:52:45 UTC) #160

commit-bot: I haz the power

3 years, 6 months ago (2017-06-15 19:52:48 UTC) #161

Message was sent while issue was closed.

Committed patchset #35 (id:1400001) as
https://chromium.googlesource.com/external/webrtc/+/38ede130420ee741f97214a1d...

Issue 2854123003: Build WebRTC with data channel only. (Closed)

Description

Patch Set 1 : Hacky Build #

Patch Set 2 : Futher optimization to 2.42Mb #

Patch Set 3 : Less hacky version. #

Patch Set 4 : Merge. #

Patch Set 5 : Address the comments. #

Patch Set 6 : Modified the rtc_event_log. Responded the comments. #

Patch Set 7 : Build parallel datachannel_only targets. #

Patch Set 8 : Add nogncheck to peerconnection_jni.cc. Make it 2.41Mb. #

Patch Set 9 : Add an end to end test for libjingle_peerconnection_datachannel_only. Make it 2.37Mb. #

Patch Set 10 : CR comments. #

Patch Set 11 : Reponses to the comments. #

Patch Set 12 : More modular apporach. Proof of concept. Need more work. #

Patch Set 13 : Renaming. #

Patch Set 14 : Fix the issues. Make it ready for another round of review. Add a test. #

Patch Set 15 : Rename the targets and response the comments. #

Patch Set 16 : Add a Java level test. #

Patch Set 17 : Address the review comments. #

Patch Set 18 : Fix the BUILD file generation error on win and mac. #

Patch Set 19 : Fix the issue on the trybots. #

Patch Set 20 : Rebase for the trybots. #

Patch Set 21 : Make it build with Chromium. #

Patch Set 22 : Replace the rtc_ prefix with webrtc_ to avoid naming conflict. #

Patch Set 23 : Resolve the comments and rename the targets in pc/. #

Patch Set 24 : Rebase. #

Patch Set 25 : Try the internal trybots. #

Patch Set 26 : Try to make it work internally. #

Patch Set 27 : Rebase #

Patch Set 28 : Remove the linking-time polymorphism. Add new CreatePeerConnectionFactory methods. Make Call and Rt… #

Patch Set 29 : Rebase. #

Patch Set 30 : address review comments. #

Patch Set 31 : Try to make work internallly. #

Patch Set 32 : Revert the android changes. #

Patch Set 33 : Address comments. #

Patch Set 34 : Thread issue. #

Patch Set 35 : Rebase. #

Messages