Add video frame refresh to MediaStream and VideoCapture stacks.

Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274
Committed: https://crrev.com/d9425d67c7e7fdd53e181f566ddc78f27dc8281c
Cr-Commit-Position: refs/heads/master@{#385582}

[miu, 2016-03-31 21:36:38]

Description was changed from

==========
O# Enter a description of the change.
Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274
==========

to

==========
Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274
==========

[miu, 2016-03-31 21:36:38]

miu@chromium.org changed reviewers:
+ emircan@chromium.org, pthatcher@chromium.org, xjz@chromium.org

[miu, 2016-03-31 21:41:01]

emircan: PTAL at content/browser/renderer_host/media/* and
content/renderer/media/*.

pthatcher: PTAL at
content/renderer/media/webrtc/media_stream_video_webrtc_sink.*.  I talked to
amp@, and he figured a once-per-second frame refresh should work well for
solving the problems (see CL description and crbug) for the WebRTC use cases.

xjz: PTAL at chrome/renderer/media/cast_rtp_stream.cc.  The other files in this
change are relevant to your upcoming project (w.r.t. plumbing a signal from
renderer-->browser process).

[emircan, 2016-04-01 00:06:47]

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:111:
base::Bind(&MediaStreamVideoWebRtcSink::RequestRefreshFrame, sink));
This logic doesn't fit to the spec descriptions of <canvas> and <video> capture.
These APIs are supposed to provide a MediaStream respecting a frame rate
constraint, including frameRate(0) where users manually request frames. This
suggests frame rate >=1 by default. Can we apply this logic to only tab capture
somehow? Maybe split the API such that RequestInitialFrame()
RequestRefreshFrame()? Canvas capture would provide only the first, and tab
capture would provide both? WDYT?

w3.org/TR/mediacapture-fromelement

[xjz, 2016-04-01 19:10:04]

lgtm. some nits:

https://codereview.chromium.org/1849003002/diff/1/chrome/renderer/media/cast_...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/1849003002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:452: // Counter for the number of
consecutive "refresh frames" received.
nit: s/received/requested.

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_video_sink.cc (right):

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_video_sink.cc:30: void
MediaStreamVideoSink::DisconnectFromTrack() {
nit: maybe early return if connected_track_.isNull()?

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_video_sink.h:49: // Requests the source a
send a frame "soon" (e.g., to resolve picture loss or
s/a send/send

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_track.cc:255: if (source_)
nit: maybe just call RequestRefreshFrame() now?

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_track.h:54: // Requests the source a
send a frame "soon" (e.g., to resolve picture loss or
s/a send/send

[miu, 2016-04-01 23:24:27]

emircan and xjz: Addressed your comments.  PTAL at patch set 2.

https://codereview.chromium.org/1849003002/diff/1/chrome/renderer/media/cast_...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/1849003002/diff/1/chrome/renderer/media/cast_...
chrome/renderer/media/cast_rtp_stream.cc:452: // Counter for the number of
consecutive "refresh frames" received.
On 2016/04/01 19:10:04, xjz wrote:
> nit: s/received/requested.

Done.  Good catch.

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_video_sink.cc (right):

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_video_sink.cc:30: void
MediaStreamVideoSink::DisconnectFromTrack() {
On 2016/04/01 19:10:04, xjz wrote:
> nit: maybe early return if connected_track_.isNull()?

The GetVideoTrack() call checks that for us.

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1849003002/diff/1/content/public/renderer/med...
content/public/renderer/media_stream_video_sink.h:49: // Requests the source a
send a frame "soon" (e.g., to resolve picture loss or
On 2016/04/01 19:10:04, xjz wrote:
> s/a send/send

Done.

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_track.cc:255: if (source_)
On 2016/04/01 19:10:04, xjz wrote:
> nit: maybe just call RequestRefreshFrame() now?

Done.

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_video_track.h:54: // Requests the source a
send a frame "soon" (e.g., to resolve picture loss or
On 2016/04/01 19:10:04, xjz wrote:
> s/a send/send

Done.

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc (right):

https://codereview.chromium.org/1849003002/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:111:
base::Bind(&MediaStreamVideoWebRtcSink::RequestRefreshFrame, sink));
On 2016/04/01 00:06:47, emircan wrote:
> This logic doesn't fit to the spec descriptions of <canvas> and <video>
capture.
> These APIs are supposed to provide a MediaStream respecting a frame rate
> constraint, including frameRate(0) where users manually request frames. This
> suggests frame rate >=1 by default. Can we apply this logic to only tab
capture
> somehow? Maybe split the API such that RequestInitialFrame()
> RequestRefreshFrame()? Canvas capture would provide only the first, and tab
> capture would provide both? WDYT?
> 
> w3.org/TR/mediacapture-fromelement

Sorry, I had not considered the other video sources.  So, yes, I changed this so
that it only applies to the screen capture cases, and I've also added some code
to respect minimum frameRate constraints, if any.

[xjz, 2016-04-05 00:04:44]

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:679:
base::Bind(&media::VideoCaptureDevice::RequestRefreshFrame,
VideoCaptureDevice::RequestRefreshFrame() currently is a virtual function with
default no-op implementation (and no other implementations anywhere else). Are
the changes on it going to be in follow up CLs?

https://codereview.chromium.org/1849003002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1849003002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:56: void
RequestRefreshFrame();
nit: move this after the implementation of MediaStreamTrack, i.e., after stop()?

[emircan, 2016-04-05 19:30:39]

lgtm % nits.

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:115: // Called by
VideoCaptureHost to request the video capture device send a
s/send/to send/

https://codereview.chromium.org/1849003002/diff/20001/content/public/renderer...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1849003002/diff/20001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:55: blink::WebMediaStreamTrack
connected_track_;
I am not sure if it is to safe to hold onto this object here. We need to make
sure that it doesn't create any problems with oilpan in garbage collection
sequence. I would rather have |MediaStreamVideoTrack*| as class member to be
safe.

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

https://codereview.chromium.org/1849003002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:26: const
int64_t kDefaultRefreshIntervalMicros = 1000000;
s/100000/base::Time::kMicrosecondsPerSecond

[miu, 2016-04-05 20:25:01]

Thanks xjz and emircan.  Addressed your comments:

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.cc (right):

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.cc:679:
base::Bind(&media::VideoCaptureDevice::RequestRefreshFrame,
On 2016/04/05 00:04:43, xjz wrote:
> VideoCaptureDevice::RequestRefreshFrame() currently is a virtual function with
> default no-op implementation (and no other implementations anywhere else). Are
> the changes on it going to be in follow up CLs?

Yep!  :)  I'll ask you to review that one shortly.

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_manager.h (right):

https://codereview.chromium.org/1849003002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_manager.h:115: // Called by
VideoCaptureHost to request the video capture device send a
On 2016/04/05 19:30:39, emircan wrote:
> s/send/to send/

It's actually proper English the way it was, but it did sound a little weird. 
I've rephrased this sentence.

https://codereview.chromium.org/1849003002/diff/20001/content/public/renderer...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1849003002/diff/20001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:55: blink::WebMediaStreamTrack
connected_track_;
On 2016/04/05 19:30:39, emircan wrote:
> I am not sure if it is to safe to hold onto this object here. We need to make
> sure that it doesn't create any problems with oilpan in garbage collection
> sequence. I would rather have |MediaStreamVideoTrack*| as class member to be
> safe.

Good point.  Done.

https://codereview.chromium.org/1849003002/diff/20001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.h (right):

https://codereview.chromium.org/1849003002/diff/20001/content/renderer/media/...
content/renderer/media/media_stream_video_track.h:56: void
RequestRefreshFrame();
On 2016/04/05 00:04:43, xjz wrote:
> nit: move this after the implementation of MediaStreamTrack, i.e., after
stop()?

Done.

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

https://codereview.chromium.org/1849003002/diff/20001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:26: const
int64_t kDefaultRefreshIntervalMicros = 1000000;
On 2016/04/05 19:30:39, emircan wrote:
> s/100000/base::Time::kMicrosecondsPerSecond

Done.

[miu, 2016-04-05 20:28:05]

miu@chromium.org changed reviewers:
+ mcasas@chromium.org, nasko@chromium.org

[miu, 2016-04-05 20:28:06]

mcasas: Need OWNERS stamp for content/*/media/* (video capture stack changes.

nasko: Need OWNERS stamp for content/common/media/video_capture_messages.h and
content/public/* changes.

[mcasas, 2016-04-05 23:12:52]

RS LGTM my parts with some micronits.

Pity all this communications infra is not migrated
to mojo yet, it'd have made this CL (and the 
necessary predecessors) much easier.

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:246: // RemoveSink().
This is true and unfortunately so because the
ownership happens to go via Blink. With the 
Onion Soup project, however, these classes will 
migrate to Blink side and they'll become garbage 
collected, so again these ownership graphs will
need revisit.

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

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:120: VLOG(1) <<
"Starting frame refresh timer with interval "
s/VLOG/DVLOG/, right?

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:200: if
(is_screencast) {
if (!is_screencast)
  return;
// ...

[miu, 2016-04-06 00:02:47]

miu@chromium.org changed reviewers:
+ nick@chromium.org
- nasko@chromium.org

[miu, 2016-04-06 00:03:18]

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_video_track.cc (right):

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_video_track.cc:246: // RemoveSink().
On 2016/04/05 23:12:52, mcasas wrote:
> This is true and unfortunately so because the
> ownership happens to go via Blink. With the 
> Onion Soup project, however, these classes will 
> migrate to Blink side and they'll become garbage 
> collected, so again these ownership graphs will
> need revisit.

FYI--Your comment here makes me wonder if this change is conflicting w/ other
current efforts: https://codereview.chromium.org/1834323002/  If that change
lands in the next 2-3 weeks, am I good?  ;-)

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

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:120: VLOG(1) <<
"Starting frame refresh timer with interval "
On 2016/04/05 23:12:52, mcasas wrote:
> s/VLOG/DVLOG/, right?

For the time being, I wanted to leave it as a VLOG().  We may need it to debug
external-to-Chromium use cases (such as Hangouts integration).

https://codereview.chromium.org/1849003002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/media_stream_video_webrtc_sink.cc:200: if
(is_screencast) {
On 2016/04/05 23:12:52, mcasas wrote:
> if (!is_screencast)
>   return;
> // ...

I can't return early here.  All the code after L226 should unconditionally
execute.

[miu, 2016-04-06 00:04:12]

ncarter: Need OWNERS stamp for content/common/media/video_capture_messages.h and
content/public/* changes.

(Note: Removed nasko@ for this, as the `git cl upload` tool had suggested him,
but it seems he has indicated his responses on code reviews will be slow.)

[ncarter (slow), 2016-04-06 17:18:26]

https://codereview.chromium.org/1849003002/diff/40001/content/common/media/vi...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1849003002/diff/40001/content/common/media/vi...
content/common/media/video_capture_messages.h:114: int /* device_id */);
You'll need an IPC security rubber stamp for this new message. I've added kenrb.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:25: // All methods calls will
be done from the main render thread.
Can you add a comment here saying that this class is designed to be extended by
content embedders?

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:45: void
DisconnectFromTrack();
Though I understand what you're trying to do here, concrete methods and data
members in a content/public class are pretty unusual. (
https://www.chromium.org/developers/content-module/content-api )

That may be why the original class was structured as it was, with static
methods, though in this case, the concrete methods vs static methods that accept
a member as the first argument are are distinction without a difference.

But I see this as basically an observer interface, and it seems natural for
observers (e.g. WebContentsObserver) to have a data member that tracks the
observed object, and ConnectToTrack functions much like.

So I think this is actually okay.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:49: bool is_connected() const
{ return !!connected_track_; }
Could we expose this as connected_track(), returning a const
WebMediaStreamTrack&. The caller can then look at isNull() to determine the
connected status?

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:53: void
RequestRefreshFrame();
This is an operation on the track/source, not on the sink, so I don't think it
belongs in this class.

Our options for this would be:
 - Add it to WebMediaStreamTrack (not sure if that makes sense or not,
blink/content layering wise, but it feels tempting).
 - Add a content/public interface and Impl class for MediaStreamVideoTrack
(feels like overkill)
 - Move this to a static method in content/public/renderer/media_stream_utils.h
(feels like cheating, but it matches the precedent for
GetCurrentVideoTrackFormat())

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:60: MediaStreamVideoTrack*
connected_track_;
Could this be a WebMediaStreamTrack? It is preferred to avoid using
content-private types in content/public, even as forward declarations, since the
#include to pick up the full declaration would actually be illegal from this
header file.

[ncarter (slow), 2016-04-06 17:18:49]

nick@chromium.org changed reviewers:
+ kenrb@chromium.org

[ncarter (slow), 2016-04-06 17:18:50]

+kenrb for IPC messages.h

[kenrb, 2016-04-06 20:32:59]

ipc lgtm

[ncarter (slow), 2016-04-06 21:06:33]

lgtm

[miu, 2016-04-06 21:07:23]

The CQ bit was checked by miu@chromium.org

[miu, 2016-04-06 21:07:23]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org,
emircan@chromium.org, kenrb@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1849003002/#ps60001
(title: "Addressed nick's PS3 comments (moving non-observer impl out of
MSVideoSink interface).")

[commit-bot: I haz the power, 2016-04-06 21:07:44]

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

[commit-bot: I haz the power, 2016-04-06 23:49:33]

Description was changed from

==========
Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274
==========

to

==========
Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274
==========

[commit-bot: I haz the power, 2016-04-06 23:49:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-06 23:51:26]

Description was changed from

==========
Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274
==========

to

==========
Add video frame refresh to MediaStream and VideoCapture stacks.

This is a follow-up on previous CLs to allow consumers of video frames
in the render process to request "refresh frames" from capturers that
live in the browser process.  This solves a number of long-standing
problems caused by capturers that halt video frame delivery (e.g., a
screen capturer stops sending frames because the screen is not being
updated).  First, it will ensure that remote clients that join a session
receive a first video frame in a timely manner.  Second, it will allow a
streaming implementation's bandwidth estimation logic to remain current
with external/environmental condiditons.  Third, it allows lossy
encoders to clean up artifacts in static content.

This change also includes some necessary prerequisite clean-up of the
public MediaStreamVideoSink interface, which contained public static
methods being passed a "self" pointer (weird!).  Instead, the methods
were turned into instance methods, and then made protected so that only
subclasses (i.e., implementations of the interface), are allowed to call
them.

BUG=486274

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

[commit-bot: I haz the power, 2016-04-06 23:51:28]

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

[miu, 2016-04-07 20:07:43]

FYI.  Responded to nick's latest round of comments before his last lgtm, but
forgot to publish my responses.  For the record:

https://codereview.chromium.org/1849003002/diff/40001/content/common/media/vi...
File content/common/media/video_capture_messages.h (right):

https://codereview.chromium.org/1849003002/diff/40001/content/common/media/vi...
content/common/media/video_capture_messages.h:114: int /* device_id */);
On 2016/04/06 17:18:26, ncarter wrote:
> You'll need an IPC security rubber stamp for this new message. I've added
kenrb.

Acknowledged.  Thanks for adding him for me.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:25: // All methods calls will
be done from the main render thread.
On 2016/04/06 17:18:26, ncarter wrote:
> Can you add a comment here saying that this class is designed to be extended
by
> content embedders?

Done.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:45: void
DisconnectFromTrack();
On 2016/04/06 17:18:26, ncarter wrote:
> Though I understand what you're trying to do here, concrete methods and data
> members in a content/public class are pretty unusual. (
> https://www.chromium.org/developers/content-module/content-api )

Ah, yes.  I reviewed the sites page, and I realized I was starting to put too
much implementation in the public class here, mostly for convenience.  I've
fixed the things mentioned in your other comments.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:49: bool is_connected() const
{ return !!connected_track_; }
On 2016/04/06 17:18:26, ncarter wrote:
> Could we expose this as connected_track(), returning a const
> WebMediaStreamTrack&. The caller can then look at isNull() to determine the
> connected status?

Done.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:53: void
RequestRefreshFrame();
On 2016/04/06 17:18:26, ncarter wrote:
> This is an operation on the track/source, not on the sink, so I don't think it
> belongs in this class.
> 
> Our options for this would be:
>  - Add it to WebMediaStreamTrack (not sure if that makes sense or not,
> blink/content layering wise, but it feels tempting).
>  - Add a content/public interface and Impl class for MediaStreamVideoTrack
> (feels like overkill)
>  - Move this to a static method in
content/public/renderer/media_stream_utils.h
> (feels like cheating, but it matches the precedent for
> GetCurrentVideoTrackFormat())

Done.  Went with the 3rd option.

https://codereview.chromium.org/1849003002/diff/40001/content/public/renderer...
content/public/renderer/media_stream_video_sink.h:60: MediaStreamVideoTrack*
connected_track_;
On 2016/04/06 17:18:26, ncarter wrote:
> Could this be a WebMediaStreamTrack? It is preferred to avoid using
> content-private types in content/public, even as forward declarations, since
the
> #include to pick up the full declaration would actually be illegal from this
> header file.

Done.