Cast: Basic cast_receiver API for chrome.

Cast: Basic cast_receiver API for chrome.
Javascript bindings in separate CL.

Committed: https://crrev.com/a5ed06ce43f57f7d4243dae407e16937a2a4e00f
Cr-Commit-Position: refs/heads/master@{#317925}

[hubbe, 2015-02-03 01:06:44]

hubbe@chromium.org changed reviewers:
+ hclam@google.com

[hubbe, 2015-02-03 01:06:44]

Preliminary review, I don't intend to check this in until I have javascript
bindings that work. (more or less)

[Alpha Left Google, 2015-02-03 23:25:41]

hclam@chromium.org changed reviewers:
+ hclam@chromium.org
- hclam@google.com

[Alpha Left Google, 2015-02-04 03:07:53]

First round of comments. Please also add perkj@ as he might have comments about
the interfacing with MediaStreamVideoSource.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_receiver_session.cc (right):

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:16: class
CastStreamingAudioCaptureSource : public media::AudioCapturerSource {
Should be CastSreamingAudioCapturerSource.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:33: class CastVideoCapturerSource
: public media::VideoCapturerSource {
Should be CastStreamingvideoCapturerSource to be consistent.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:167: base::WaitableEvent
event(true, false);
There is no need to stop and wait. |frame_callback| can be called safely (on IO
thread) even after this method is called.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:183: if (params.sample_rate() !=
48000 ||
Need a comment or TODO to fix this limitation.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:199: base::WaitableEvent
event(true, false);
Instead of blocking here what about having a third party ref counted thread safe
proxy object that allows you to submit audio data and detach under a lock?

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:29: class CastReceiverSession :
public base::RefCounted<CastReceiverSession> {
I think this is a render thread object. Please document.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:36: // incoming streams
immediately, buffering input until
This will be more clear if it reads incoming packets or incoming RTP streams. I
was confusing it with MediaStream.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:53: ~CastReceiverSession();
virtual.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:57: void
StopAudio(base::WaitableEvent* event);
Blocking is bad, particularly on render thread.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:62: void
StopVideo(base::WaitableEvent* event);
Same here. Blocking is bad.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_receiver_session_delegate.cc (right):

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session_delegate.cc:83:
content::VideoCaptureDeliverFrameCB video_callback) {
This callback passed from CastVideoCapturerSource must be called from the IO
thread, which is unknown to the interface of VideoCapturerSource. I think this
method should take a MessageLoopProxy and then perform a DCHECK in
OnDecodedVideoFrame to verify the callback is called on the correct thread.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session_delegate.cc:88: 
nit: empty line.

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_source_api.h (right):

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_source_api.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
Maybe this file should be media_stream_source.h? To be consistent with
media_stream_sink.h.

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_source_api.h:18: CONTENT_EXPORT void
InitializeVideoWebMediaSource(
Maybe InitializeMediaStreamVideoSource and InitializeMediaStreamAudioSource?

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
File media/video/capture/video_capturer_source.h (right):

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:21: class MEDIA_EXPORT
VideoCapturerSource
I think this is best put in media/base. media/video/capture is mostly for
implemention with hardware.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:22: : public
base::RefCountedThreadSafe<media::VideoCapturerSource> {
There's no need for this to be ref counted thread safe.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:40: typedef base::Callback<
Please remove the definition of VideoCaptureDeliverFrameCB from
media_stream_video_sink.h. There should just be one definition.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:68: virtual void StartCapture(
I see this interface is extracted from MediaStreamVideoCapturerSource. The
details of threading must be considered carefully.
MediaStreamVideoCapturerSource was designed to operate on the render thread
while VideoCaptureDeliverFrameCB was to be called on IO thread only. The details
of threading is available for code in content but not in media/. I think this
method should take a message loop for calling |new_frame_callback|.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:74: // SupportedFormatsCallback
callback. Waits until it is safe to
There shouldn't be any wait.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:76: virtual void StopCapture() = 0;
Note that it is okay that |new_frame_callback| is called after StopCapture() is
called. There is no need to wait. This method is asynchronous (unlike
AudioCapturerSource) and this information should be in the comments.

[hubbe, 2015-02-05 20:23:01]

PTAL

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_receiver_session.cc (right):

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:16: class
CastStreamingAudioCaptureSource : public media::AudioCapturerSource {
On 2015/02/04 03:07:52, Alpha wrote:
> Should be CastSreamingAudioCapturerSource.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:33: class CastVideoCapturerSource
: public media::VideoCapturerSource {
On 2015/02/04 03:07:52, Alpha wrote:
> Should be CastStreamingvideoCapturerSource to be consistent.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:167: base::WaitableEvent
event(true, false);
On 2015/02/04 03:07:52, Alpha wrote:
> There is no need to stop and wait. |frame_callback| can be called safely (on
IO
> thread) even after this method is called.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:183: if (params.sample_rate() !=
48000 ||
On 2015/02/04 03:07:52, Alpha wrote:
> Need a comment or TODO to fix this limitation.

Improved to not hardcode values and added TODO to consider conversion.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.cc:199: base::WaitableEvent
event(true, false);
On 2015/02/04 03:07:52, Alpha wrote:
> Instead of blocking here what about having a third party ref counted thread
safe
> proxy object that allows you to submit audio data and detach under a lock?

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:29: class CastReceiverSession :
public base::RefCounted<CastReceiverSession> {
On 2015/02/04 03:07:52, Alpha wrote:
> I think this is a render thread object. Please document.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:36: // incoming streams
immediately, buffering input until
On 2015/02/04 03:07:52, Alpha wrote:
> This will be more clear if it reads incoming packets or incoming RTP streams.
I
> was confusing it with MediaStream.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:53: ~CastReceiverSession();
On 2015/02/04 03:07:52, Alpha wrote:
> virtual.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:57: void
StopAudio(base::WaitableEvent* event);
On 2015/02/04 03:07:52, Alpha wrote:
> Blocking is bad, particularly on render thread.

It doesn't block. The caller may block waiting for the event though.
It's written this way because AudioCapturerSource::Stop seems to require it.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session.h:62: void
StopVideo(base::WaitableEvent* event);
On 2015/02/04 03:07:52, Alpha wrote:
> Same here. Blocking is bad.

Hmm, I might be able to change this.
I just did it this way so that it would be the same as audio.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_receiver_session_delegate.cc (right):

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session_delegate.cc:83:
content::VideoCaptureDeliverFrameCB video_callback) {
On 2015/02/04 03:07:52, Alpha wrote:
> This callback passed from CastVideoCapturerSource must be called from the IO
> thread, which is unknown to the interface of VideoCapturerSource. I think this
> method should take a MessageLoopProxy and then perform a DCHECK in
> OnDecodedVideoFrame to verify the callback is called on the correct thread.

Done.

https://codereview.chromium.org/883293005/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_receiver_session_delegate.cc:88: 
On 2015/02/04 03:07:52, Alpha wrote:
> nit: empty line.

Done.

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_source_api.h (right):

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_source_api.h:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/02/04 03:07:52, Alpha wrote:
> Maybe this file should be media_stream_source.h? To be consistent with
> media_stream_sink.h.

Seems like a file called media_stream_source.h should contain the
MediaStreamSource class.

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_source_api.h:18: CONTENT_EXPORT void
InitializeVideoWebMediaSource(
On 2015/02/04 03:07:52, Alpha wrote:
> Maybe InitializeMediaStreamVideoSource and InitializeMediaStreamAudioSource?

How about Initialize{Audio,Video}WebMediaStreamSource ?

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
File media/video/capture/video_capturer_source.h (right):

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:21: class MEDIA_EXPORT
VideoCapturerSource
On 2015/02/04 03:07:52, Alpha wrote:
> I think this is best put in media/base. media/video/capture is mostly for
> implemention with hardware.

That leaves a rather awkward dependency on video_capture_types.h in media/base

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:22: : public
base::RefCountedThreadSafe<media::VideoCapturerSource> {
On 2015/02/04 03:07:53, Alpha wrote:
> There's no need for this to be ref counted thread safe. 

Done.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:40: typedef base::Callback<
On 2015/02/04 03:07:52, Alpha wrote:
> Please remove the definition of VideoCaptureDeliverFrameCB from
> media_stream_video_sink.h. There should just be one definition.

Done.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:68: virtual void StartCapture(
On 2015/02/04 03:07:52, Alpha wrote:
> I see this interface is extracted from MediaStreamVideoCapturerSource. The
> details of threading must be considered carefully.
> MediaStreamVideoCapturerSource was designed to operate on the render thread
> while VideoCaptureDeliverFrameCB was to be called on IO thread only. The
details
> of threading is available for code in content but not in media/. I think this
> method should take a message loop for calling |new_frame_callback|.

Added task runner, added documentation.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:74: // SupportedFormatsCallback
callback. Waits until it is safe to
On 2015/02/04 03:07:52, Alpha wrote:
> There shouldn't be any wait.

Changed to not require wait.

https://codereview.chromium.org/883293005/diff/1/media/video/capture/video_ca...
media/video/capture/video_capturer_source.h:76: virtual void StopCapture() = 0;
On 2015/02/04 03:07:52, Alpha wrote:
> Note that it is okay that |new_frame_callback| is called after StopCapture()
is
> called. There is no need to wait. This method is asynchronous (unlike
> AudioCapturerSource) and this information should be in the comments.

Changed to state that there might be additional calls after stop.

[Alpha Left Google, 2015-02-07 01:30:30]

LGTM with two nits. Please add perkj@ to review the MediaStream* changes.

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_source_api.h (right):

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_source_api.h:18: CONTENT_EXPORT void
InitializeVideoWebMediaSource(
On 2015/02/05 20:23:00, hubbe wrote:
> On 2015/02/04 03:07:52, Alpha wrote:
> > Maybe InitializeMediaStreamVideoSource and InitializeMediaStreamAudioSource?
> 
> How about Initialize{Audio,Video}WebMediaStreamSource ?

InitializeVideoWebMediaStreamSource is a bunch of nouns.
InitializeVideoForWebMediaStreamSource seems to be more readable.

https://codereview.chromium.org/883293005/diff/100001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/100001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:62: // stop immediately.
nit: This can go to the previous line.

[perkj_chrome, 2015-02-09 14:50:25]

perkj@chromium.org changed reviewers:
+ perkj@chromium.org

[perkj_chrome, 2015-02-09 14:50:27]

Looks good. A nit but the name VideoCapturer disturbs me a bit in this context.
To me, a capturer is something local, ie a camera, tab capture, microphone,
screen capture etc. But this will use it in a wider meaning.

https://codereview.chromium.org/883293005/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/883293005/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_video_capturer_source.cc:259:
RenderThreadImpl::current()->GetIOMessageLoopProxy(),
indentation

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
File content/public/renderer/media_stream_source_api.cc (right):

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:24: true /* remote */,
Remote means that this is something you receive.
read_only means that the source  can not be changed by constraints.

http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...

(these are not exposed to JS (yet))

If this API is only intended for those use cases it should be documented in the
header file.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:27: new
content::MediaStreamVideoCapturerSource(
We had a few discussion regarding inheritance and delegation when we started
refactoring MediaStreamSources.
MediaStreamVideoSource is the base class and the class I would have expected you
to inherit and use for video. There are a few different other video specific
ones already such as the one from video tracks received from a PeerConnection.
But I like delegation and I like this.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:28: StreamDeviceInfo(),
Humm, StreamDeviceInfo seems wrong here. Its only used by video cameras and tab
capture / screen capture. I wonder if it can be removed all together from
MediaStreamSource now. (I don't expect you to do it)

Note though that if you had choosen to implement MediaStreamVideoSource instead,
you would not have needed to use specify it even though it still exist in
MediaStreamSource. 

Can you move them from the ctor to another public method so you don't have to
provide them?

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:49: -1,
indentation

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
File content/public/renderer/media_stream_source_api.h (right):

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.h:18: CONTENT_EXPORT void
InitializeVideoWebMediaStreamSource(
please add documentation

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.h:25: // TODO(Hubbe): remove
methods?
?

https://codereview.chromium.org/883293005/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/883293005/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:114: 
remove

https://codereview.chromium.org/883293005/diff/100001/media/base/video_captur...
File media/base/video_capturer_source.h (right):

https://codereview.chromium.org/883293005/diff/100001/media/base/video_captur...
media/base/video_capturer_source.h:9: #include <vector>
nit: new line after system includes.

https://codereview.chromium.org/883293005/diff/100001/media/base/video_captur...
media/base/video_capturer_source.h:23: : public
base::RefCounted<media::VideoCapturerSource> {
does it have to be ref counted?

[hubbe, 2015-02-09 20:07:53]

New patchsets have been uploaded after l-g-t-m from hclam@chromium.org

[hubbe, 2015-02-09 20:13:33]

I realize that the terminology is getting pretty convoluted.
For now I think "capturer" is probably still want we want, because we want it to
be as similar as possible to the audio pipeline. Also, since it serves as an
input to a "capturer" class, it's still correct, even if it's not a local
source.

Another way to see it would simply be to expand our thinking so that "capturing"
simply means "get data from", then you can "capture" data from a socket for
instance....

Perhaps the best thing to do is to keep the naming the way it is and add some
TODOs for re-visiting the terminology.

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_source_api.h (right):

https://codereview.chromium.org/883293005/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_source_api.h:18: CONTENT_EXPORT void
InitializeVideoWebMediaSource(
On 2015/02/07 01:30:30, Alpha wrote:
> On 2015/02/05 20:23:00, hubbe wrote:
> > On 2015/02/04 03:07:52, Alpha wrote:
> > > Maybe InitializeMediaStreamVideoSource and
InitializeMediaStreamAudioSource?
> > 
> > How about Initialize{Audio,Video}WebMediaStreamSource ?
> 
> InitializeVideoWebMediaStreamSource is a bunch of nouns.
> InitializeVideoForWebMediaStreamSource seems to be more readable.

How about InitializeWebMediaStreamSourceFor{Audio,Video} ?

https://codereview.chromium.org/883293005/diff/100001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/100001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:62: // stop immediately.
On 2015/02/07 01:30:30, Alpha wrote:
> nit: This can go to the previous line.

Done.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
File content/public/renderer/media_stream_source_api.cc (right):

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:24: true /* remote */,
On 2015/02/09 14:50:27, perkj wrote:
> Remote means that this is something you receive.
> read_only means that the source  can not be changed by constraints.
> 
>
http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaStreamTrac...
> 
> (these are not exposed to JS (yet))
> 
> If this API is only intended for those use cases it should be documented in
the
> header file.


I've added and documented these arguments to the API methods.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:27: new
content::MediaStreamVideoCapturerSource(
On 2015/02/09 14:50:27, perkj wrote:
> We had a few discussion regarding inheritance and delegation when we started
> refactoring MediaStreamSources.
> MediaStreamVideoSource is the base class and the class I would have expected
you
> to inherit and use for video. There are a few different other video specific
> ones already such as the one from video tracks received from a PeerConnection.
> But I like delegation and I like this. 

I started out with trying to do this, but MediaStreamVideoSource is not an
interface and putting it in content/public would either require some major
refactoring, or moving a lot of stuff into content/public.

When I found the AudioCapturerSource class, I realized I could replicate this
pattern for video, which would be a much cleaner way to do things
implementation-wise. (However, it does tend to mess up some terminology and
assumptions about what is local and what isn't...)

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:28: StreamDeviceInfo(),
On 2015/02/09 14:50:27, perkj wrote:
> Humm, StreamDeviceInfo seems wrong here. Its only used by video cameras and
tab
> capture / screen capture. I wonder if it can be removed all together from
> MediaStreamSource now. (I don't expect you to do it)
> 
> Note though that if you had choosen to implement MediaStreamVideoSource
instead,
> you would not have needed to use specify it even though it still exist in
> MediaStreamSource. 
> 
> Can you move them from the ctor to another public method so you don't have to
> provide them?

Fixed the constructor to not require the device info and update all call sites.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:49: -1,
On 2015/02/09 14:50:27, perkj wrote:
> indentation

Done.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
File content/public/renderer/media_stream_source_api.h (right):

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.h:18: CONTENT_EXPORT void
InitializeVideoWebMediaStreamSource(
On 2015/02/09 14:50:27, perkj wrote:
> please add documentation

Comment added.

https://codereview.chromium.org/883293005/diff/100001/content/public/renderer...
content/public/renderer/media_stream_source_api.h:25: // TODO(Hubbe): remove
methods?
On 2015/02/09 14:50:27, perkj wrote:
> ? 

This comment made more sense when the above methods were called
Add{Audio,Video}SourceToMediaStream

Comment removed.

https://codereview.chromium.org/883293005/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/883293005/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:114: 
On 2015/02/09 14:50:27, perkj wrote:
> remove

Done.

https://codereview.chromium.org/883293005/diff/100001/media/base/video_captur...
File media/base/video_capturer_source.h (right):

https://codereview.chromium.org/883293005/diff/100001/media/base/video_captur...
media/base/video_capturer_source.h:9: #include <vector>
On 2015/02/09 14:50:27, perkj wrote:
> nit: new line after system includes.

Done.

https://codereview.chromium.org/883293005/diff/100001/media/base/video_captur...
media/base/video_capturer_source.h:23: : public
base::RefCounted<media::VideoCapturerSource> {
On 2015/02/09 14:50:27, perkj wrote:
> does it have to be ref counted?

No, it doesn't, fixed.
(I originally did it for parity with AudioCapturerSource, but I'm not sure it
makes sense there either.)

[perkj_chrome, 2015-02-10 15:29:51]

lgtm

I have not reviewed cast specific files.
Note: You will need owners review in media/base and content/renderer/public/

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
File content/public/renderer/media_stream_source_api.cc (right):

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:44: MediaStreamAudioSource*
audio_source(
This seems to require dark art in order to get right. It should be up to the
webrtc team to clean up the audio source code so only arguments that make sense
are needed and the dependencies understandable. Unfortunately, that will not
happen for quite some time.

[miu, 2015-02-11 02:52:47]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2015-02-11 02:52:50]

lgtm

Lots of little things (mostly nits and sanity-check questions):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/DEPS
File chrome/renderer/media/DEPS (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/D...
chrome/renderer/media/DEPS:3: "+media/video",  # For basic audio functions.
s/audio/video/ in the comment.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_audio_valve.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_audio_valve.cc:32: cb_ = NULL;
nit: nullptr

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:31:
scoped_refptr<CastReceiverSession> cast_receiver_session_;
nit: const scoped_refptr<...>

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:53:
scoped_refptr<CastReceiverSession> cast_receiver_session_;
ditto on const scoped_refptr<...>

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:138:
CastStreamingVideoCapturerSource::CastStreamingVideoCapturerSource(
Should this class include a destructor with a DCHECK that StopCapture() was
called before destruction?

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:179:
CastStreamingAudioCapturerSource::~CastStreamingAudioCapturerSource() {}
Do we need to check that Stop() was called first?

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:53: friend class
CastStreamingVideoCapturerSource;
Instead of friending, consider making these nested classes to gain access to
private members:

  class CastStreamingVideoCapturerSource;
  class CastStreamingAudioCapturerSource;

(with definition remaining in .cc file)

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:64: const
media::VideoCaptureFormat& format() {
This is strange.  Why call format() if format_ is also private?

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session_delegate.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session_delegate.cc:56: if (!audio_valve_)
Please add:

  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session_delegate.h (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session_delegate.h:30: void
StartAudio(scoped_refptr<CastReceiverAudioValve> audio_valve);
indentation

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:53: cast_environment_ = new
CastEnvironment(
This is a little weird.  I would expect the CastEnvironment to be initialized in
the ctor initializor list, rather than when StartUDP is called.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:42: // Breaks out functionality
that is common between.
Please finish this sentence.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:67:
scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
nit: const scoped_refptr<...>

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
File content/public/renderer/media_stream_source_api.cc (right):

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:55: NULL,
nit: nullptr

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:58:
capturer->SetCapturerSource(source, params);
It works with invalid AudioParameters?  Is that because there's a later
SetFormat() call before the audio comes pouring in?

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:262: NULL,
nit: nullptr

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:145: // for calling Start() on
its own source to have the ball rolling.
nit: s/have/get/

[hubbe, 2015-02-11 22:37:58]

New patchsets have been uploaded after l-g-t-m from
perkj@chromium.org,miu@chromium.org

[hubbe, 2015-02-11 22:38:17]

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/DEPS
File chrome/renderer/media/DEPS (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/D...
chrome/renderer/media/DEPS:3: "+media/video",  # For basic audio functions.
On 2015/02/11 02:52:49, miu wrote:
> s/audio/video/ in the comment.

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_audio_valve.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_audio_valve.cc:32: cb_ = NULL;
On 2015/02/11 02:52:49, miu wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:31:
scoped_refptr<CastReceiverSession> cast_receiver_session_;
On 2015/02/11 02:52:49, miu wrote:
> nit: const scoped_refptr<...>

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:53:
scoped_refptr<CastReceiverSession> cast_receiver_session_;
On 2015/02/11 02:52:49, miu wrote:
> ditto on const scoped_refptr<...>

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:138:
CastStreamingVideoCapturerSource::CastStreamingVideoCapturerSource(
On 2015/02/11 02:52:49, miu wrote:
> Should this class include a destructor with a DCHECK that StopCapture() was
> called before destruction?

For video it is actually not relevant if they called StopCapture or not.
Capture will stop, but there may be additional frame callback calls on another
thread either way.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.cc:179:
CastStreamingAudioCapturerSource::~CastStreamingAudioCapturerSource() {}
On 2015/02/11 02:52:49, miu wrote:
> Do we need to check that Stop() was called first?

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:53: friend class
CastStreamingVideoCapturerSource;
On 2015/02/11 02:52:50, miu wrote:
> Instead of friending, consider making these nested classes to gain access to
> private members:
> 
>   class CastStreamingVideoCapturerSource;
>   class CastStreamingAudioCapturerSource;
> 
> (with definition remaining in .cc file)

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:64: const
media::VideoCaptureFormat& format() {
On 2015/02/11 02:52:49, miu wrote:
> This is strange.  Why call format() if format_ is also private?

Removed.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session_delegate.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session_delegate.cc:56: if (!audio_valve_)
On 2015/02/11 02:52:50, miu wrote:
> Please add:
> 
>   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session_delegate.h (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session_delegate.h:30: void
StartAudio(scoped_refptr<CastReceiverAudioValve> audio_valve);
On 2015/02/11 02:52:50, miu wrote:
> indentation

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.cc (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.cc:53: cast_environment_ = new
CastEnvironment(
On 2015/02/11 02:52:50, miu wrote:
> This is a little weird.  I would expect the CastEnvironment to be initialized
in
> the ctor initializor list, rather than when StartUDP is called.

This is simply replicating existing (but perhaps somewhat odd) behavior.  I can
leave a TODO to clean it up later.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
File chrome/renderer/media/cast_session_delegate.h (right):

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:42: // Breaks out functionality
that is common between.
On 2015/02/11 02:52:50, miu wrote:
> Please finish this sentence.

Done.

https://codereview.chromium.org/883293005/diff/140001/chrome/renderer/media/c...
chrome/renderer/media/cast_session_delegate.h:67:
scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
On 2015/02/11 02:52:50, miu wrote:
> nit: const scoped_refptr<...>

Done.

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
File content/public/renderer/media_stream_source_api.cc (right):

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:44: MediaStreamAudioSource*
audio_source(
On 2015/02/10 15:29:51, perkj wrote:
> This seems to require dark art in order to get right. It should be up to the
> webrtc team to clean up the audio source code so only arguments that make
sense
> are needed and the dependencies understandable. Unfortunately, that will not
> happen for quite some time.
> 

Acknowledged.

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:55: NULL,
On 2015/02/11 02:52:50, miu wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/883293005/diff/140001/content/public/renderer...
content/public/renderer/media_stream_source_api.cc:58:
capturer->SetCapturerSource(source, params);
On 2015/02/11 02:52:50, miu wrote:
> It works with invalid AudioParameters?  Is that because there's a later
> SetFormat() call before the audio comes pouring in?

Unfortunately I have not been able to properly test this code yet, so I don't
know if it really works or not.
I'm not going to check it in until I've managed to actually make it work at
least.

However, the right thing to do is probably to have media::AudioParameters to be
an argument to this function and let the caller figure it out.

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
File content/renderer/media/media_stream_video_capturer_source.cc (right):

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:262: NULL,
On 2015/02/11 02:52:50, miu wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
content/renderer/media/media_stream_video_capturer_source.cc:262: NULL,
On 2015/02/11 02:52:50, miu wrote:
> nit: nullptr

Done.

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/883293005/diff/140001/content/renderer/media/...
content/renderer/media/webrtc_audio_capturer.h:145: // for calling Start() on
its own source to have the ball rolling.
On 2015/02/11 02:52:50, miu wrote:
> nit: s/have/get/

Done.

[hubbe, 2015-02-19 01:13:56]

hubbe@chromium.org changed reviewers:
+ dalecurtis@chromium.org, tommi@chromium.org, tsepez@chromium.org

[hubbe, 2015-02-19 01:13:57]

Ok, preliminary reviews done and the code that uses this code is now done (in a
separate CL). Time for OWNER reviews.

Alpha: Please take another look at cast_receiver_session
       (minor changes)
dalecurtis: please review media/* and chrome/renderer/media/DEPS
tsepez: security review for video_capture_messages.h
tommi: I've renamed some include files, please review the 1-line changes

[Alpha Left Google, 2015-02-19 01:21:07]

https://codereview.chromium.org/883293005/diff/180001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/180001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:67:
media::cast::FrameReceiverConfig video_config_;
Why remove the const for these two members? I don't see them  altered outside of
the constructor.

[hubbe, 2015-02-19 01:22:31]

https://codereview.chromium.org/883293005/diff/180001/chrome/renderer/media/c...
File chrome/renderer/media/cast_receiver_session.h (right):

https://codereview.chromium.org/883293005/diff/180001/chrome/renderer/media/c...
chrome/renderer/media/cast_receiver_session.h:67:
media::cast::FrameReceiverConfig video_config_;
On 2015/02/19 01:21:07, Alpha wrote:
> Why remove the const for these two members? I don't see them  altered outside
of
> the constructor.

They are modified in Start().

[Alpha Left Google, 2015-02-19 01:26:02]

On 2015/02/19 01:22:31, hubbe wrote:
>
https://codereview.chromium.org/883293005/diff/180001/chrome/renderer/media/c...
> File chrome/renderer/media/cast_receiver_session.h (right):
> 
>
https://codereview.chromium.org/883293005/diff/180001/chrome/renderer/media/c...
> chrome/renderer/media/cast_receiver_session.h:67:
> media::cast::FrameReceiverConfig video_config_;
> On 2015/02/19 01:21:07, Alpha wrote:
> > Why remove the const for these two members? I don't see them  altered
outside
> of
> > the constructor.
> 
> They are modified in Start().

Right. I can't read. LGTM then.

[DaleCurtis, 2015-02-19 01:29:40]

Why the move of video/capture stuff into media/base ?

[Tom Sepez, 2015-02-19 17:57:15]

Rubberstamp LGTM for changing include path in video_capture_messages.h

[hubbe, 2015-02-19 18:31:49]

On 2015/02/19 01:29:40, DaleCurtis wrote:
> Why the move of video/capture stuff into media/base ?

Because video_capturer_source.{h,cc} (which is new) depends on it.
Having media/base depend on media/video/capture seems like a non-starter, so at
first I put video_capturer_source.{h,cc} in
media/video/capture, but it Alpha pointed out that it should be in media/base
(and I agree), but that meant moving video_capture_types.{h,cc} as well.

[DaleCurtis, 2015-02-19 20:06:42]

I don't think we want video/capture code in media/base; it's already a poor
dumping ground as it is. The right place feels like video/capture; which should
be anything capture related, not just hardware. If the hardware property is
concerning I'd suggest a reordering within video/capture to accommodate the
naming you like.

Putting it in base/ means the top-level media owners have to review instead of
the video capture specific owners (without OWNERS contortions), which are better
suited.

[Alpha Left Google, 2015-02-19 20:17:06]

On 2015/02/19 20:06:42, DaleCurtis wrote:
> I don't think we want video/capture code in media/base; it's already a poor
> dumping ground as it is. The right place feels like video/capture; which
should
> be anything capture related, not just hardware. If the hardware property is
> concerning I'd suggest a reordering within video/capture to accommodate the
> naming you like.
> 
> Putting it in base/ means the top-level media owners have to review instead of
> the video capture specific owners (without OWNERS contortions), which are
better
> suited.

The reason I suggested VideoCapturerSource be moved to media/base is because
AudioCapturerSource is already there.

It will be strange that we have:
media/base/audio_capturer_source.h
media/video/capture/video_capturer_source.h

And more importantly VideoCapturerSource has nothing to do with interfacing with
video capture hardware, which is what media/video/capture does.
VideoCapturerSource is an interface used only in content/.

[hubbe, 2015-02-19 20:47:00]

On 2015/02/19 20:17:06, Alpha wrote:
> On 2015/02/19 20:06:42, DaleCurtis wrote:
> > I don't think we want video/capture code in media/base; it's already a poor
> > dumping ground as it is. The right place feels like video/capture; which
> should
> > be anything capture related, not just hardware. If the hardware property is
> > concerning I'd suggest a reordering within video/capture to accommodate the
> > naming you like.
> > 
> > Putting it in base/ means the top-level media owners have to review instead
of
> > the video capture specific owners (without OWNERS contortions), which are
> better
> > suited.
> 
> The reason I suggested VideoCapturerSource be moved to media/base is because
> AudioCapturerSource is already there.
> 
> It will be strange that we have:
> media/base/audio_capturer_source.h
> media/video/capture/video_capturer_source.h
> 
> And more importantly VideoCapturerSource has nothing to do with interfacing
with
> video capture hardware, which is what media/video/capture does.
> VideoCapturerSource is an interface used only in content/.

It should also be noted that VideoCapturerSource really has very little to do
with *capturing* a video.
VideoSource might be a better name for the interface, I only named it
VideoCapturerSource because it makes it
obvious that it is similar to AudioCapturerSource.

[DaleCurtis, 2015-02-19 20:51:53]

Hmm, I see. This still feels pretty intimately tied with video capturing, most
notably around the capture device formats query.

What do you think about creating a media/capture directory and moving the new
files and AudioCaptureSource there? You could then move the video/capture folder
to capture/video.

https://codereview.chromium.org/883293005/diff/180001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/883293005/diff/180001/media/base/video_captur...
media/base/video_capture_types.h:5: #ifndef
MEDIA_VIDEO_CAPTURE_VIDEO_CAPTURE_TYPES_H_
Needs updated header guards.

[hubbe, 2015-02-19 22:53:38]

New patchsets have been uploaded after l-g-t-m from
hclam@chromium.org,tsepez@chromium.org

[hubbe, 2015-02-19 22:55:25]

https://codereview.chromium.org/883293005/diff/180001/media/base/video_captur...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/883293005/diff/180001/media/base/video_captur...
media/base/video_capture_types.h:5: #ifndef
MEDIA_VIDEO_CAPTURE_VIDEO_CAPTURE_TYPES_H_
On 2015/02/19 20:51:53, DaleCurtis wrote:
> Needs updated header guards.

Done.

[hubbe, 2015-02-19 23:17:39]

On 2015/02/19 20:51:53, DaleCurtis wrote:
> Hmm, I see. This still feels pretty intimately tied with video capturing, most
> notably around the capture device formats query.
> 
> What do you think about creating a media/capture directory and moving the new
> files and AudioCaptureSource there? You could then move the video/capture
folder
> to capture/video.
> 

I'm not a big fan of the term "capture" actually. In my head it only makes sense
with
certain types of video sources, like "tab capture" or "screen capture". A name
like
media/video/source_interface.h would make more sense to me.

However, you seem to care about the organization of media/* in ways that I don't
so your
wish is my command. If you think a capture/ directory is the way to go, that's
what I'll do.

/Hubbe

[DaleCurtis, 2015-02-20 21:10:30]

I'd defer to the capture/ folk on whether they like that or not. I think for now
since media/base already owns AudioCaptureSource, I'm fine with lgtm 'ing this;
however before any future reviews are approved an organizational plan should be
figured out.

[hubbe, 2015-02-23 23:18:22]

hubbe@chromium.org changed reviewers:
+ jam@chromium.org

[hubbe, 2015-02-23 23:18:23]

Tommi: ping?

Missing LGTM from an OWNER for these files:
    chrome/renderer/BUILD.gn
    content/public/renderer/media_stream_video_sink.h
    content/renderer/pepper/pepper_platform_camera_device.h
    content/renderer/pepper/pepper_platform_video_capture.h
    content/renderer/pepper/pepper_video_capture_host.h

Adding jam as backup if tommi is not around.

[jam, 2015-02-23 23:57:58]

On 2015/02/23 23:18:23, hubbe wrote:
> Tommi: ping?
> 
> Missing LGTM from an OWNER for these files:
>     chrome/renderer/BUILD.gn
>     content/public/renderer/media_stream_video_sink.h
>     content/renderer/pepper/pepper_platform_camera_device.h
>     content/renderer/pepper/pepper_platform_video_capture.h
>     content/renderer/pepper/pepper_video_capture_host.h
> 
> Adding jam as backup if tommi is not around.

lgtm with comment

https://codereview.chromium.org/883293005/diff/220001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/883293005/diff/220001/chrome/renderer/BUILD.g...
chrome/renderer/BUILD.gn:116: "//content/public/renderer",
this doesn't seem necessary, it's already listed above

[hubbe, 2015-02-24 23:17:44]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-02-24 23:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, hclam@chromium.org, perkj@chromium.org,
tsepez@chromium.org, miu@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/883293005/#ps240001
(title: "removed extra BUILD.gn line")

[commit-bot: I haz the power, 2015-02-24 23:18:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/883293005/240001

[commit-bot: I haz the power, 2015-02-25 00:01:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-25 00:02:07]

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