Effects Pepper Plugin and MediaStream Glue.

Effects Pepper Plugin and MediaStream Glue.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198312

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198377

[Ronghua Wu (Left Chromium), 2013-04-25 23:33:45]

This is the glue layer between the Effects Pepper Plugin host and the webrtc
MediaStream.

The main classes are in:
video_destination_handler.cc|h
video_source_handler.cc|h

The design doc should have been shared with you.

Note:
1) Converting between the pp video frame and libjingle video frame is missing. I
will talk to Bill offline for this.
2) We are targeting this for M28.

[Ronghua Wu (Left Chromium), 2013-04-29 05:35:06]

Talked to Bill and we agreed that this cl will use whatever libjingle preferred
video frame format. And the pepper host will do necessary converting to the
pepper imagedata. So updated the cl accordingly.

To be clear, I wish:
Tommy and Wei may do the major review. 
Bill and Justin to make sure the interfaces are what you expected, namely (not
limited to) the video_destination_handler.h and video_source_handler.h.

Thanks in advance!

[Ronghua Wu (Left Chromium), 2013-04-30 15:41:19]

Ping for review as we are running out of time.

The cl looks big but as you can see half of them are mocking and testing code.
So it's actually not that big change.

[wjia(left Chromium), 2013-04-30 18:32:11]

Some high level comments. Will dive into detailed later.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_destination_handler.cc (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:80: if (!started_) {
Is there race condition to access |started_|? AFAIK, PutFrame will be called on
render thread since it comes from Pepper. On which thread Stop() will be called?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:92:
MediaStreamRegistryInterface* registry)
What's the life time of factory and registry? How to prevent use-after-free?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:114: PpFrameWriter* capturer
= new PpFrameWriter();
Does it make sense to make PpFrameWriter class internal to this file if it won't
be accessed directly from outside?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_destination_handler.h (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.h:68: bool Open(const
std::string& url, FrameWriterInterface** frame_writer);
Please add detailed comments for API functions.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:45: MediaStreamRegistryInterface*
registry)
How is it guaranteed that we won't have use-after-free case for factory and/or
registry? Would it make sense to use something like weak ptr?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:57: PpFrameReader*
new_frame_reader = new PpFrameReader();
It seems that PpFrameReader is an internal class to this file since it would be
used as cricket::VideoRenderer and FrameReaderInterface by others. In other
words, PpFrameReader is not visible to other classes.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:74:
scoped_refptr<VideoSourceInterface> VideoSourceHandler::GetFirstVideoSource(
Is it possible move this function into media_stream_impl.cc? It seems it has
most of the logic similar/same to what media_stream_impl has.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.h:32: virtual bool
GetFrame(cricket::VideoFrame** frame) = 0;
Any reason for pull model here? How does the plugin decide when to call
GetFrame? Since the frames come from cricket::VideoRenderer, would it be better
to use push model to deliver frames to effects plugin?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.h:68: bool Close(const std::string&
url, FrameReaderInterface* frame_reader);
Please add detailed comments for public API functions.

[Ronghua Wu (Left Chromium), 2013-04-30 22:42:16]

Thanks Wei!

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_destination_handler.cc (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:80: if (!started_) {
On 2013/04/30 18:32:11, wjia wrote:
> Is there race condition to access |started_|? AFAIK, PutFrame will be called
on
> render thread since it comes from Pepper. On which thread Stop() will be
called?

Stop will be called from a libjingle worker thread. So there's a race condition.
What about just remove the check?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:92:
MediaStreamRegistryInterface* registry)
On 2013/04/30 18:32:11, wjia wrote:
> What's the life time of factory and registry? How to prevent use-after-free?

VideoDestinationHandler, the factory and the registry are all owned by the user
of VideoDestinationHandler - in our case the pepper plugin host. So I expect the
pepper plugin host will take care of the life time.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:114: PpFrameWriter* capturer
= new PpFrameWriter();
On 2013/04/30 18:32:11, wjia wrote:
> Does it make sense to make PpFrameWriter class internal to this file if it
won't
> be accessed directly from outside?

It makes sense. But also what to have unit tests for it. It won't be possible if
I put it inside the cc file.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_destination_handler.h (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.h:68: bool Open(const
std::string& url, FrameWriterInterface** frame_writer);
On 2013/04/30 18:32:11, wjia wrote:
> Please add detailed comments for API functions.

Done.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:45: MediaStreamRegistryInterface*
registry)
On 2013/04/30 18:32:11, wjia wrote:
> How is it guaranteed that we won't have use-after-free case for factory and/or
> registry? Would it make sense to use something like weak ptr?

The VideoSourceHandler doesn't take the ownership of the factory and registry. I
expect the user of this class will take care of this.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:57: PpFrameReader*
new_frame_reader = new PpFrameReader();
On 2013/04/30 18:32:11, wjia wrote:
> It seems that PpFrameReader is an internal class to this file since it would
be
> used as cricket::VideoRenderer and FrameReaderInterface by others. In other
> words, PpFrameReader is not visible to other classes.

Correct, I left it in the header more for the testing purpose.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:74:
scoped_refptr<VideoSourceInterface> VideoSourceHandler::GetFirstVideoSource(
On 2013/04/30 18:32:11, wjia wrote:
> Is it possible move this function into media_stream_impl.cc? It seems it has
> most of the logic similar/same to what media_stream_impl has.

I think MediaStreamImpl is suppose be used by WebKit? We don't have access to it
from here. 

So might be easier to keep it here as a local helper function?

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.h:32: virtual bool
GetFrame(cricket::VideoFrame** frame) = 0;
On 2013/04/30 18:32:11, wjia wrote:
> Any reason for pull model here? How does the plugin decide when to call
> GetFrame? Since the frames come from cricket::VideoRenderer, would it be
better
> to use push model to deliver frames to effects plugin?

I asked the same question in the pepper host cl, but I didn't get clear answer
yet. Asked again in the design doc.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.h:68: bool Close(const std::string&
url, FrameReaderInterface* frame_reader);
On 2013/04/30 18:32:11, wjia wrote:
> Please add detailed comments for public API functions.

Done.

[juberti, 2013-04-30 22:54:49]

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
File content/renderer/media/video_destination_handler.cc (right):

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:63: return false;
once is enough

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:67: // TODO(ronghuawu): How
to handle this better?
I think this approach is fine.

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:79: void
PpFrameWriter::PutFrame(cricket::CapturedFrame* frame) {
Should this take an ImageData, so the surrounding code doesn't have to know
about libjingle internals?

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:115: *frame_writer =
capturer;
should the capturer be Start()ed here, and stopped at some point?

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:37: bool
PpFrameReader::GetFrame(VideoFrame** frame) {
Should this emit something in ImageData format, so it can easily be consumed
downstream?

[dmichael (off chromium), 2013-04-30 23:05:39]

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.h:32: virtual bool
GetFrame(cricket::VideoFrame** frame) = 0;
On 2013/04/30 22:42:17, Ronghua Wu wrote:
> On 2013/04/30 18:32:11, wjia wrote:
> > Any reason for pull model here? How does the plugin decide when to call
> > GetFrame? Since the frames come from cricket::VideoRenderer, would it be
> better
> > to use push model to deliver frames to effects plugin?
> 
> I asked the same question in the pepper host cl, but I didn't get clear answer
> yet. Asked again in the design doc.
Either model is fine w.r.t. how you hook it up to Pepper. But push might be
easiest. The PPAPI interface is "pull", but it also allows for synchronous or
asynchronous completion. So when you call GetFrame there, if there's not a frame
already waiting, your plugin will get called later when there *is* a frame. But
if a frame *is* ready, you can get it right away, synchronously. So it really
can be used like the push model; the only difference being each time you get a
frame sent to you, you call GetFrame again to schedule the next "push".

To achieve this, we'll want to buffer frames in the proxy (possibly with a
limit, maybe even only keeping the latest frame). The easiest way to put the
frames there is _probably_ by pushing them. We don't want the out-of-process
plugin to "pull" frames across IPC; that would require a synchronous
plugin->renderer call.

Hope that makes sense.

[Ronghua Wu (Left Chromium), 2013-04-30 23:07:14]

Bill, as Justin brought this up, do you think I should use ImageData as the
interface in this cl? Or you prefer the current way.

Since I called this cl "glue" layer, maybe make sense to hide the libjingle
internal detail for you, i.e. use the ImageData instead of libjingle video frame
in the Get/PutFrame.

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
File content/renderer/media/video_destination_handler.cc (right):

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:63: return false;
On 2013/04/30 22:54:49, juberti wrote:
> once is enough

indeed.

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:67: // TODO(ronghuawu): How
to handle this better?
On 2013/04/30 22:54:49, juberti wrote:
> I think this approach is fine.

Done.

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:79: void
PpFrameWriter::PutFrame(cricket::CapturedFrame* frame) {
On 2013/04/30 22:54:49, juberti wrote:
> Should this take an ImageData, so the surrounding code doesn't have to know
> about libjingle internals?

I talked to Bill about this he thought we should use whatever libjingle format
here so that this cl doesn't need to know the pepper detail. :) I can go either
way.

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_destination_handler.cc:115: *frame_writer =
capturer;
On 2013/04/30 22:54:49, juberti wrote:
> should the capturer be Start()ed here, and stopped at some point?

Once a capturer is added to the video source, it will be started there
(LocalVideoSource::Initialize). And it will be stopped in the LocalVideoSource
dtor.

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.cc (right):

https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
content/renderer/media/video_source_handler.cc:37: bool
PpFrameReader::GetFrame(VideoFrame** frame) {
On 2013/04/30 22:54:49, juberti wrote:
> Should this emit something in ImageData format, so it can easily be consumed
> downstream?

See the other comment.

[bbudge, 2013-04-30 23:09:42]

On 2013/04/30 23:07:14, Ronghua Wu wrote:
> Bill, as Justin brought this up, do you think I should use ImageData as the
> interface in this cl? Or you prefer the current way.
> 
> Since I called this cl "glue" layer, maybe make sense to hide the libjingle
> internal detail for you, i.e. use the ImageData instead of libjingle video
frame
> in the Get/PutFrame.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> File content/renderer/media/video_destination_handler.cc (right):
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:63: return false;
> On 2013/04/30 22:54:49, juberti wrote:
> > once is enough
> 
> indeed.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:67: // TODO(ronghuawu):
How
> to handle this better?
> On 2013/04/30 22:54:49, juberti wrote:
> > I think this approach is fine.
> 
> Done.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:79: void
> PpFrameWriter::PutFrame(cricket::CapturedFrame* frame) {
> On 2013/04/30 22:54:49, juberti wrote:
> > Should this take an ImageData, so the surrounding code doesn't have to know
> > about libjingle internals?
> 
> I talked to Bill about this he thought we should use whatever libjingle format
> here so that this cl doesn't need to know the pepper detail. :) I can go
either
> way.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:115: *frame_writer =
> capturer;
> On 2013/04/30 22:54:49, juberti wrote:
> > should the capturer be Start()ed here, and stopped at some point?
> 
> Once a capturer is added to the video source, it will be started there
> (LocalVideoSource::Initialize). And it will be stopped in the LocalVideoSource
> dtor.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> File content/renderer/media/video_source_handler.cc (right):
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_source_handler.cc:37: bool
> PpFrameReader::GetFrame(VideoFrame** frame) {
> On 2013/04/30 22:54:49, juberti wrote:
> > Should this emit something in ImageData format, so it can easily be consumed
> > downstream?
> 
> See the other comment.

ImageData would be easier for me.

[juberti, 2013-04-30 23:23:29]

On 2013/04/30 23:07:14, Ronghua Wu wrote:
> Bill, as Justin brought this up, do you think I should use ImageData as the
> interface in this cl? Or you prefer the current way.
> 
> Since I called this cl "glue" layer, maybe make sense to hide the libjingle
> internal detail for you, i.e. use the ImageData instead of libjingle video
frame
> in the Get/PutFrame.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> File content/renderer/media/video_destination_handler.cc (right):
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:63: return false;
> On 2013/04/30 22:54:49, juberti wrote:
> > once is enough
> 
> indeed.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:67: // TODO(ronghuawu):
How
> to handle this better?
> On 2013/04/30 22:54:49, juberti wrote:
> > I think this approach is fine.
> 
> Done.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:79: void
> PpFrameWriter::PutFrame(cricket::CapturedFrame* frame) {
> On 2013/04/30 22:54:49, juberti wrote:
> > Should this take an ImageData, so the surrounding code doesn't have to know
> > about libjingle internals?
> 
> I talked to Bill about this he thought we should use whatever libjingle format
> here so that this cl doesn't need to know the pepper detail. :) I can go
either
> way.
>

I think any color conversion should happen in the glue class. The only reason
not to would be if exposing the libjingle native type saved a copy.

>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_destination_handler.cc:115: *frame_writer =
> capturer;
> On 2013/04/30 22:54:49, juberti wrote:
> > should the capturer be Start()ed here, and stopped at some point?
> 
> Once a capturer is added to the video source, it will be started there
> (LocalVideoSource::Initialize). And it will be stopped in the LocalVideoSource
> dtor.
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> File content/renderer/media/video_source_handler.cc (right):
> 
>
https://codereview.chromium.org/14312015/diff/82001/content/renderer/media/vi...
> content/renderer/media/video_source_handler.cc:37: bool
> PpFrameReader::GetFrame(VideoFrame** frame) {
> On 2013/04/30 22:54:49, juberti wrote:
> > Should this emit something in ImageData format, so it can easily be consumed
> > downstream?
> 
> See the other comment.

[Ronghua Wu (Left Chromium), 2013-05-01 04:07:31]

Talked to Bill, he mentioned we should use PPB_ImageData_Impl.

@juberti, your comment makes sense. For the VideoSource case using the
cricket::VideoFrame may actually save memcpy because we can do shallow copy and
let the plugin host to do the last copy before send it back to plugin. I will
sort this out with Bill tomorrow.

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
File content/renderer/media/video_source_handler.h (right):

https://codereview.chromium.org/14312015/diff/78001/content/renderer/media/vi...
content/renderer/media/video_source_handler.h:32: virtual bool
GetFrame(cricket::VideoFrame** frame) = 0;
On 2013/04/30 23:05:39, dmichael wrote:
> On 2013/04/30 22:42:17, Ronghua Wu wrote:
> > On 2013/04/30 18:32:11, wjia wrote:
> > > Any reason for pull model here? How does the plugin decide when to call
> > > GetFrame? Since the frames come from cricket::VideoRenderer, would it be
> > better
> > > to use push model to deliver frames to effects plugin?
> > 
> > I asked the same question in the pepper host cl, but I didn't get clear
answer
> > yet. Asked again in the design doc.
> Either model is fine w.r.t. how you hook it up to Pepper. But push might be
> easiest. The PPAPI interface is "pull", but it also allows for synchronous or
> asynchronous completion. So when you call GetFrame there, if there's not a
frame
> already waiting, your plugin will get called later when there *is* a frame.
But
> if a frame *is* ready, you can get it right away, synchronously. So it really
> can be used like the push model; the only difference being each time you get a
> frame sent to you, you call GetFrame again to schedule the next "push".
> 
> To achieve this, we'll want to buffer frames in the proxy (possibly with a
> limit, maybe even only keeping the latest frame). The easiest way to put the
> frames there is _probably_ by pushing them. We don't want the out-of-process
> plugin to "pull" frames across IPC; that would require a synchronous
> plugin->renderer call.
> 
> Hope that makes sense.

Thanks David.

If I understand correctly, for this cl you suggest we have some kind of callback
to deliver the frames to the plugin proxy/host. Then the proxy can return the
frame to the plugin via the callback. Correct?

[Ronghua Wu (Left Chromium), 2013-05-01 05:34:21]

* Changed GetFrame (pulling) to GotFrame (pushing).
* Changed PutFrame to use PPB_ImageData_Impl. (not completed, few lines
missing.)

PTAL

[Ronghua Wu (Left Chromium), 2013-05-01 22:35:37]

Finished the converting from PPB_ImageData_Impl to cricket::CapturedFrame.

[wjia(left Chromium), 2013-05-03 02:00:38]

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_destination_handler.cc (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.cc:101: LOG(ERROR) <<
"PpFrameWriter::PutFrame - Got RGBA which is not supported.";
Is this technically impossible or just it just can't happen? I think converting
RGBA to BGRA is not a big task.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_destination_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:9: #include <vector>
not needed since vector is used as part of API in cricket::VideoCapturer.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:14: #include
"third_party/WebKit/Source/Platform/chromium/public/WebMediaStream.h"
not used?

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:44: class CONTENT_EXPORT
PpFrameWriter
How is lifetime of PpFrameWriter managed? It will be accessed from both
libjingle and Pepper. How to guarantee it won't be used after free?

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:74: class CONTENT_EXPORT
VideoDestinationHandler {
Will this class be expanded in the future? If not, how about making it a static
function inside PpFrameWriter, e.g.,

static Open(MediaStreamDependencyFactory* factory,
            MediaStreamRegistryInterface* registry,
            const std::string& url,
            FrameWriterInterface** frame_writer);

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_source_handler.cc (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.cc:87: source->RemoveSink(receiver);
should receiver be removed from reader_to_receiver_ somewhere?

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_source_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:12: #include
"base/memory/scoped_ptr.h"
this one is not needed.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:13: #include
"base/synchronization/lock.h"
no lock is used.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:15: #include
"content/renderer/media/media_stream_registry_interface.h"
it's already forward declared. do you still need this one?

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:17: #include
"third_party/libjingle/source/talk/media/base/videoframe.h"
use forward declaration.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:18: #include
"third_party/libjingle/source/talk/media/base/videorenderer.h"
not needed.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:19: #include
"third_party/WebKit/Source/Platform/chromium/public/WebMediaStream.h"
Is this one needed?

[Ronghua Wu (Left Chromium), 2013-05-03 05:37:28]

Thanks Wei. PTAL.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_destination_handler.cc (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.cc:101: LOG(ERROR) <<
"PpFrameWriter::PutFrame - Got RGBA which is not supported.";
On 2013/05/03 02:00:38, wjia wrote:
> Is this technically impossible or just it just can't happen? I think
converting
> RGBA to BGRA is not a big task.

RGBA is not supported by the libjingle video frame, but as you said we can also
do the converting here. However effects plugin should output BGRA according to
the current design. So I thought just put an assert here. 

If later the plugin outputs different format, we can add the converting support
to libjingle then here we can just do frame.fourcc = XXXX; and let the libjingle
does the converting.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_destination_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:9: #include <vector>
On 2013/05/03 02:00:38, wjia wrote:
> not needed since vector is used as part of API in cricket::VideoCapturer.

True. But somehow lint is not happy if I don't include vector. So I will keep it
anyway.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:14: #include
"third_party/WebKit/Source/Platform/chromium/public/WebMediaStream.h"
On 2013/05/03 02:00:38, wjia wrote:
> not used?

Done.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:44: class CONTENT_EXPORT
PpFrameWriter
On 2013/05/03 02:00:38, wjia wrote:
> How is lifetime of PpFrameWriter managed? It will be accessed from both
> libjingle and Pepper. How to guarantee it won't be used after free?

Good question. The PpFrameWriter is owned by the mediatrack. What about let the
Pepper owns a reference to the mediatrack, then PpFrameWriter will always be
valid before the mediatrack is released.

Please see the PpFrameWriterProxy I added.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_destination_handler.h:74: class CONTENT_EXPORT
VideoDestinationHandler {
On 2013/05/03 02:00:38, wjia wrote:
> Will this class be expanded in the future? If not, how about making it a
static
> function inside PpFrameWriter, e.g.,
> 
> static Open(MediaStreamDependencyFactory* factory,
>             MediaStreamRegistryInterface* registry,
>             const std::string& url,
>             FrameWriterInterface** frame_writer);

Didn't seem will be expanded from what I can tell. I think it's a good idea.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_source_handler.cc (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.cc:87: source->RemoveSink(receiver);
On 2013/05/03 02:00:38, wjia wrote:
> should receiver be removed from reader_to_receiver_ somewhere?

It should. thanks.
Added test as well.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
File content/renderer/media/video_source_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:12: #include
"base/memory/scoped_ptr.h"
On 2013/05/03 02:00:38, wjia wrote:
> this one is not needed.

Done.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:13: #include
"base/synchronization/lock.h"
On 2013/05/03 02:00:38, wjia wrote:
> no lock is used.

Done.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:15: #include
"content/renderer/media/media_stream_registry_interface.h"
On 2013/05/03 02:00:38, wjia wrote:
> it's already forward declared. do you still need this one?

Moved to ccl

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:17: #include
"third_party/libjingle/source/talk/media/base/videoframe.h"
On 2013/05/03 02:00:38, wjia wrote:
> use forward declaration.

Done.

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:18: #include
"third_party/libjingle/source/talk/media/base/videorenderer.h"
On 2013/05/03 02:00:38, wjia wrote:
> not needed.

Moved to cc

https://chromiumcodereview.appspot.com/14312015/diff/120002/content/renderer/...
content/renderer/media/video_source_handler.h:19: #include
"third_party/WebKit/Source/Platform/chromium/public/WebMediaStream.h"
On 2013/05/03 02:00:38, wjia wrote:
> Is this one needed?

dito

[Ronghua Wu (Left Chromium), 2013-05-03 19:01:06]

Wei, I've done with the further changes we discussed today:
1) Add lock to protect accessing of |start_|.
2) Holds native video track instead of the webkit video track in the
PpFrameWriterProxy.

PTAL, thanks.

[wjia(left Chromium), 2013-05-03 21:42:55]

look good. possibly last round.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
File content/renderer/media/video_destination_handler.cc (right):

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.cc:158: const std::string& url,
FrameWriterInterface** frame_writer) {
nit: one argument per line.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
File content/renderer/media/video_destination_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:33: virtual void
PutFrame(webkit::ppapi::PPB_ImageData_Impl* image_data,
const webkit::ppapi::PPB_ImageData_Impl& image_data ? It seems image_data will
not be modified inside PutFrame().

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:67: base::Lock lock_;
Please add comment on which is protected by this |lock_|.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:80: // Returns true on
success and false on failure.
Could you add some comments on what are |factory| and |registry|? |registry|
could be NULL?

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:84: FrameWriterInterface**
frame_writer);
Is it possible to use frame_writer==NULL as indication of false? If so, you can
return FrameWriterInterface*, instead of bool.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
File content/renderer/media/video_source_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_source_handler.h:30: // The ownership of the
|frame| transfers to the caller. So the caller must
nit: is transferred to.

[Ronghua Wu (Left Chromium), 2013-05-03 22:40:02]

PTAL thanks!

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
File content/renderer/media/video_destination_handler.cc (right):

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.cc:158: const std::string& url,
FrameWriterInterface** frame_writer) {
On 2013/05/03 21:42:55, wjia wrote:
> nit: one argument per line.

Done.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
File content/renderer/media/video_destination_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:33: virtual void
PutFrame(webkit::ppapi::PPB_ImageData_Impl* image_data,
On 2013/05/03 21:42:55, wjia wrote:
> const webkit::ppapi::PPB_ImageData_Impl& image_data ? It seems image_data will
> not be modified inside PutFrame().

I can't actually. Before using the PPB_ImageData_Impl, I need to verify it with
ImageDataAutoMapper, which doesn't take a const pointer.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:67: base::Lock lock_;
On 2013/05/03 21:42:55, wjia wrote:
> Please add comment on which is protected by this |lock_|.

Done.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:80: // Returns true on
success and false on failure.
On 2013/05/03 21:42:55, wjia wrote:
> Could you add some comments on what are |factory| and |registry|? |registry|
> could be NULL?

Added comments. And change the code a bit so that the factory can be null as
well.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_destination_handler.h:84: FrameWriterInterface**
frame_writer);
On 2013/05/03 21:42:55, wjia wrote:
> Is it possible to use frame_writer==NULL as indication of false? If so, you
can
> return FrameWriterInterface*, instead of bool.

I prefer to keep as it's to align with the other Open we have in the
video_source_handler.

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
File content/renderer/media/video_source_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/98005/content/renderer/m...
content/renderer/media/video_source_handler.h:30: // The ownership of the
|frame| transfers to the caller. So the caller must
On 2013/05/03 21:42:55, wjia wrote:
> nit: is transferred to.

Done.

[wjia(left Chromium), 2013-05-03 22:45:16]

nice work! lgtm with nit.

https://chromiumcodereview.appspot.com/14312015/diff/180001/content/renderer/...
File content/renderer/media/video_destination_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/180001/content/renderer/...
content/renderer/media/video_destination_handler.h:81: // If |factory| is NULL
the MediaStreamDependencyFactory owns by
nit: owned by

[Ronghua Wu (Left Chromium), 2013-05-03 22:47:55]

Thanks a lot Wei.

Will rebase, try one more time and then submit.

https://chromiumcodereview.appspot.com/14312015/diff/180001/content/renderer/...
File content/renderer/media/video_destination_handler.h (right):

https://chromiumcodereview.appspot.com/14312015/diff/180001/content/renderer/...
content/renderer/media/video_destination_handler.h:81: // If |factory| is NULL
the MediaStreamDependencyFactory owns by
On 2013/05/03 22:45:16, wjia wrote:
> nit: owned by

Done.

[commit-bot: I haz the power, 2013-05-03 23:04:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/14312015/191003

[Ronghua Wu (Left Chromium), 2013-05-04 04:21:41]

+ avi, jam, piman

Can any of you review the changes to the 2 gyp files? Thanks!

[Avi (use Gerrit), 2013-05-04 04:45:57]

gyp lgtm

[commit-bot: I haz the power, 2013-05-04 04:48:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/14312015/135006

[commit-bot: I haz the power, 2013-05-04 14:06:20]

Change committed as 198312

[commit-bot: I haz the power, 2013-05-05 15:11:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/14312015/225001

[commit-bot: I haz the power, 2013-05-05 15:30:10]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-05 15:35:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ronghuawu@chromium.org/14312015/170002

[commit-bot: I haz the power, 2013-05-06 05:35:09]

Change committed as 198377

[Jered, 2013-05-07 15:42:54]

On 2013/05/06 05:35:09, I haz the power (commit-bot) wrote:
> Change committed as 198377

This broke enable_webrtc=0 because MediaStreamDependencyFactory is not around.

[Jered, 2013-05-07 15:46:20]

On 2013/05/07 15:42:54, Jered wrote:
> On 2013/05/06 05:35:09, I haz the power (commit-bot) wrote:
> > Change committed as 198377
> 
> This broke enable_webrtc=0 because MediaStreamDependencyFactory is not around.

Or actually VideoDestinationHandler::Open(). Dunno where the reference is,
possibly the one in pepper. 

$ out/Release/chrome
out/Release/chrome: symbol lookup error:
/usr/local/google/chromium.git/src/out/Release/lib/libcontent.so: undefined
symbol:
_ZN7content23VideoDestinationHandler4OpenEPNS_28MediaStreamDependencyFactoryEPNS_28MediaStreamRegistryInterfaceERKSsPPNS_20FrameWriterInterfaceE

which is
content::VideoDestinationHandler::Open(content::MediaStreamDependencyFactory*,
content::MediaStreamRegistryInterface*, std::string const&, content

[Ronghua Wu (Left Chromium), 2013-05-07 17:14:50]

On 2013/05/07 15:46:20, Jered wrote:
> On 2013/05/07 15:42:54, Jered wrote:
> > On 2013/05/06 05:35:09, I haz the power (commit-bot) wrote:
> > > Change committed as 198377
> > 
> > This broke enable_webrtc=0 because MediaStreamDependencyFactory is not
around.
> 
> Or actually VideoDestinationHandler::Open(). Dunno where the reference is,
> possibly the one in pepper. 
> 
> $ out/Release/chrome
> out/Release/chrome: symbol lookup error:
> /usr/local/google/chromium.git/src/out/Release/lib/libcontent.so: undefined
> symbol:
>
_ZN7content23VideoDestinationHandler4OpenEPNS_28MediaStreamDependencyFactoryEPNS_28MediaStreamRegistryInterfaceERKSsPPNS_20FrameWriterInterfaceE
> 
> which is
> content::VideoDestinationHandler::Open(content::MediaStreamDependencyFactory*,
> content::MediaStreamRegistryInterface*, std::string const&, content

Thanks for letting us know. https://codereview.chromium.org/14692004/  should
fix this.