Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame on source remova…

Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame on source removal.

BUG=webrtc:6371, webrtc:6983
Review-Url: https://codereview.webrtc.org/2469993003
Cr-Commit-Position: refs/heads/master@{#16048}
Committed: https://chromium.googlesource.com/external/webrtc/+/d533aec3b8425edd81c2743f7683c12b74c307b5

[perkj_webrtc, 2016-11-02 20:38:03]

perkj@webrtc.org changed reviewers:
+ mflodman@webrtc.org, nisse@webrtc.org

[perkj_webrtc, 2016-11-02 20:38:04]

Can we remove sending black frames?

[nisse-webrtc, 2016-11-07 09:52:46]

Regarding the black frame feature, I wonder who knows the background about that.
Harald, maybe?

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1910: bool encoder_sink_valid = sink
== encoder_sink_;
Style: I'd prefer a pair of () around the comparison.

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1910: bool encoder_sink_valid = sink
== encoder_sink_;
Hmm, this also appears to be the only use of the encoder_sink_ member. What
would it take to delete it? If I understand this correctly, it is intended to
handle a
race between application calling RemoveSink, and encoder calling AddOrUpdateSink
(from cpu adaptation logic). Right?

But how will the encoder ever call AddOrUpdateSink on this object? It looks like
you are hooking it up directly to the source, replacing SetSource(this, ...) by
SetSource(source_)? Then this race must be handled elsewhere. If there's no
better way, maybe we have to introduce a VideoSourceInterface::UpdateSink
method, which behaves like AddOrUpdateSink except that it's a nop if the sink is
not registered.

[perkj_webrtc, 2016-11-16 21:27:24]

ptal

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1621: source,
enable_cpu_overuse_detection_ &&
ops, this should still be |this|

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1910: bool encoder_sink_valid = sink
== encoder_sink_;
On 2016/11/07 09:52:46, nisse-webrtc wrote:
> Hmm, this also appears to be the only use of the encoder_sink_ member. What
> would it take to delete it? If I understand this correctly, it is intended to
> handle a
> race between application calling RemoveSink, and encoder calling
AddOrUpdateSink
> (from cpu adaptation logic). Right?
> 
> But how will the encoder ever call AddOrUpdateSink on this object? It looks
like
> you are hooking it up directly to the source, replacing SetSource(this, ...)
by
> SetSource(source_)? Then this race must be handled elsewhere. If there's no
> better way, maybe we have to introduce a VideoSourceInterface::UpdateSink
> method, which behaves like AddOrUpdateSink except that it's a nop if the sink
is
> not registered. 

Since we can not use rtc::Threads on lower levels I see no other way than to do
it here or as you way in all source implementations. But then I prefer to do it
here.

[perkj_webrtc, 2016-11-16 21:27:40]

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

[commit-bot: I haz the power, 2016-11-16 21:27:44]

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, 2016-11-16 21:34:07]

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

[commit-bot: I haz the power, 2016-11-16 21:34:08]

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

[nisse-webrtc, 2016-11-17 08:07:13]

lgtm

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2469993003/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoengine2.cc:1910: bool encoder_sink_valid = sink
== encoder_sink_;
On 2016/11/16 21:27:24, perkj_webrtc wrote:
> On 2016/11/07 09:52:46, nisse-webrtc wrote:
> > Hmm, this also appears to be the only use of the encoder_sink_ member. What
> > would it take to delete it? If I understand this correctly, it is intended
to
> > handle a
> > race between application calling RemoveSink, and encoder calling
> AddOrUpdateSink
> > (from cpu adaptation logic). Right?
> > 
> > But how will the encoder ever call AddOrUpdateSink on this object? It looks
> like
> > you are hooking it up directly to the source, replacing SetSource(this, ...)
> by
> > SetSource(source_)? Then this race must be handled elsewhere. If there's no
> > better way, maybe we have to introduce a VideoSourceInterface::UpdateSink
> > method, which behaves like AddOrUpdateSink except that it's a nop if the
sink
> is
> > not registered. 
> 
> Since we can not use rtc::Threads on lower levels I see no other way than to
do
> it here or as you way in all source implementations. But then I prefer to do
it
> here.

Not sure what other use cases there are with AddOrUpdateSink called from
multiple threads. In general, that's a sign of missing coordination (threads
will overwrite eachother's settings). But in this case, if I understand it
correctly, there's a call from the worker thread when the sink is wired up
(essentially AddSink), and calls from the encoder thread to tweak resolution
(essentially UpdateSink). If separating those methods can be done safely and
that helps us to connect camera and encoder directly, I think we should do it.
But that's for a separate cl.

[nisse-webrtc, 2016-11-17 08:10:31]

BTW, title of cl is misleading, we're removing the VideoSinkInterface
inheritance, but it's still a VideoSourceInterface.

[perkj_webrtc, 2016-11-17 21:25:12]

Description was changed from

==========
Remove WebRtcVideoSendStream2::VideoSource inheritance. Remove sending black
frame on source removal.

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

to

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

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

[perkj_webrtc, 2016-11-30 19:03:32]

On 2016/11/17 08:10:31, nisse-webrtc wrote:
> BTW, title of cl is misleading, we're removing the VideoSinkInterface
> inheritance, but it's still a VideoSourceInterface.

ping mflodman. Do we dare to land this change?

[mflodman, 2016-12-07 15:37:12]

LGTM, I don't know of any case where this CL shouldn't be ok.

Make sure to file a bug and attach before submitting, to make sure this is
recorded in release notes.

[perkj_webrtc, 2017-01-11 20:01:54]

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

[commit-bot: I haz the power, 2017-01-11 20:02:05]

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

[commit-bot: I haz the power, 2017-01-11 20:06:03]

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

[commit-bot: I haz the power, 2017-01-11 20:06:04]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16220)
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[perkj_webrtc, 2017-01-12 10:41:33]

Patchset #4 (id:60001) has been deleted

[perkj_webrtc, 2017-01-12 10:45:23]

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

[commit-bot: I haz the power, 2017-01-12 10:45:30]

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

[commit-bot: I haz the power, 2017-01-12 10:49:39]

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

[commit-bot: I haz the power, 2017-01-12 10:49:40]

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/12139)

[nisse-webrtc, 2017-01-13 07:48:09]

On 2016/12/07 15:37:12, mflodman wrote:
> LGTM, I don't know of any case where this CL shouldn't be ok.
> 
> Make sure to file a bug and attach before submitting, to make sure this is
> recorded in release notes.

To summarize offline discussion: The problem with the
P2PTestConductor.ForwardVideoOnlyStream was that the send pipeline uses 0 for
unset ntp timestamp as, while the receive pipeline use -1 for unset. And this cl
deletes an unnecessary copy, which had the side effect of always clearing the
ntp timestamp. Hence, after this cl, VideoFrames with ntp timestamp -1 are
inserted into the send pipeline.

Minimal fix for this issue now landed, see
https://codereview.webrtc.org/2620383005/

[perkj_webrtc, 2017-01-13 07:59:43]

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

[commit-bot: I haz the power, 2017-01-13 07:59:48]

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

[perkj_webrtc, 2017-01-13 08:00:16]

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

[commit-bot: I haz the power, 2017-01-13 08:00:28]

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

[commit-bot: I haz the power, 2017-01-13 08:04:37]

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

[commit-bot: I haz the power, 2017-01-13 08:04:37]

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/12200)

[perkj_webrtc, 2017-01-13 08:32:16]

On 2017/01/13 07:48:09, nisse-webrtc wrote:
> On 2016/12/07 15:37:12, mflodman wrote:
> > LGTM, I don't know of any case where this CL shouldn't be ok.
> > 
> > Make sure to file a bug and attach before submitting, to make sure this is
> > recorded in release notes.
> 
> To summarize offline discussion: The problem with the
> P2PTestConductor.ForwardVideoOnlyStream was that the send pipeline uses 0 for
> unset ntp timestamp as, while the receive pipeline use -1 for unset. And this
cl
> deletes an unnecessary copy, which had the side effect of always clearing the
> ntp timestamp. Hence, after this cl, VideoFrames with ntp timestamp -1 are
> inserted into the send pipeline.
> 
> Minimal fix for this issue now landed, see
> https://codereview.webrtc.org/2620383005/

thanks

[perkj_webrtc, 2017-01-13 08:59:05]

Description was changed from

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

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

to

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,
==========

[perkj_webrtc, 2017-01-13 09:06:38]

Description was changed from

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,
==========

to

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,webrtc::6983
==========

[perkj_webrtc, 2017-01-13 09:06:52]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2017-01-13 09:06:52]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2469993003/#ps100001 (title:
"Rebased")

[commit-bot: I haz the power, 2017-01-13 09:06:56]

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

[perkj_webrtc, 2017-01-13 09:20:29]

Description was changed from

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,webrtc::6983
==========

to

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,webrtc:6983
==========

[commit-bot: I haz the power, 2017-01-13 13:57:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484298412179070,
"parent_rev": "c5da08fc12faa862fbf0c34181332c32dae458a3", "commit_rev":
"d533aec3b8425edd81c2743f7683c12b74c307b5"}

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

Description was changed from

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,webrtc:6983
==========

to

==========
Remove WebRtcVideoSendStream2::VideoSink inheritance. Remove sending black frame
on source removal.

BUG=webrtc:6371,webrtc:6983

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

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

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/d533aec3b8425edd81c2743f7...