Change VideoTrack implementation to invoke VideoTrackSourceInterface::AddOrUpdateSink on wt

Change VideoTrack implementation to invoke VideoTrackSourceInterface::AddOrUpdateSink on the worker thread. 

Added documentation of thread expectations for video tracks and sources to the API.

BUG=None

Review-Url: https://codereview.webrtc.org/2964863002
Cr-Commit-Position: refs/heads/master@{#18938}
Committed: https://chromium.googlesource.com/external/webrtc/+/f1377f722271b4b5a090d4dba10201031d31f20d

[perkj_webrtc, 2017-07-03 19:58:33]

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

[commit-bot: I haz the power, 2017-07-03 19:58:40]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-03 20:04:31]

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

[commit-bot: I haz the power, 2017-07-03 20:04:31]

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

[perkj_webrtc, 2017-07-04 07:50:48]

Description was changed from

==========
Move creation of source proxy to PeerConnectionFactory::CreateVideoTrack.

BUG=
==========

to

==========
Move creation of source proxy to PeerConnectionFactory::CreateVideoTrack.

In order for custom VideoTrackSourceInterface implementations to inherit
AdaptedVideoTrackSource without also having to create a thread proxy, I suggest
we move creation of the source proxies to when the tracks are created. A track
will then only access the source implementation via the proxy.
==========

[perkj_webrtc, 2017-07-04 07:50:49]

perkj@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, noahric@chromium.org

[perkj_webrtc, 2017-07-05 05:44:20]

WDYT?

[Taylor Brandstetter, 2017-07-05 23:13:40]

https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
File webrtc/pc/peerconnectionfactory.cc (left):

https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
webrtc/pc/peerconnectionfactory.cc:255:
rtc::scoped_refptr<VideoTrackSourceInterface> source(
Hmm... I don't know if it's safe to do this, because existing applications may
be expecting the returned object to be a proxy. Would it be too egregious to end
up with an object wrapped in two layers of proxy? I guess we could add an
"is_proxy" method or something to avoid the double wrapping... No solution seems
perfect though.

Maybe we could add a method that creates a video track directly from a video
capturer, without going through the VideoTrackSourceInterface in the middle, and
deprecate this method? All the code I'm finding calls CreateVideoTrack directly
after CreateVideoSource, and doesn't hold onto the source pointer. Then we could
avoid the double-wrapping without any hacks.

[perkj_webrtc, 2017-07-06 15:08:20]

https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
File webrtc/pc/peerconnectionfactory.cc (left):

https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
webrtc/pc/peerconnectionfactory.cc:255:
rtc::scoped_refptr<VideoTrackSourceInterface> source(
On 2017/07/05 23:13:40, Taylor Brandstetter wrote:
> Hmm... I don't know if it's safe to do this, because existing applications may
> be expecting the returned object to be a proxy. Would it be too egregious to
end
> up with an object wrapped in two layers of proxy? I guess we could add an
> "is_proxy" method or something to avoid the double wrapping... No solution
seems
> perfect though.
> 
> Maybe we could add a method that creates a video track directly from a video
> capturer, without going through the VideoTrackSourceInterface in the middle,
and
> deprecate this method? All the code I'm finding calls CreateVideoTrack
directly
> after CreateVideoSource, and doesn't hold onto the source pointer. Then we
could
> avoid the double-wrapping without any hacks.

VideoTrackSourceInterface is supposed to be an implementation of a capturer and
the cricket::VideoCapturer thing should be deprecated. This function exist to
wrap old cricket::VideoCapturer implementations such as the one in Chrome.

Our Android implementation actually inherit AdaptedVideoTrackSource and create a
proxy itself. So if we go down this road, we should remove creating the proxy
here:
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/v...

An extra layer of proxies should be fine but is of course not optimal.
The proxy implementation checks the current thread and just invokes the function
if a thread hop is not needed. 
https://cs.chromium.org/chromium/src/third_party/webrtc/api/proxy.h?rcl=74848...

[Taylor Brandstetter, 2017-07-06 22:24:05]

https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
File webrtc/pc/peerconnectionfactory.cc (left):

https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
webrtc/pc/peerconnectionfactory.cc:255:
rtc::scoped_refptr<VideoTrackSourceInterface> source(
On 2017/07/06 15:08:20, perkj_webrtc wrote:
> On 2017/07/05 23:13:40, Taylor Brandstetter wrote:
> > Hmm... I don't know if it's safe to do this, because existing applications
may
> > be expecting the returned object to be a proxy. Would it be too egregious to
> end
> > up with an object wrapped in two layers of proxy? I guess we could add an
> > "is_proxy" method or something to avoid the double wrapping... No solution
> seems
> > perfect though.
> > 
> > Maybe we could add a method that creates a video track directly from a video
> > capturer, without going through the VideoTrackSourceInterface in the middle,
> and
> > deprecate this method? All the code I'm finding calls CreateVideoTrack
> directly
> > after CreateVideoSource, and doesn't hold onto the source pointer. Then we
> could
> > avoid the double-wrapping without any hacks.
> 
> VideoTrackSourceInterface is supposed to be an implementation of a capturer
and
> the cricket::VideoCapturer thing should be deprecated. This function exist to
> wrap old cricket::VideoCapturer implementations such as the one in Chrome.
> 
> Our Android implementation actually inherit AdaptedVideoTrackSource and create
a
> proxy itself. So if we go down this road, we should remove creating the proxy
> here:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/v...
> 
> An extra layer of proxies should be fine but is of course not optimal.
> The proxy implementation checks the current thread and just invokes the
function
> if a thread hop is not needed. 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/proxy.h?rcl=74848...
>  
> 
> 

So, what do you plan to do here then? If this method is deprecated anyway, maybe
biting the bullet and having two layers of proxies isn't so terrible.

I have another suggestion, though: why not have VideoTrack handle the thread
hopping itself, right here:
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/videotrack.cc?type...

It's calling AddOrUpdateSink in a loop for each sink. Which is suboptimal in the
first place, since it will result in multiple thread hops if there are multiple
sinks.

[perkj_webrtc, 2017-07-07 08:36:42]

Description was changed from

==========
Move creation of source proxy to PeerConnectionFactory::CreateVideoTrack.

In order for custom VideoTrackSourceInterface implementations to inherit
AdaptedVideoTrackSource without also having to create a thread proxy, I suggest
we move creation of the source proxies to when the tracks are created. A track
will then only access the source implementation via the proxy.
==========

to

==========
Change VideoTrack implementation to invoke
VideoTrackSourceInterface::AddOrUpdateSink on the worker thread. 

Added documentation of thread expectations for video tracks and sources to the
API.
==========

[perkj_webrtc, 2017-07-07 08:37:59]

On 2017/07/06 22:24:05, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
> File webrtc/pc/peerconnectionfactory.cc (left):
> 
>
https://codereview.webrtc.org/2964863002/diff/1/webrtc/pc/peerconnectionfacto...
> webrtc/pc/peerconnectionfactory.cc:255:
> rtc::scoped_refptr<VideoTrackSourceInterface> source(
> On 2017/07/06 15:08:20, perkj_webrtc wrote:
> > On 2017/07/05 23:13:40, Taylor Brandstetter wrote:
> > > Hmm... I don't know if it's safe to do this, because existing applications
> may
> > > be expecting the returned object to be a proxy. Would it be too egregious
to
> > end
> > > up with an object wrapped in two layers of proxy? I guess we could add an
> > > "is_proxy" method or something to avoid the double wrapping... No solution
> > seems
> > > perfect though.
> > > 
> > > Maybe we could add a method that creates a video track directly from a
video
> > > capturer, without going through the VideoTrackSourceInterface in the
middle,
> > and
> > > deprecate this method? All the code I'm finding calls CreateVideoTrack
> > directly
> > > after CreateVideoSource, and doesn't hold onto the source pointer. Then we
> > could
> > > avoid the double-wrapping without any hacks.
> > 
> > VideoTrackSourceInterface is supposed to be an implementation of a capturer
> and
> > the cricket::VideoCapturer thing should be deprecated. This function exist
to
> > wrap old cricket::VideoCapturer implementations such as the one in Chrome.
> > 
> > Our Android implementation actually inherit AdaptedVideoTrackSource and
create
> a
> > proxy itself. So if we go down this road, we should remove creating the
proxy
> > here:
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/sdk/android/src/jni/v...
> > 
> > An extra layer of proxies should be fine but is of course not optimal.
> > The proxy implementation checks the current thread and just invokes the
> function
> > if a thread hop is not needed. 
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/api/proxy.h?rcl=74848...
> >  
> > 
> > 
> 
> So, what do you plan to do here then? If this method is deprecated anyway,
maybe
> biting the bullet and having two layers of proxies isn't so terrible.
> 
> I have another suggestion, though: why not have VideoTrack handle the thread
> hopping itself, right here:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/videotrack.cc?type...
> 
> It's calling AddOrUpdateSink in a loop for each sink. Which is suboptimal in
the
> first place, since it will result in multiple thread hops if there are
multiple
> sinks.

Seems like a good idea. There is actually a potential race in VideoTrack at the
moment since its base VideoSource was used from two threads.

[perkj_webrtc, 2017-07-07 08:38:51]

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

[commit-bot: I haz the power, 2017-07-07 08:38:56]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-07 08:44:46]

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

[commit-bot: I haz the power, 2017-07-07 08:44:47]

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

[Taylor Brandstetter, 2017-07-07 16:02:02]

lgtm. Noah, do you expect any problems with this?

[noahric, 2017-07-07 16:41:01]

noahric@chromium.org changed reviewers:
+ yujo@chromium.org

[noahric, 2017-07-07 16:42:31]

On 2017/07/07 16:02:02, Taylor Brandstetter wrote:
> lgtm. Noah, do you expect any problems with this?

+jon for that. I'm not sure which things were happening off the worker thread,
but I had assumed AddOrUpdateSink would need to be invoked on the worker thread,
vs. just asserted. But he'd know better than me :)

[yujo, 2017-07-07 17:58:33]

On 2017/07/07 16:42:31, noahric wrote:
> On 2017/07/07 16:02:02, Taylor Brandstetter wrote:
> > lgtm. Noah, do you expect any problems with this?
> 
> +jon for that. I'm not sure which things were happening off the worker thread,
> but I had assumed AddOrUpdateSink would need to be invoked on the worker
thread,
> vs. just asserted. But he'd know better than me :)

Looks good, this should solve the issue we were having. I couldn't tell from the
conversation - is the idea to delete VideoTrackSourceProxy entirely then?

[perkj_webrtc, 2017-07-07 20:04:36]

On 2017/07/07 17:58:33, yujo wrote:
> On 2017/07/07 16:42:31, noahric wrote:
> > On 2017/07/07 16:02:02, Taylor Brandstetter wrote:
> > > lgtm. Noah, do you expect any problems with this?
> > 
> > +jon for that. I'm not sure which things were happening off the worker
thread,
> > but I had assumed AddOrUpdateSink would need to be invoked on the worker
> thread,
> > vs. just asserted. But he'd know better than me :)
> 
> Looks good, this should solve the issue we were having. I couldn't tell from
the
> conversation - is the idea to delete VideoTrackSourceProxy entirely then?

I think Chrome will still need  VideoTrackSourceProxy. It uses an implementation
of cricket::VideoCapturer  and not the AdaptedVideoTrackSource yet.

[perkj_webrtc, 2017-07-07 20:05:03]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2017-07-07 20:05:16]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-07 20:11:43]

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

[commit-bot: I haz the power, 2017-07-07 20:11:44]

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

[Taylor Brandstetter, 2017-07-07 23:34:57]

Description was changed from

==========
Change VideoTrack implementation to invoke
VideoTrackSourceInterface::AddOrUpdateSink on the worker thread. 

Added documentation of thread expectations for video tracks and sources to the
API.
==========

to

==========
Change VideoTrack implementation to invoke
VideoTrackSourceInterface::AddOrUpdateSink on the worker thread. 

Added documentation of thread expectations for video tracks and sources to the
API.

BUG=None
==========

[Taylor Brandstetter, 2017-07-07 23:35:04]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-07-07 23:35:14]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-07 23:38:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499470504835090,
"parent_rev": "cf39dd5d82a565f1ec5fd45453208adf4ee01407", "commit_rev":
"f1377f722271b4b5a090d4dba10201031d31f20d"}

[commit-bot: I haz the power, 2017-07-07 23:38:38]

Description was changed from

==========
Change VideoTrack implementation to invoke
VideoTrackSourceInterface::AddOrUpdateSink on the worker thread. 

Added documentation of thread expectations for video tracks and sources to the
API.

BUG=None
==========

to

==========
Change VideoTrack implementation to invoke
VideoTrackSourceInterface::AddOrUpdateSink on the worker thread. 

Added documentation of thread expectations for video tracks and sources to the
API.

BUG=None

Review-Url: https://codereview.webrtc.org/2964863002
Cr-Commit-Position: refs/heads/master@{#18938}
Committed:
https://chromium.googlesource.com/external/webrtc/+/f1377f722271b4b5a090d4dba...
==========

[commit-bot: I haz the power, 2017-07-07 23:38:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/f1377f722271b4b5a090d4dba...

[mbonadei, 2017-07-10 09:08:52]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2979493003/ by mbonadei@webrtc.org.

The reason for reverting is: It breaks a downstream project..