Implement and use VideoTrackSource directly.

Implement and use VideoTrackSource directly.

Removes dependency on PeerConnectionDependencyFactory for creating a
VideoTrackSource. Also implements VideoTrackSource::needs_denoising()
and wires it up to googNoiseReduction from getUserMedia().

This has less of an explicit dependency on cricket::VideoCapturer, and
the intent is to remove the dependency on cricket::VideoCapturer
completely as soon as the logic for AdaptFrame has been extracted.

BUG=chromium:645907
R=nisse@chromium.org, hta@chromium.org, perkj@chromium.org

Committed: https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c
Cr-Commit-Position: refs/heads/master@{#420947}

[pbos, 2016-09-20 00:27:48]

The CQ bit was checked by pbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 00:28:15]

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

[pbos, 2016-09-20 00:29:29]

PTAL, this fixes googNoiseReduction wireup. The mock for creating the local
track was unused (had a RTC_NOTREACHED()), so I don't believe test coverage is
dropped.

If you have any ideas for where adding testing would be appropriate let me know.

I also think that removing cricket::VideoCapturer should be easier after this
change. :)

[pbos, 2016-09-20 00:31:28]

On 2016/09/20 00:29:29, pbos wrote:
> PTAL, this fixes googNoiseReduction wireup. The mock for creating the local
> track was unused (had a RTC_NOTREACHED()), so I don't believe test coverage is
> dropped.
> 
> If you have any ideas for where adding testing would be appropriate let me
know.
> 
> I also think that removing cricket::VideoCapturer should be easier after this
> change. :)

This was manually tested using:
https://appr.tc/r/asdf?debug=loopback&video=googNoiseReduction=true, set to
true, false and removing it completely. Then I verified that that the
corresponding option in RtpSender was true, false and unset correspondingly,
using this patch:

diff --git a/api/rtpsender.cc b/api/rtpsender.cc
index 2e01d96..6f2ecfe 100644
--- a/api/rtpsender.cc
+++ b/api/rtpsender.cc
@@ -369,8 +369,15 @@ void VideoRtpSender::SetVideoSend() {
   cricket::VideoOptions options;
   VideoTrackSourceInterface* source = track_->GetSource();
   if (source) {
+    puts("Yes, it's a source!!");
     options.is_screencast = rtc::Optional<bool>(source->is_screencast());
     options.video_noise_reduction = source->needs_denoising();
+    if (options.video_noise_reduction) {
+      printf("Video noise reduction is set to: %s!\n",
+             *options.video_noise_reduction ? "true" : "false");
+    } else {
+      puts("Video noise reduction is NOT set!");
+    }
   }
   if (!channel_->SetVideoSend(ssrc_, track_->enabled(), &options, track_)) {
     RTC_DCHECK(false);

[commit-bot: I haz the power, 2016-09-20 00:49:13]

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

[commit-bot: I haz the power, 2016-09-20 00:49:13]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pbos, 2016-09-20 02:40:29]

Use a VideoTrackSourceProxy for the local track.

[pbos, 2016-09-20 02:41:12]

The CQ bit was checked by pbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 02:41:34]

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

[pbos, 2016-09-20 02:44:23]

Thread-hopping is scary, I hope I got it right now, I don't believe the capture
part needs a signalling/worker proxy

I believe this is the line I'm not friends with:

https://cs.chromium.org/chromium/src/third_party/webrtc/api/videotrack.cc?dr=...

Should we change that assumption and have VideoTrack marshal calls over to the
worker thread instead of requiring all objects to be proxies?

[commit-bot: I haz the power, 2016-09-20 02:56:57]

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

[commit-bot: I haz the power, 2016-09-20 02:56:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nisse-chromium (ooo August 14), 2016-09-20 07:28:25]

On 2016/09/20 00:29:29, pbos wrote:
> PTAL, this fixes googNoiseReduction wireup. The mock for creating the local
> track was unused (had a RTC_NOTREACHED()), so I don't believe test coverage is
> dropped.
> 
> If you have any ideas for where adding testing would be appropriate let me
know.
> 
> I also think that removing cricket::VideoCapturer should be easier after this
> change. :)

Does this mean that you don't need the hack I added in
https://codereview.webrtc.org/2334683002/? In that case, I guess we should
simply revert that.

Also have a look at https://codereview.webrtc.org/2328333002/ and see if that
fits your needs here.

(I haven't looked at the details yet).

[nisse-chromium (ooo August 14), 2016-09-20 08:34:09]

https://codereview.chromium.org/2356663002/diff/20001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/2356663002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:26: : public
webrtc::VideoTrackSource {
This is where you'd inherit the new class AdaptedVideoTrackSource, I think.

[perkj_chrome, 2016-09-20 08:43:45]

note that all public methods in the peerconnection / mediastream interfaces are
supposed to be called on libjingles signaling thread. That is not the same
thread as chromes main render thread and that is why we have the proxies around
all libjingle implementations of VideoTrack, PeerConnection, VideoTrackSource.

[pbos, 2016-09-20 13:17:37]

create MockWebRtcVideoTrack directly

[pbos, 2016-09-20 13:24:35]

PTAL

This would replace https://codereview.webrtc.org/2334683002/, I think it can be
reverted. I thought the change in webrtc/api/videocapturertracksource.cc would
be necessary, but we just stop using VideoCapturer altogether, and the change in
RtpSender was already there.

I think this should be able to be moved to AdaptedVideoTrackSource as soon as
that is in, but I think this change works stand-alone too. Let me know what you
think here.

Unless there's magic going on in the background, Chromium only calls into the
OnFrame method explicitly on the libjingle worker thread, so the hop is
currently already done.

VideoTrack expects VideoTrackSource to be a proxy object (e.g. no one calls
directly into the worker methods of  VideoTrackSource, but uses a proxy). To me
it'd make more sense if VideoTrackSource either expected calls on the signaling
thread (and marshaled them over to the worker thread internally) or if
VideoTrack marshaled them over to the worker thread since it knows that's what's
expected (per the comment at least).

[pbos, 2016-09-20 13:25:01]

The CQ bit was checked by pbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 13:25:10]

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

[commit-bot: I haz the power, 2016-09-20 13:50:02]

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

[commit-bot: I haz the power, 2016-09-20 13:50:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[pbos, 2016-09-20 15:41:17]

avoid instantiating PeerConnection in tests

[pbos, 2016-09-20 15:45:51]

The CQ bit was checked by pbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 15:46:15]

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

[hta - Chromium, 2016-09-20 16:33:43]

lgtm

Please add a TODO to test the passing of the denoiser constraint; there are no
changes to what's tested here.

https://codereview.chromium.org/2356663002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/2356663002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:42: return
needs_denoising_;
Can we get the API in place to change needs_denoising_ after creation?
We're going to need that for ApplyConstraints() even if it always fails.

https://codereview.chromium.org/2356663002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:264: // by
removing the need for and dependency on a circket::VideoCapturer.
spelling: circket -> cricket

[commit-bot: I haz the power, 2016-09-20 17:00:56]

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

[commit-bot: I haz the power, 2016-09-20 17:00:57]

Dry run: This issue passed the CQ dry run.

[pbos, 2016-09-20 17:13:47]

PTAL

https://codereview.chromium.org/2356663002/diff/60001/content/renderer/media/...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/2356663002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:42: return
needs_denoising_;
On 2016/09/20 16:33:43, hta - Chromium wrote:
> Can we get the API in place to change needs_denoising_ after creation?
> We're going to need that for ApplyConstraints() even if it always fails.

I'll defer that because it affects threading, as the values are const on
construction I don't have to care right now.

ApplyConstraints() is likely to have ramifications way further down the stack as
well (RtpSender needs to re-pull these options etc). Adding
set_needs_denoising() seems to only be a tiny part of it.

https://codereview.chromium.org/2356663002/diff/60001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:264: // by
removing the need for and dependency on a circket::VideoCapturer.
On 2016/09/20 16:33:43, hta - Chromium wrote:
> spelling: circket -> cricket

Done.

[pbos, 2016-09-20 17:13:55]

spelling

[pbos, 2016-09-20 17:26:59]

add test TODO

[pbos, 2016-09-23 14:01:48]

On 2016/09/20 17:26:59, pbos wrote:
> add test TODO

Ping, woooooo!

(PTAL nisse@/perkj@)

[nisse-chromium (ooo August 14), 2016-09-26 06:51:01]

LGTM. I don't understand the threading issues, though, I'll have to let Per
review that.

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:228:
needs_denoising = rtc::Optional<bool>(denoising_value);
Maybe move this logic closer to where needs_denoising is used, in the call to
WebRtcVideoCapturerAdapter?

[perkj_chrome, 2016-09-26 07:09:05]

lgtm but would like the test to be added in this cl or very soon.

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:221: //
TODO(pbos): Add tests that make sure that googNoiseReduction has properly
This class is currently tested in rtc_peer_connection_handler_unittest.cc and
the browser tests.

Can you do this todo now?

[pbos, 2016-09-26 17:11:44]

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:221: //
TODO(pbos): Add tests that make sure that googNoiseReduction has properly
On 2016/09/26 07:09:04, perkj_chrome wrote:
> This class is currently tested in rtc_peer_connection_handler_unittest.cc and
> the browser tests.
> 
> Can you do this todo now? 

Would like to do it in a follow-up, won't close the bug until it's tested.

https://codereview.chromium.org/2356663002/diff/100001/content/renderer/media...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:228:
needs_denoising = rtc::Optional<bool>(denoising_value);
On 2016/09/26 06:51:01, nisse-chromium wrote:
> Maybe move this logic closer to where needs_denoising is used, in the call to
> WebRtcVideoCapturerAdapter?
> 

Done this way to keep constraint extraction in one place.

[pbos, 2016-09-26 17:11:50]

The CQ bit was checked by pbos@chromium.org

[pbos, 2016-09-26 17:11:50]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@chromium.org
Link to the patchset: https://codereview.chromium.org/2356663002/#ps100001
(title: "add test TODO")

[commit-bot: I haz the power, 2016-09-26 17:12:30]

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

[commit-bot: I haz the power, 2016-09-26 18:45:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-26 18:49:36]

Description was changed from

==========
Implement and use VideoTrackSource directly.

Removes dependency on PeerConnectionDependencyFactory for creating a
VideoTrackSource. Also implements VideoTrackSource::needs_denoising()
and wires it up to googNoiseReduction from getUserMedia().

This has less of an explicit dependency on cricket::VideoCapturer, and
the intent is to remove the dependency on cricket::VideoCapturer
completely as soon as the logic for AdaptFrame has been extracted.

BUG=chromium:645907
R=nisse@chromium.org, hta@chromium.org, perkj@chromium.org
==========

to

==========
Implement and use VideoTrackSource directly.

Removes dependency on PeerConnectionDependencyFactory for creating a
VideoTrackSource. Also implements VideoTrackSource::needs_denoising()
and wires it up to googNoiseReduction from getUserMedia().

This has less of an explicit dependency on cricket::VideoCapturer, and
the intent is to remove the dependency on cricket::VideoCapturer
completely as soon as the logic for AdaptFrame has been extracted.

BUG=chromium:645907
R=nisse@chromium.org, hta@chromium.org, perkj@chromium.org

Committed: https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c
Cr-Commit-Position: refs/heads/master@{#420947}
==========

[commit-bot: I haz the power, 2016-09-26 18:49:37]

Patchset 6 (id:??) landed as
https://crrev.com/36f03f6131cb668e175cadd2d0407d09b33c199c
Cr-Commit-Position: refs/heads/master@{#420947}