Implement missing features in WebrtcVideoStream.

Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.

BUG=547158
Committed: https://crrev.com/bf81cd6508a642e071cd6b8a8a44ad80edbacc8c
Cr-Commit-Position: refs/heads/master@{#368531}

[Sergey Ulanov, 2016-01-08 06:20:02]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-01-08 06:22:25]

Description was changed from

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.
3. WebrtcVideoCapturerAdapter now doesn't resizes video frames it
   generates.

BUG=547158
==========

to

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.
3. WebrtcVideoCapturerAdapter now doesn't resizes video frames it
   generates.
4. Fixed WebrtcVideoCapturerAdapter to avoid multiple simultaneous
   capture requests, which was causing assert failure in
   DesktopCapturer.

BUG=547158
==========

[Sergey Ulanov, 2016-01-08 06:26:26]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2016-01-08 06:26:44]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2016-01-08 18:29:04]

Can you split this into separate CLs? It doesn't seem like the features are
related, and it's much harder to review a CL if one first has to decide which
feature a line is related to before deciding whether or not it is correct.

[Sergey Ulanov, 2016-01-08 19:36:45]

Description was changed from

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.
3. WebrtcVideoCapturerAdapter now doesn't resizes video frames it
   generates.
4. Fixed WebrtcVideoCapturerAdapter to avoid multiple simultaneous
   capture requests, which was causing assert failure in
   DesktopCapturer.

BUG=547158
==========

to

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.

BUG=547158
==========

[Sergey Ulanov, 2016-01-08 19:40:27]

On 2016/01/08 18:29:04, Jamie wrote:
> Can you split this into separate CLs? It doesn't seem like the features are
> related, and it's much harder to review a CL if one first has to decide which
> feature a line is related to before deciding whether or not it is correct.

Moved WebrtcVideoCapturerAdapter changes to
https://codereview.chromium.org/1574543002/

[Jamie, 2016-01-08 22:59:14]

https://codereview.chromium.org/1571543002/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/1571543002/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:91:
video_track_->GetSource()->GetVideoCapturer());
Is this allowed? I've never seen reinterpret_cast used to cast from a base class
to a derived one. Even if it's safe, you're relying on an implementation detail
of webrtc::PeerConnectionFactoryInterface, which doesn't seem very nice.

[Sergey Ulanov, 2016-01-08 23:39:56]

https://codereview.chromium.org/1571543002/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/1571543002/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:91:
video_track_->GetSource()->GetVideoCapturer());
On 2016/01/08 22:59:14, Jamie wrote:
> Is this allowed? I've never seen reinterpret_cast used to cast from a base
class
> to a derived one. Even if it's safe, you're relying on an implementation
detail
> of webrtc::PeerConnectionFactoryInterface, which doesn't seem very nice.

The VideoCapturer passed to the VideoTrack is created in
WebrtcVideoStream::Start(), so it's always WebrtcVideoCapturerAdapter and the
upcast is always safe. This is not implementation detail of
VideoCapturerAdapter.

Anyway, I agree it's ugly. Replaced this with a WeakPtr<> for
WebrtcVideoCapturerAdapter.

[Jamie, 2016-01-08 23:44:45]

lgtm

https://codereview.chromium.org/1571543002/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_video_stream.cc (right):

https://codereview.chromium.org/1571543002/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_video_stream.cc:91:
video_track_->GetSource()->GetVideoCapturer());
On 2016/01/08 23:39:56, Sergey Ulanov wrote:
> On 2016/01/08 22:59:14, Jamie wrote:
> > Is this allowed? I've never seen reinterpret_cast used to cast from a base
> class
> > to a derived one. Even if it's safe, you're relying on an implementation
> detail
> > of webrtc::PeerConnectionFactoryInterface, which doesn't seem very nice.
> 
> The VideoCapturer passed to the VideoTrack is created in
> WebrtcVideoStream::Start(), so it's always WebrtcVideoCapturerAdapter and the
> upcast is always safe. This is not implementation detail of
> VideoCapturerAdapter.
> 
> Anyway, I agree it's ugly. Replaced this with a WeakPtr<> for
> WebrtcVideoCapturerAdapter.

It's conceivable that video_track_ might wrap the capturer though, so while I
don't think it's very likely, it's still a potential source of tricky future
bugs.

WeakPtr is much nicer though, thanks for changing it.

[Sergey Ulanov, 2016-01-10 07:29:39]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-01-10 07:29:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1571543002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1571543002/100001

[commit-bot: I haz the power, 2016-01-10 08:00:49]

Description was changed from

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.

BUG=547158
==========

to

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.

BUG=547158
==========

[commit-bot: I haz the power, 2016-01-10 08:00:52]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-01-10 08:02:15]

Description was changed from

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.

BUG=547158
==========

to

==========
Implement missing features in WebrtcVideoStream.

1. WebrtcVideoStream::Pause() now pauses the stream.
2. WebrtcVideoStream calls SizeCallback to notify about size changes.

BUG=547158

Committed: https://crrev.com/bf81cd6508a642e071cd6b8a8a44ad80edbacc8c
Cr-Commit-Position: refs/heads/master@{#368531}
==========

[commit-bot: I haz the power, 2016-01-10 08:02:15]

Patchset 4 (id:??) landed as
https://crrev.com/bf81cd6508a642e071cd6b8a8a44ad80edbacc8c
Cr-Commit-Position: refs/heads/master@{#368531}