New class AdaptedVideoTrackSource.

New class AdaptedVideoTrackSource.

BUG=webrtc:6353
Committed: https://crrev.com/6f5a6c318890bf544fc12fedd56db4bc29af69ef
Cr-Commit-Position: refs/heads/master@{#14345}

[nisse-webrtc, 2016-09-14 07:22:48]

This is a different approach to https://codereview.webrtc.org/2318953005/,
exctracting code from AndroidVideoTrackSource into a more general base class for
capturer sources using video adaptation.

[nisse-webrtc, 2016-09-14 08:17:41]

Description was changed from

==========
New class AdaptedVideoTrackSource.

On top of cl https://codereview.webrtc.org/2325563002/

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

to

==========
New class AdaptedVideoTrackSource.

BUG=webrtc:6353
==========

[nisse-webrtc, 2016-09-14 08:40:00]

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

[nisse-webrtc, 2016-09-14 08:40:00]

PTAL.

[magjed_webrtc, 2016-09-14 13:30:56]

https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
webrtc/api/androidvideotracksource.cc:76: if (apply_rotation_.value && (rotation
== 90 || rotation == 270)) {
The |apply_rotation_| variable is not defined?

[nisse-webrtc, 2016-09-14 14:10:33]

https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
webrtc/api/androidvideotracksource.cc:76: if (apply_rotation_.value && (rotation
== 90 || rotation == 270)) {
On 2016/09/14 13:30:56, magjed_webrtc wrote:
> The |apply_rotation_| variable is not defined?

It's a protected variable in the base class. Maybe that's prohibited by
style-guide? That seemed to be the easiest way to get properly locked access to
it, but I can add an accessor function for it, if you want.

[nisse-webrtc, 2016-09-15 07:39:17]

https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
webrtc/api/androidvideotracksource.cc:76: if (apply_rotation_.value && (rotation
== 90 || rotation == 270)) {
On 2016/09/14 14:10:33, nisse-webrtc wrote:
> On 2016/09/14 13:30:56, magjed_webrtc wrote:
> > The |apply_rotation_| variable is not defined?
> 
> It's a protected variable in the base class. Maybe that's prohibited by
> style-guide? That seemed to be the easiest way to get properly locked access
to
> it, but I can add an accessor function for it, if you want.

Added now. 

I wonder if there's some other way to handle the race, without giving
AndroidVideoTrackSource (and other capturers migrating to using this class)
access to the lock? Say that AndroidVideoTrackSource gets the value of
apply_rotation, but doesn't hold on to the lock, and apply_rotation changes (due
to a call to AddOrUpdateSink) before the frame is passed on to OnFrame.

Then OnFrame can get the lock again, and hold on to it until returning. It can
check for the case apply_rotation == true, frame.rotation != 0, and in that
case, simply discard the frame.

That should handle the case of a sink added with wants_rotation true; it
guarantees that that sink can only see pre-rotated frames.

It doesn't guarantee anything for the transition apply_rotation true -> false.
Scenario: A sink changes its wants.rotation_applied from true to false; then due
to the race with OnFrame, it might get a prerotated frame or two after
AddOrUpdateSink has returned.

But I don't think that matters very much, because

1. A sink with wants.apply_rotation = false must still be prepared to handle
prerotated frames.

2. A sink changing its wants.apply_rotation mid-stream seems like a very obscure
usecase.

[magjed_webrtc, 2016-09-15 15:02:52]

On 2016/09/15 07:39:17, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
> File webrtc/api/androidvideotracksource.cc (right):
> 
>
https://codereview.webrtc.org/2328333002/diff/20001/webrtc/api/androidvideotr...
> webrtc/api/androidvideotracksource.cc:76: if (apply_rotation_.value &&
(rotation
> == 90 || rotation == 270)) {
> On 2016/09/14 14:10:33, nisse-webrtc wrote:
> > On 2016/09/14 13:30:56, magjed_webrtc wrote:
> > > The |apply_rotation_| variable is not defined?
> > 
> > It's a protected variable in the base class. Maybe that's prohibited by
> > style-guide? That seemed to be the easiest way to get properly locked access
> to
> > it, but I can add an accessor function for it, if you want.
> 
> Added now. 
> 
> I wonder if there's some other way to handle the race, without giving
> AndroidVideoTrackSource (and other capturers migrating to using this class)
> access to the lock? Say that AndroidVideoTrackSource gets the value of
> apply_rotation, but doesn't hold on to the lock, and apply_rotation changes
(due
> to a call to AddOrUpdateSink) before the frame is passed on to OnFrame.
> 
> Then OnFrame can get the lock again, and hold on to it until returning. It can
> check for the case apply_rotation == true, frame.rotation != 0, and in that
> case, simply discard the frame.
> 
> That should handle the case of a sink added with wants_rotation true; it
> guarantees that that sink can only see pre-rotated frames.
> 
> It doesn't guarantee anything for the transition apply_rotation true -> false.
> Scenario: A sink changes its wants.rotation_applied from true to false; then
due
> to the race with OnFrame, it might get a prerotated frame or two after
> AddOrUpdateSink has returned.
> 
> But I don't think that matters very much, because
> 
> 1. A sink with wants.apply_rotation = false must still be prepared to handle
> prerotated frames.
> 
> 2. A sink changing its wants.apply_rotation mid-stream seems like a very
obscure
> usecase.

This sounds like a better approach. The unlocked 'bool apply_rotation()'
function is a bit ugly, but I can live with it.

[nisse-webrtc, 2016-09-16 11:54:10]

Dropped access to the apply_rotation lock. OnFrame in the base class does a
properly synchronized check if applied_rotation_ is true and frame needs
rotation. If so, plain memory frames are rotated, and native frames are dropped.

[magjed_webrtc, 2016-09-19 09:43:26]

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/api/androidvideotr...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/api/androidvideotr...
webrtc/api/androidvideotracksource.cc:78: // TODO(nisse, magjed): Don't rotate
here, leave it to the base
You need to rebase this function (I changed it recently). You should be able to
just remove some rotation code here after you rebase.

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:34: rtc::CritScope
lock(&stats_crit_);
Can we move the stats_ thing to AdaptFrame instead and remove the width/height
arguments from this function?

[nisse-webrtc, 2016-09-20 08:23:42]

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/api/androidvideotr...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/api/androidvideotr...
webrtc/api/androidvideotracksource.cc:78: // TODO(nisse, magjed): Don't rotate
here, leave it to the base
On 2016/09/19 09:43:26, magjed_webrtc wrote:
> You need to rebase this function (I changed it recently). You should be able
to
> just remove some rotation code here after you rebase.

Done, hope I got the rebasing right.

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/60001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:34: rtc::CritScope
lock(&stats_crit_);
On 2016/09/19 09:43:26, magjed_webrtc wrote:
> Can we move the stats_ thing to AdaptFrame instead and remove the width/height
> arguments from this function?

Done.

[magjed_webrtc, 2016-09-20 09:16:02]

lgtm!

[nisse-webrtc, 2016-09-20 09:31:02]

The CQ bit was checked by nisse@webrtc.org

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

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-09-20 11:31:43]

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

[commit-bot: I haz the power, 2016-09-20 11:31:44]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-09-20 12:25:36]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-09-20 12:25:44]

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

[perkj_webrtc, 2016-09-20 12:26:10]

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/api/androidvideot...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/api/androidvideot...
webrtc/api/androidvideotracksource.cc:98: OnFrame(cricket::WebRtcVideoFrame(
How about we let OnFrame take VideoFrameBuffer as argument instead of a
VideoFrame. Then the base class can own the timestamp_aligner_ and subclasses
does not have to worry about it.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.cc:31: rtc::CritScope
lock(&apply_rotation_crit_);
I would avoid holding a lock when you call the broadcaster.  Everything here is
thread safe if you use apply_rotation() instead.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.cc:75: const rtc::VideoSinkWants&
wants) {
add thread checker. Should be called on same thread as AddOrUpdateSink.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.h (right):

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:21: class AdaptedVideoTrackSource
Can you add a not about how this class should be used?
And please comment about that AdaptFrame and OnFrame is thread safe and can be
called on the camera thread.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:29: bool GetStats(Stats* stats)
override;
All these methods should be made private. An implementation or a subclass should
not call these methods.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:43: void OnSinkWantsChanged(const
rtc::VideoSinkWants& wants);
private

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:60: // using this method, the value
may become stale before it is used.
mm-ok. the problem is not really appy_rotation()- the problem is that it should
be used on the same thread as where OnSinkWantsChanged is called. if not- is has
to be synchronized just the way you are.

Alternatively, you can have a virtual void OnApplyRotationChanged(bool
apply_rotation) = 0; method that the base class call when the flag is changed,
not sure if that is any better though.

[nisse-webrtc, 2016-09-20 12:26:29]

The CQ bit was unchecked by nisse@webrtc.org

[nisse-webrtc, 2016-09-20 13:17:48]

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/api/androidvideot...
File webrtc/api/androidvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/api/androidvideot...
webrtc/api/androidvideotracksource.cc:98: OnFrame(cricket::WebRtcVideoFrame(
On 2016/09/20 12:26:09, perkj_webrtc wrote:
> How about we let OnFrame take VideoFrameBuffer as argument instead of a
> VideoFrame. 

And rotation and timestamp arguments too? I think it's nicer to stick to the
VideoSinkInterface::OnFrame signature (even if we're not formally implementing
that interface).

> Then the base class can own the timestamp_aligner_ and subclasses
> does not have to worry about it.

For now, I prefer to let the TimestampAligner be an independent thing, and each
source can use the helpers it really needs.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.cc:31: rtc::CritScope
lock(&apply_rotation_crit_);
On 2016/09/20 12:26:09, perkj_webrtc wrote:
> I would avoid holding a lock when you call the broadcaster.  Everything here
is
> thread safe if you use apply_rotation() instead.

The lock is needed to ensure that a sink that sets wants.rotation_applied = true
never receives a frame with pending rotation.

We could get rid of this lock if we relax the requirements a bit and say that a
sink can get frames with any rotation. And for a sink which sets
wants.rotation_applied == true, it is expected to check for pending rotation and
discard such frames.

Hmm, and the only place I can find within webrtc where rotation_applied can be
set to true is WebRtcVideoChannel2::WebRtcVideoSendStream, and it should be easy
to add that logic to WebRtcVideoChannel2::WebRtcVideoSendStream::OnFrame. Does
that make sense? Not sure what other applications might be affected.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.cc:75: const rtc::VideoSinkWants&
wants) {
On 2016/09/20 12:26:09, perkj_webrtc wrote:
> add thread checker. Should be called on same thread as AddOrUpdateSink.

Done. Even if it seems a bit overkill to me: method is now private, there are
only two call sites, and both already use a thread checker.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.h (right):

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:21: class AdaptedVideoTrackSource
On 2016/09/20 12:26:10, perkj_webrtc wrote:
> Can you add a not about how this class should be used?
> And please comment about that AdaptFrame and OnFrame is thread safe and can be
> called on the camera thread.

Done.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:29: bool GetStats(Stats* stats)
override;
On 2016/09/20 12:26:10, perkj_webrtc wrote:
> All these methods should be made private. An implementation or a subclass
should
> not call these methods.

Done. I'll never get used to this convention.

https://codereview.webrtc.org/2328333002/diff/100001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:43: void OnSinkWantsChanged(const
rtc::VideoSinkWants& wants);
On 2016/09/20 12:26:10, perkj_webrtc wrote:
> private

Done.

[nisse-webrtc, 2016-09-21 07:40:08]

Please have another look. I've reworked the synchronization between OnFrame and
changes to apply_rotation. Now, the VideoBroadcaster (which already has a big
lock) makes sure that no sink gets frames with unexpected pending rotation. And
the other two classes don't care about synchronization between their OnFrame
calls and changes to the apply_rotation state.

The apply_rotation() method no longer uses its own lock, but calls into the
broadcaster which uses its sinks_and_wants_lock_ to protect the access. I think
this should be safe, since the same lock is taken by the same thread later as
part of the OnFrame processing, so lock order is unchanged.

[perkj_webrtc, 2016-09-21 08:59:23]

lgtm with the below addressed.

https://codereview.webrtc.org/2328333002/diff/120001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.h (right):

https://codereview.webrtc.org/2328333002/diff/120001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:33: bool GetStats(Stats* stats)
override;
Also private?

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.h (right):

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:33: bool GetStats(Stats* stats)
override;
make this private too please.

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/videos...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/videos...
webrtc/media/base/videosourceinterface.h:24: // default, any rotation must be
applied by the sink. The VideoBroadcaster
I don't think think the comment about VideoBroadCaster belongs in the interface.
That is an implementation detail.

[nisse-webrtc, 2016-09-21 09:43:59]

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/adapte...
File webrtc/media/base/adaptedvideotracksource.h (right):

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/adapte...
webrtc/media/base/adaptedvideotracksource.h:33: bool GetStats(Stats* stats)
override;
On 2016/09/21 08:59:23, perkj_webrtc wrote:
> make this private too please.

Done.

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/videos...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/2328333002/diff/160001/webrtc/media/base/videos...
webrtc/media/base/videosourceinterface.h:24: // default, any rotation must be
applied by the sink. The VideoBroadcaster
On 2016/09/21 08:59:23, perkj_webrtc wrote:
> I don't think think the comment about VideoBroadCaster belongs in the
interface.
> That is an implementation detail.

The intention is to explain what the rotation_applied flag means. Which I think
belongs here.

Now, the meaning currently is a bit muddy. I think the alternative is to clarify
that even if it is set, the sink might get frames with pending rotation. (And
then it becomes an implementation detail of VideoBroadcaster that it's more
strict, to help current sinks which need that).

What do you suggest, re-phrasing, moving to where?

[nisse-webrtc, 2016-09-22 07:43:57]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-09-22 07:43:58]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2328333002/#ps200001 (title:
"Comment improvement.")

[commit-bot: I haz the power, 2016-09-22 07:44:03]

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-09-22 08:26:01]

Description was changed from

==========
New class AdaptedVideoTrackSource.

BUG=webrtc:6353
==========

to

==========
New class AdaptedVideoTrackSource.

BUG=webrtc:6353
==========

[commit-bot: I haz the power, 2016-09-22 08:26:02]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-22 08:26:09]

Description was changed from

==========
New class AdaptedVideoTrackSource.

BUG=webrtc:6353
==========

to

==========
New class AdaptedVideoTrackSource.

BUG=webrtc:6353

Committed: https://crrev.com/6f5a6c318890bf544fc12fedd56db4bc29af69ef
Cr-Commit-Position: refs/heads/master@{#14345}
==========

[commit-bot: I haz the power, 2016-09-22 08:26:10]

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