Issue 7553003: Video Capture Pepper API

piman

This is CL 2 of 5 to add a Video Capture API to pepper. This ...

9 years, 4 months ago (2011-08-02 02:42:43 UTC) #1

piman

On 2011/08/02 02:42:43, piman wrote: > This is CL 2 of 5 to add a ...

9 years, 4 months ago (2011-08-02 02:43:53 UTC) #2

wjia(left Chromium)

http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_delegate_impl.cc File content/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_delegate_impl.cc#newcode367 content/renderer/pepper_plugin_delegate_impl.cc:367: video_capture_ = manager->AddDevice(1, handler_proxy_.get()); Is there any plan on ...

9 years, 4 months ago (2011-08-02 16:22:28 UTC) #3

brettw

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/pp_video_capture_dev.h File ppapi/c/dev/pp_video_capture_dev.h (right): http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/pp_video_capture_dev.h#newcode50 ppapi/c/dev/pp_video_capture_dev.h:50: #endif /* PPAPI_C_DEV_PP_VIDEO_CAPTURE_DEV_H_ */ Can you clean up the ...

9 years, 4 months ago (2011-08-02 17:17:44 UTC) #4

darin (slow to review)

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_dev.h File ppapi/c/dev/ppp_video_capture_dev.h (right): http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_dev.h#newcode20 ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev { it is necessary to have PPP_VideoCapture_Dev? ...

9 years, 4 months ago (2011-08-02 18:02:59 UTC) #5

darin (slow to review)

On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote: > > http://codereview.chromium.**org/7553003/diff/1/ppapi/c/** > dev/ppp_video_capture_dev.h<http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_dev.h> ...

9 years, 4 months ago (2011-08-02 18:05:58 UTC) #6

brettw

On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org> wrote: > On Tue, ...

9 years, 4 months ago (2011-08-02 18:08:07 UTC) #7

piman

On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org> wrote: > On Tue, ...

9 years, 4 months ago (2011-08-02 18:40:35 UTC) #8

On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org> wrote:

> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org> wrote:
> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
> >>
> >>
> >>
>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
> >>
> >>
> >>
>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev {
> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
> >> "error" and "buffer-ready" state?  Then, it seems like you just need a
> >> way to get a notification when state changes occur.  That's what
> >> PP_CompletionCallback is for.
> >
> > It's possible my suggestion is crazy.  Just looking for consistency in
> > Pepper API design.  We normally only use PPP_ for cases where the plugin
> is
> > not explicitly requesting something (i.e., sending input events to the
> > plugin).  In this case, the plugin asks to start / stop video capture,
> and
> > there is a way to get the captured video.  This feels a lot like
> > PPB_URLLoader in that you can start it, get data, and stop it.  We should
> > probably try to use a consistent model for all of our APIs.
>
> I've been having the same thought. I think the new video decode API
> uses the PPP approach, so we should figure this out soon. Antoine: do
> you have an opinion on which way is better? We could also have a
> struct of function pointers that we give to the PPB interface, but
> don't actually have the browser do GetInterface for that name. The old
> PPP_Class worked that way.
>

Because PP_CompletionCallbacks are one-offs and not cancellable, whereas all
the functions in the PPP_VideoCapture_Dev can get called an arbitrary number
of times in an arbitrary order, it will make it very awkward for the plugin
to implement something efficient. Basically you'd need to have a PPB
function "NotifyStateChanged" with a callback, and when that callback gets
called, you'll need to inspect the entire state (was it an error ? was it a
status change ? was it a buffer ? was it several of the above ? In what
order ?), and then immediately call NotifyStateChanged again with the
callback. It adds a lot of complicated logic in the browser (which will need
to keep track of state not yet communicated to the plugin), in the proxy
(which will need to serialize the "state" to avoid round trips), and in the
plugin, and I'm not sure I see the benefit.
I view things this way: PPB are service interfaces implemented by the
browser. PPP are client interfaces that the browser calls into (like a pure
interface in C++). When you have 2-way communication you need both, like the
interfaces in the WebKit API (which you could also implement with callbacks,
but what for ?).
Many other services are also already implemented this way with a PPB/PPP
pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't see
a problem.
Lastly, please note that if the PPP interface is designed in a way that if
it isn't implemented (or implemented trivially), nothing bad happens.

> Brett
>

darin (slow to review)

On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 19:52:16 UTC) #9

On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org> wrote:
>
>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org> wrote:
>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>> >>
>> >>
>> >>
>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>> >>
>> >>
>> >>
>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev {
>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>> >> "error" and "buffer-ready" state?  Then, it seems like you just need a
>> >> way to get a notification when state changes occur.  That's what
>> >> PP_CompletionCallback is for.
>> >
>> > It's possible my suggestion is crazy.  Just looking for consistency in
>> > Pepper API design.  We normally only use PPP_ for cases where the plugin
>> is
>> > not explicitly requesting something (i.e., sending input events to the
>> > plugin).  In this case, the plugin asks to start / stop video capture,
>> and
>> > there is a way to get the captured video.  This feels a lot like
>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>> should
>> > probably try to use a consistent model for all of our APIs.
>>
>> I've been having the same thought. I think the new video decode API
>> uses the PPP approach, so we should figure this out soon. Antoine: do
>> you have an opinion on which way is better? We could also have a
>> struct of function pointers that we give to the PPB interface, but
>> don't actually have the browser do GetInterface for that name. The old
>> PPP_Class worked that way.
>>
>
> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
> number of times in an arbitrary order, it will make it very awkward for the
> plugin to implement something efficient. Basically you'd need to have a PPB
> function "NotifyStateChanged" with a callback, and when that callback gets
> called, you'll need to inspect the entire state (was it an error ? was it a
> status change ? was it a buffer ? was it several of the above ? In what
> order ?), and then immediately call NotifyStateChanged again with the
> callback. It adds a lot of complicated logic in the browser (which will need
> to keep track of state not yet communicated to the plugin), in the proxy
> (which will need to serialize the "state" to avoid round trips), and in the
> plugin, and I'm not sure I see the benefit.
> I view things this way: PPB are service interfaces implemented by the
> browser. PPP are client interfaces that the browser calls into (like a pure
> interface in C++). When you have 2-way communication you need both, like the
> interfaces in the WebKit API (which you could also implement with callbacks,
> but what for ?).
> Many other services are also already implemented this way with a PPB/PPP
> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't see
> a problem.
> Lastly, please note that if the PPP interface is designed in a way that if
> it isn't implemented (or implemented trivially), nothing bad happens.
>
>
Scrollbar and Widget have PPP interfaces for out-of-band events.  they are
for notifications like "invalidate" and "valueChanged".  they don't really
correspond to an asynchronous process that the plugin starts / stops in the
same way that they start / stop an URL load or a Video capture.

Video decode seems to have the same issue as Video capture.

It seems to me like you can replace OnDeviceInfo with a
PPB_VideoCapture::Open method that takes an out parameter that receives all
of the information passed to OnDeviceInfo.  It would also take a
CompletionCallback.  When Open completes, you either get an error or upon
success the out parameter is filled in with device info.

Capturing a frame would be poll based instead of push based.  The plugin
would call GetNextBuffer to fetch the next available buffer instead of
waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
out parameter.  When GetNextBuffer completes, you either get an error or
upon success the out parameter is filled with a reference to the buffer.

ReuseBuffer seems like it can stay the same.  And, the notion of stopping
capture, might be better expressed with a Close method to parallel the Open
method.

Seems like this would not change your implementation that much since an IPC
implementation already has to deal with queuing frames to be consumed by the
plugin process.  The differences between push and pull are likely cosmetic.

-Darin



>
>
>> Brett
>>
>
>

darin (slow to review)

On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 19:54:27 UTC) #10

On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>
>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>> wrote:
>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>> >>
>>> >>
>>> >>
>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>> >>
>>> >>
>>> >>
>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev {
>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>> >> "error" and "buffer-ready" state?  Then, it seems like you just need a
>>> >> way to get a notification when state changes occur.  That's what
>>> >> PP_CompletionCallback is for.
>>> >
>>> > It's possible my suggestion is crazy.  Just looking for consistency in
>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>> plugin is
>>> > not explicitly requesting something (i.e., sending input events to the
>>> > plugin).  In this case, the plugin asks to start / stop video capture,
>>> and
>>> > there is a way to get the captured video.  This feels a lot like
>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>> should
>>> > probably try to use a consistent model for all of our APIs.
>>>
>>> I've been having the same thought. I think the new video decode API
>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>> you have an opinion on which way is better? We could also have a
>>> struct of function pointers that we give to the PPB interface, but
>>> don't actually have the browser do GetInterface for that name. The old
>>> PPP_Class worked that way.
>>>
>>
>> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
>> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
>> number of times in an arbitrary order, it will make it very awkward for the
>> plugin to implement something efficient. Basically you'd need to have a PPB
>> function "NotifyStateChanged" with a callback, and when that callback gets
>> called, you'll need to inspect the entire state (was it an error ? was it a
>> status change ? was it a buffer ? was it several of the above ? In what
>> order ?), and then immediately call NotifyStateChanged again with the
>> callback. It adds a lot of complicated logic in the browser (which will need
>> to keep track of state not yet communicated to the plugin), in the proxy
>> (which will need to serialize the "state" to avoid round trips), and in the
>> plugin, and I'm not sure I see the benefit.
>> I view things this way: PPB are service interfaces implemented by the
>> browser. PPP are client interfaces that the browser calls into (like a pure
>> interface in C++). When you have 2-way communication you need both, like the
>> interfaces in the WebKit API (which you could also implement with callbacks,
>> but what for ?).
>> Many other services are also already implemented this way with a PPB/PPP
>> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't see
>> a problem.
>> Lastly, please note that if the PPP interface is designed in a way that if
>> it isn't implemented (or implemented trivially), nothing bad happens.
>>
>>
> Scrollbar and Widget have PPP interfaces for out-of-band events.  they are
> for notifications like "invalidate" and "valueChanged".  they don't really
> correspond to an asynchronous process that the plugin starts / stops in the
> same way that they start / stop an URL load or a Video capture.
>
> Video decode seems to have the same issue as Video capture.
>
> It seems to me like you can replace OnDeviceInfo with a
> PPB_VideoCapture::Open method that takes an out parameter that receives all
> of the information passed to OnDeviceInfo.  It would also take a
> CompletionCallback.  When Open completes, you either get an error or upon
> success the out parameter is filled in with device info.
>
> Capturing a frame would be poll based instead of push based.  The plugin
> would call GetNextBuffer to fetch the next available buffer instead of
> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
> out parameter.  When GetNextBuffer completes, you either get an error or
> upon success the out parameter is filled with a reference to the buffer.
>
> ReuseBuffer seems like it can stay the same.  And, the notion of stopping
> capture, might be better expressed with a Close method to parallel the Open
> method.
>
> Seems like this would not change your implementation that much since an IPC
> implementation already has to deal with queuing frames to be consumed by the
> plugin process.  The differences between push and pull are likely cosmetic.
>
> -Darin
>
>
To be clear, there is much more going on here than just a cosmetic
API-consistency win.  The CompletionCallback approach creates an API that
can be used from any thread.  On a background thread, you can pass "null" to
block until the method call completes.  In this way, you get an API that
doesn't need to be re-designed to be used on a background thread.

I really think we should try to see if this can work for anything high
performance / streaming-like.

-Darin



>
>
>>
>>
>>> Brett
>>>
>>
>>
>

piman

On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 20:20:39 UTC) #11

On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>
>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>> wrote:
>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>> >>
>>> >>
>>> >>
>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>> >>
>>> >>
>>> >>
>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev {
>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>> >> "error" and "buffer-ready" state?  Then, it seems like you just need a
>>> >> way to get a notification when state changes occur.  That's what
>>> >> PP_CompletionCallback is for.
>>> >
>>> > It's possible my suggestion is crazy.  Just looking for consistency in
>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>> plugin is
>>> > not explicitly requesting something (i.e., sending input events to the
>>> > plugin).  In this case, the plugin asks to start / stop video capture,
>>> and
>>> > there is a way to get the captured video.  This feels a lot like
>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>> should
>>> > probably try to use a consistent model for all of our APIs.
>>>
>>> I've been having the same thought. I think the new video decode API
>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>> you have an opinion on which way is better? We could also have a
>>> struct of function pointers that we give to the PPB interface, but
>>> don't actually have the browser do GetInterface for that name. The old
>>> PPP_Class worked that way.
>>>
>>
>> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
>> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
>> number of times in an arbitrary order, it will make it very awkward for the
>> plugin to implement something efficient. Basically you'd need to have a PPB
>> function "NotifyStateChanged" with a callback, and when that callback gets
>> called, you'll need to inspect the entire state (was it an error ? was it a
>> status change ? was it a buffer ? was it several of the above ? In what
>> order ?), and then immediately call NotifyStateChanged again with the
>> callback. It adds a lot of complicated logic in the browser (which will need
>> to keep track of state not yet communicated to the plugin), in the proxy
>> (which will need to serialize the "state" to avoid round trips), and in the
>> plugin, and I'm not sure I see the benefit.
>> I view things this way: PPB are service interfaces implemented by the
>> browser. PPP are client interfaces that the browser calls into (like a pure
>> interface in C++). When you have 2-way communication you need both, like the
>> interfaces in the WebKit API (which you could also implement with callbacks,
>> but what for ?).
>> Many other services are also already implemented this way with a PPB/PPP
>> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't see
>> a problem.
>> Lastly, please note that if the PPP interface is designed in a way that if
>> it isn't implemented (or implemented trivially), nothing bad happens.
>>
>>
> Scrollbar and Widget have PPP interfaces for out-of-band events.  they are
> for notifications like "invalidate" and "valueChanged".  they don't really
> correspond to an asynchronous process that the plugin starts / stops in the
> same way that they start / stop an URL load or a Video capture.
>
> Video decode seems to have the same issue as Video capture.
>
> It seems to me like you can replace OnDeviceInfo with a
> PPB_VideoCapture::Open method that takes an out parameter that receives all
> of the information passed to OnDeviceInfo.  It would also take a
> CompletionCallback.  When Open completes, you either get an error or upon
> success the out parameter is filled in with device info.
>

- OnDeviceInfo also comes out-of-band (may come multiple times per
"StartCapture").
- Such an Open that has a side effect on callback is messy on the plugin
side because you need to keep the recipient of the value in an object that
you keep alive until the callback gets called (and that's harder for plugin
authors to get correct). If you get it wrong, the browser will helpfully
blit an area of freed/reused memory. Not a net win IMO.


> Capturing a frame would be poll based instead of push based.  The plugin
> would call GetNextBuffer to fetch the next available buffer instead of
> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
> out parameter.  When GetNextBuffer completes, you either get an error or
> upon success the out parameter is filled with a reference to the buffer.
>

What if the capture becomes paused because it's out of buffers ? The
callback never comes ? If it comes then you also need a way to poll for the
restart. And then you increase latency because you need an extra round trip.
Also I have the same problems as above with a side effect on callback issue.

Also, I'll make the same argument with InputEvents. Why don't we make a PPB
API that is GetNextInputEvent that takes a callback ? Same with Scrollbar
and Widget.


>
> ReuseBuffer seems like it can stay the same.  And, the notion of stopping
> capture, might be better expressed with a Close method to parallel the Open
> method.
>
> Seems like this would not change your implementation that much since an IPC
> implementation already has to deal with queuing frames to be consumed by the
> plugin process.  The differences between push and pull are likely cosmetic.
>

It becomes messy when you have multiple pieces of state you need to query
asynchronously, e.g. the "next" buffer and the paused/running status. You
need to keep track of the order of one wrt the other. So you need to keep a
queue of event and a state machine on the proxy so that you issue the
callbacks in the right order. Otherwise the plugin will get lost (e.g. gets
the paused callback, but still has buffers coming, but has no way to get all
pending buffers because it doesn't know which one is the last before it gets
paused).
Today, the proxy implementation is trivial. No extra queuing is necessary,
or should I say, is implicit with the message queue. I'll send the CL once
it's cleaned up.

Antoine


>
> -Darin
>
>
>
>>
>>
>>> Brett
>>>
>>
>>
>

piman

On Tue, Aug 2, 2011 at 12:54 PM, Darin Fisher <darin@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 20:23:33 UTC) #12

On Tue, Aug 2, 2011 at 12:54 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>>
>>>
>>>
>>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>>
>>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>>> wrote:
>>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>>> >>
>>>> >>
>>>> >>
>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>>> >>
>>>> >>
>>>> >>
>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev {
>>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>>> >> "error" and "buffer-ready" state?  Then, it seems like you just need
>>>> a
>>>> >> way to get a notification when state changes occur.  That's what
>>>> >> PP_CompletionCallback is for.
>>>> >
>>>> > It's possible my suggestion is crazy.  Just looking for consistency in
>>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>>> plugin is
>>>> > not explicitly requesting something (i.e., sending input events to the
>>>> > plugin).  In this case, the plugin asks to start / stop video capture,
>>>> and
>>>> > there is a way to get the captured video.  This feels a lot like
>>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>>> should
>>>> > probably try to use a consistent model for all of our APIs.
>>>>
>>>> I've been having the same thought. I think the new video decode API
>>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>>> you have an opinion on which way is better? We could also have a
>>>> struct of function pointers that we give to the PPB interface, but
>>>> don't actually have the browser do GetInterface for that name. The old
>>>> PPP_Class worked that way.
>>>>
>>>
>>> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
>>> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
>>> number of times in an arbitrary order, it will make it very awkward for the
>>> plugin to implement something efficient. Basically you'd need to have a PPB
>>> function "NotifyStateChanged" with a callback, and when that callback gets
>>> called, you'll need to inspect the entire state (was it an error ? was it a
>>> status change ? was it a buffer ? was it several of the above ? In what
>>> order ?), and then immediately call NotifyStateChanged again with the
>>> callback. It adds a lot of complicated logic in the browser (which will need
>>> to keep track of state not yet communicated to the plugin), in the proxy
>>> (which will need to serialize the "state" to avoid round trips), and in the
>>> plugin, and I'm not sure I see the benefit.
>>> I view things this way: PPB are service interfaces implemented by the
>>> browser. PPP are client interfaces that the browser calls into (like a pure
>>> interface in C++). When you have 2-way communication you need both, like the
>>> interfaces in the WebKit API (which you could also implement with callbacks,
>>> but what for ?).
>>> Many other services are also already implemented this way with a PPB/PPP
>>> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't see
>>> a problem.
>>> Lastly, please note that if the PPP interface is designed in a way that
>>> if it isn't implemented (or implemented trivially), nothing bad happens.
>>>
>>>
>> Scrollbar and Widget have PPP interfaces for out-of-band events.  they are
>> for notifications like "invalidate" and "valueChanged".  they don't really
>> correspond to an asynchronous process that the plugin starts / stops in the
>> same way that they start / stop an URL load or a Video capture.
>>
>> Video decode seems to have the same issue as Video capture.
>>
>> It seems to me like you can replace OnDeviceInfo with a
>> PPB_VideoCapture::Open method that takes an out parameter that receives all
>> of the information passed to OnDeviceInfo.  It would also take a
>> CompletionCallback.  When Open completes, you either get an error or upon
>> success the out parameter is filled in with device info.
>>
>> Capturing a frame would be poll based instead of push based.  The plugin
>> would call GetNextBuffer to fetch the next available buffer instead of
>> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
>> out parameter.  When GetNextBuffer completes, you either get an error or
>> upon success the out parameter is filled with a reference to the buffer.
>>
>> ReuseBuffer seems like it can stay the same.  And, the notion of stopping
>> capture, might be better expressed with a Close method to parallel the Open
>> method.
>>
>> Seems like this would not change your implementation that much since an
>> IPC implementation already has to deal with queuing frames to be consumed by
>> the plugin process.  The differences between push and pull are likely
>> cosmetic.
>>
>> -Darin
>>
>>
> To be clear, there is much more going on here than just a cosmetic
> API-consistency win.  The CompletionCallback approach creates an API that
> can be used from any thread.  On a background thread, you can pass "null" to
> block until the method call completes.  In this way, you get an API that
> doesn't need to be re-designed to be used on a background thread.
>

That's only true if you wait on only one thing at a time. Otherwise, you
need to use the callback. Nobody has yet implemented out-of-thread PPAPI, so
I don't know if the plan is for callback to be called on the caller thread
or the main thread (both have issues), but the exact same can be done for
the PPP API.

Antoine


>
> I really think we should try to see if this can work for anything high
> performance / streaming-like.
>

> -Darin
>
>
>
>>
>>
>>>
>>>
>>>> Brett
>>>>
>>>
>>>
>>
>

darin (slow to review)

On Tue, Aug 2, 2011 at 1:20 PM, Antoine Labour <piman@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 20:57:24 UTC) #13

On Tue, Aug 2, 2011 at 1:20 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>>
>>>
>>>
>>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>>
>>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>>> wrote:
>>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>>> >>
>>>> >>
>>>> >>
>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>>> >>
>>>> >>
>>>> >>
>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev {
>>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>>> >> "error" and "buffer-ready" state?  Then, it seems like you just need
>>>> a
>>>> >> way to get a notification when state changes occur.  That's what
>>>> >> PP_CompletionCallback is for.
>>>> >
>>>> > It's possible my suggestion is crazy.  Just looking for consistency in
>>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>>> plugin is
>>>> > not explicitly requesting something (i.e., sending input events to the
>>>> > plugin).  In this case, the plugin asks to start / stop video capture,
>>>> and
>>>> > there is a way to get the captured video.  This feels a lot like
>>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>>> should
>>>> > probably try to use a consistent model for all of our APIs.
>>>>
>>>> I've been having the same thought. I think the new video decode API
>>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>>> you have an opinion on which way is better? We could also have a
>>>> struct of function pointers that we give to the PPB interface, but
>>>> don't actually have the browser do GetInterface for that name. The old
>>>> PPP_Class worked that way.
>>>>
>>>
>>> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
>>> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
>>> number of times in an arbitrary order, it will make it very awkward for the
>>> plugin to implement something efficient. Basically you'd need to have a PPB
>>> function "NotifyStateChanged" with a callback, and when that callback gets
>>> called, you'll need to inspect the entire state (was it an error ? was it a
>>> status change ? was it a buffer ? was it several of the above ? In what
>>> order ?), and then immediately call NotifyStateChanged again with the
>>> callback. It adds a lot of complicated logic in the browser (which will need
>>> to keep track of state not yet communicated to the plugin), in the proxy
>>> (which will need to serialize the "state" to avoid round trips), and in the
>>> plugin, and I'm not sure I see the benefit.
>>> I view things this way: PPB are service interfaces implemented by the
>>> browser. PPP are client interfaces that the browser calls into (like a pure
>>> interface in C++). When you have 2-way communication you need both, like the
>>> interfaces in the WebKit API (which you could also implement with callbacks,
>>> but what for ?).
>>> Many other services are also already implemented this way with a PPB/PPP
>>> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't see
>>> a problem.
>>> Lastly, please note that if the PPP interface is designed in a way that
>>> if it isn't implemented (or implemented trivially), nothing bad happens.
>>>
>>>
>> Scrollbar and Widget have PPP interfaces for out-of-band events.  they are
>> for notifications like "invalidate" and "valueChanged".  they don't really
>> correspond to an asynchronous process that the plugin starts / stops in the
>> same way that they start / stop an URL load or a Video capture.
>>
>> Video decode seems to have the same issue as Video capture.
>>
>> It seems to me like you can replace OnDeviceInfo with a
>> PPB_VideoCapture::Open method that takes an out parameter that receives all
>> of the information passed to OnDeviceInfo.  It would also take a
>> CompletionCallback.  When Open completes, you either get an error or upon
>> success the out parameter is filled in with device info.
>>
>
> - OnDeviceInfo also comes out-of-band (may come multiple times per
> "StartCapture").
>

OK, I missed that in the API documentation.  It seemed to be 1:1, but now I
see the comment:

> The browser may change the resolution based on the constraints of the
system,
> in which case OnDeviceInfo will be called again, with new buffers.

Hmm...

One solution would be to have a GetDeviceInfo function that you should call
to verify that you have the latest device info before making assumptions
about the device info.  This would probably not need to take a completion
callback since you could poll it before calling GetNextBuffer.



> - Such an Open that has a side effect on callback is messy on the plugin
> side because you need to keep the recipient of the value in an object that
> you keep alive until the callback gets called (and that's harder for plugin
> authors to get correct). If you get it wrong, the browser will helpfully
> blit an area of freed/reused memory. Not a net win IMO.
>

It sounds like you are making a general argument against
PP_CompletionCallback when there is an out parameter.  We already have many
frozen interfaces that use this method.  I'm not convinced that this is hard
to get right.  The consumer has to explicitly Close the resource on which
they have an outstanding completion callback.

In other words, it isn't clear that this problem is more of a problem for
video capture than it is for file reading.



>
>
>> Capturing a frame would be poll based instead of push based.  The plugin
>> would call GetNextBuffer to fetch the next available buffer instead of
>> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
>> out parameter.  When GetNextBuffer completes, you either get an error or
>> upon success the out parameter is filled with a reference to the buffer.
>>
>
> What if the capture becomes paused because it's out of buffers ? The
> callback never comes ? If it comes then you also need a way to poll for the
> restart. And then you increase latency because you need an extra round trip.
> Also I have the same problems as above with a side effect on callback issue.
>

If the browser wishes to slow down the plugin, then the browser can suspend
the callback.  When the browser wants to resume the plugin, then the browser
can run the callback.



> Also, I'll make the same argument with InputEvents. Why don't we make a PPB
> API that is GetNextInputEvent that takes a callback ? Same with Scrollbar
> and Widget.
>

I really wanted to create a PPB_InputEventQueue and allow for polling input
events (from any thread), but we have a bit of an issue going down that
route.  Input events can be dispatched synchronously from the webkit thread,
and it is important for the plugin to handle them "quickly."

For PPB_Messaging, I also wanted it to work this way, but the push form of
the API is just so much simpler.  The key difference between input events
and messaging is that you don't need a Start function.  For those, you just
implement the PPP interface, and that's enough to cause you to start
receiving events.

For video capture, you have to use a PPB interface to start the asynchronous
process.

(Note that neither Widget nor Scrollbar are frozen.  We may revisit their
design during API review.)



>
>
>>
>> ReuseBuffer seems like it can stay the same.  And, the notion of stopping
>> capture, might be better expressed with a Close method to parallel the Open
>> method.
>>
>> Seems like this would not change your implementation that much since an
>> IPC implementation already has to deal with queuing frames to be consumed by
>> the plugin process.  The differences between push and pull are likely
>> cosmetic.
>>
>
> It becomes messy when you have multiple pieces of state you need to query
> asynchronously, e.g. the "next" buffer and the paused/running status. You
> need to keep track of the order of one wrt the other. So you need to keep a
> queue of event and a state machine on the proxy so that you issue the
> callbacks in the right order. Otherwise the plugin will get lost (e.g. gets
> the paused callback, but still has buffers coming, but has no way to get all
> pending buffers because it doesn't know which one is the last before it gets
> paused).
>

^^^ I had a hard time following the above.  I suspect some time in front of
a whiteboard might help :-)



> Today, the proxy implementation is trivial. No extra queuing is necessary,
> or should I say, is implicit with the message queue. I'll send the CL once
> it's cleaned up.
>

I'd expect that you must have some rate-limiting as you wouldn't want to
blast too much data at the plugin before it has a chance to consume it.
 We've implemented rate limiting like this in many places in Chrome.  In the
end it just ends up looking like a queue with a fixed size.  When the plugin
consumes some data, the browser can put more data into the queue.

-Darin



>
> Antoine
>
>
>>
>> -Darin
>>
>>
>>
>>>
>>>
>>>> Brett
>>>>
>>>
>>>
>>
>

darin (slow to review)

On Tue, Aug 2, 2011 at 1:23 PM, Antoine Labour <piman@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 21:03:16 UTC) #14

On Tue, Aug 2, 2011 at 1:23 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 12:54 PM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>>
>>>
>>> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>>>
>>>>
>>>>
>>>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>>>
>>>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>>>> wrote:
>>>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>>>> >>
>>>>> >>
>>>>> >>
>>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>>>> >>
>>>>> >>
>>>>> >>
>>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev
>>>>> {
>>>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>>>> >> "error" and "buffer-ready" state?  Then, it seems like you just need
>>>>> a
>>>>> >> way to get a notification when state changes occur.  That's what
>>>>> >> PP_CompletionCallback is for.
>>>>> >
>>>>> > It's possible my suggestion is crazy.  Just looking for consistency
>>>>> in
>>>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>>>> plugin is
>>>>> > not explicitly requesting something (i.e., sending input events to
>>>>> the
>>>>> > plugin).  In this case, the plugin asks to start / stop video
>>>>> capture, and
>>>>> > there is a way to get the captured video.  This feels a lot like
>>>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>>>> should
>>>>> > probably try to use a consistent model for all of our APIs.
>>>>>
>>>>> I've been having the same thought. I think the new video decode API
>>>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>>>> you have an opinion on which way is better? We could also have a
>>>>> struct of function pointers that we give to the PPB interface, but
>>>>> don't actually have the browser do GetInterface for that name. The old
>>>>> PPP_Class worked that way.
>>>>>
>>>>
>>>> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
>>>> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
>>>> number of times in an arbitrary order, it will make it very awkward for the
>>>> plugin to implement something efficient. Basically you'd need to have a PPB
>>>> function "NotifyStateChanged" with a callback, and when that callback gets
>>>> called, you'll need to inspect the entire state (was it an error ? was it a
>>>> status change ? was it a buffer ? was it several of the above ? In what
>>>> order ?), and then immediately call NotifyStateChanged again with the
>>>> callback. It adds a lot of complicated logic in the browser (which will
need
>>>> to keep track of state not yet communicated to the plugin), in the proxy
>>>> (which will need to serialize the "state" to avoid round trips), and in the
>>>> plugin, and I'm not sure I see the benefit.
>>>> I view things this way: PPB are service interfaces implemented by the
>>>> browser. PPP are client interfaces that the browser calls into (like a pure
>>>> interface in C++). When you have 2-way communication you need both, like
the
>>>> interfaces in the WebKit API (which you could also implement with
callbacks,
>>>> but what for ?).
>>>> Many other services are also already implemented this way with a PPB/PPP
>>>> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't
see
>>>> a problem.
>>>> Lastly, please note that if the PPP interface is designed in a way that
>>>> if it isn't implemented (or implemented trivially), nothing bad happens.
>>>>
>>>>
>>> Scrollbar and Widget have PPP interfaces for out-of-band events.  they
>>> are for notifications like "invalidate" and "valueChanged".  they don't
>>> really correspond to an asynchronous process that the plugin starts / stops
>>> in the same way that they start / stop an URL load or a Video capture.
>>>
>>> Video decode seems to have the same issue as Video capture.
>>>
>>> It seems to me like you can replace OnDeviceInfo with a
>>> PPB_VideoCapture::Open method that takes an out parameter that receives all
>>> of the information passed to OnDeviceInfo.  It would also take a
>>> CompletionCallback.  When Open completes, you either get an error or upon
>>> success the out parameter is filled in with device info.
>>>
>>> Capturing a frame would be poll based instead of push based.  The plugin
>>> would call GetNextBuffer to fetch the next available buffer instead of
>>> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
>>> out parameter.  When GetNextBuffer completes, you either get an error or
>>> upon success the out parameter is filled with a reference to the buffer.
>>>
>>> ReuseBuffer seems like it can stay the same.  And, the notion of stopping
>>> capture, might be better expressed with a Close method to parallel the Open
>>> method.
>>>
>>> Seems like this would not change your implementation that much since an
>>> IPC implementation already has to deal with queuing frames to be consumed by
>>> the plugin process.  The differences between push and pull are likely
>>> cosmetic.
>>>
>>> -Darin
>>>
>>>
>> To be clear, there is much more going on here than just a cosmetic
>> API-consistency win.  The CompletionCallback approach creates an API that
>> can be used from any thread.  On a background thread, you can pass "null" to
>> block until the method call completes.  In this way, you get an API that
>> doesn't need to be re-designed to be used on a background thread.
>>
>
> That's only true if you wait on only one thing at a time. Otherwise, you
> need to use the callback. Nobody has yet implemented out-of-thread PPAPI, so
> I don't know if the plan is for callback to be called on the caller thread
> or the main thread (both have issues), but the exact same can be done for
> the PPP API.
>
>
Yes, the idea is that you should be able to use these APIs to write "serial"
code, like blocking file reads.  NaCl supports some FileIO methods on a
background thread, but if you pass a completion callback to those methods,
the work callbacks run on the main thread.  This is by design.

Brett and I have talked about providing plugins with a PPB interface to
setup a Pepper event queue on a background thread.  Since the plugin
controls the construction of the thread, they would need to call a PPB
"RunLoop" function to give us control over the thread's execution so we
could schedule callbacks there.

We would probably schedule any callbacks there that originated on the same
thread.  If someone called FileIO::Read(callback) on a background thread
with a registered RunLoop, then we would be able to dispatch the callback to
that RunLoop.

I'm pretty sure we would not be able to schedule PPP calls to a background
thread though.  The idea with PPP calls is that they happen on the main
thread.  There is no obvious connection between a PPB_Foo call made on a
background thread and the plugin's implementation of a PPP_Foo interface.
 This is probably related to Brett's suggestion of having PPB_VideoCapture
be initialized with a function table in case we really had to have an array
of callback functions.

-Darin



> Antoine
>
>
>>
>> I really think we should try to see if this can work for anything high
>> performance / streaming-like.
>>
>
>> -Darin
>>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>> Brett
>>>>>
>>>>
>>>>
>>>
>>
>

piman

On Tue, Aug 2, 2011 at 1:57 PM, Darin Fisher <darin@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 21:51:59 UTC) #15

On Tue, Aug 2, 2011 at 1:57 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 1:20 PM, Antoine Labour <piman@chromium.org> wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>>
>>>
>>> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>>>
>>>>
>>>>
>>>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>>>
>>>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>>>> wrote:
>>>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>>>> >>
>>>>> >>
>>>>> >>
>>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>>>> >>
>>>>> >>
>>>>> >>
>>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev
>>>>> {
>>>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>>>> >> "error" and "buffer-ready" state?  Then, it seems like you just need
>>>>> a
>>>>> >> way to get a notification when state changes occur.  That's what
>>>>> >> PP_CompletionCallback is for.
>>>>> >
>>>>> > It's possible my suggestion is crazy.  Just looking for consistency
>>>>> in
>>>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>>>> plugin is
>>>>> > not explicitly requesting something (i.e., sending input events to
>>>>> the
>>>>> > plugin).  In this case, the plugin asks to start / stop video
>>>>> capture, and
>>>>> > there is a way to get the captured video.  This feels a lot like
>>>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>>>> should
>>>>> > probably try to use a consistent model for all of our APIs.
>>>>>
>>>>> I've been having the same thought. I think the new video decode API
>>>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>>>> you have an opinion on which way is better? We could also have a
>>>>> struct of function pointers that we give to the PPB interface, but
>>>>> don't actually have the browser do GetInterface for that name. The old
>>>>> PPP_Class worked that way.
>>>>>
>>>>
>>>> Because PP_CompletionCallbacks are one-offs and not cancellable, whereas
>>>> all the functions in the PPP_VideoCapture_Dev can get called an arbitrary
>>>> number of times in an arbitrary order, it will make it very awkward for the
>>>> plugin to implement something efficient. Basically you'd need to have a PPB
>>>> function "NotifyStateChanged" with a callback, and when that callback gets
>>>> called, you'll need to inspect the entire state (was it an error ? was it a
>>>> status change ? was it a buffer ? was it several of the above ? In what
>>>> order ?), and then immediately call NotifyStateChanged again with the
>>>> callback. It adds a lot of complicated logic in the browser (which will
need
>>>> to keep track of state not yet communicated to the plugin), in the proxy
>>>> (which will need to serialize the "state" to avoid round trips), and in the
>>>> plugin, and I'm not sure I see the benefit.
>>>> I view things this way: PPB are service interfaces implemented by the
>>>> browser. PPP are client interfaces that the browser calls into (like a pure
>>>> interface in C++). When you have 2-way communication you need both, like
the
>>>> interfaces in the WebKit API (which you could also implement with
callbacks,
>>>> but what for ?).
>>>> Many other services are also already implemented this way with a PPB/PPP
>>>> pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I don't
see
>>>> a problem.
>>>> Lastly, please note that if the PPP interface is designed in a way that
>>>> if it isn't implemented (or implemented trivially), nothing bad happens.
>>>>
>>>>
>>> Scrollbar and Widget have PPP interfaces for out-of-band events.  they
>>> are for notifications like "invalidate" and "valueChanged".  they don't
>>> really correspond to an asynchronous process that the plugin starts / stops
>>> in the same way that they start / stop an URL load or a Video capture.
>>>
>>> Video decode seems to have the same issue as Video capture.
>>>
>>> It seems to me like you can replace OnDeviceInfo with a
>>> PPB_VideoCapture::Open method that takes an out parameter that receives all
>>> of the information passed to OnDeviceInfo.  It would also take a
>>> CompletionCallback.  When Open completes, you either get an error or upon
>>> success the out parameter is filled in with device info.
>>>
>>
>> - OnDeviceInfo also comes out-of-band (may come multiple times per
>> "StartCapture").
>>
>
> OK, I missed that in the API documentation.  It seemed to be 1:1, but now I
> see the comment:
>
> > The browser may change the resolution based on the constraints of the
> system,
> > in which case OnDeviceInfo will be called again, with new buffers.
>
> Hmm...
>
> One solution would be to have a GetDeviceInfo function that you should call
> to verify that you have the latest device info before making assumptions
> about the device info.  This would probably not need to take a completion
> callback since you could poll it before calling GetNextBuffer.
>

That again adds complexity to the plugin, because GetDeviceInfo may or may
not have a new set of buffers. Buffers are allocated by the browser to avoid
round trips, because the browser is the first one to know the size of the
frames. So the plugin would need information about whether the buffers have
changed, to know if it needs to re-map the new ones or not - you don't want
to do that every frame because MMU operations are super costly. Of course,
it can parse the set of buffers, check if they are the same resources or
not, but then again, why ??


>
>
>> - Such an Open that has a side effect on callback is messy on the plugin
>> side because you need to keep the recipient of the value in an object that
>> you keep alive until the callback gets called (and that's harder for plugin
>> authors to get correct). If you get it wrong, the browser will helpfully
>> blit an area of freed/reused memory. Not a net win IMO.
>>
>
> It sounds like you are making a general argument against
> PP_CompletionCallback when there is an out parameter.
>

Yes.


>  We already have many frozen interfaces that use this method.
>

Well, they're frozen, we can't change them. Doesn't mean they're perfect.


>   I'm not convinced that this is hard to get right.
>

We've had bugs caused by it. And they're super hard to track.

The consumer has to explicitly Close the resource on which they have an
> outstanding completion callback.
>

If the plugin has a 1:1 mapping between some kind of object and that
resource, fully controls the lifetime of that object, and is used on a
single thread, then it's relatively easy to ensure.
If any of that is not true then it gets hard.

In other words, it isn't clear that this problem is more of a problem for
> video capture than it is for file reading.
>

I'm not saying that it is more or less of a problem for video vs file
reading. I'm just saying, we have a chance to design an API right, in a way
that is simple to use right and hard to use wrong. Are you arguing that we
should scratch that because some other API we had to rush to finalize
doesn't provide that ?


>
>
>
>>
>>
>>> Capturing a frame would be poll based instead of push based.  The plugin
>>> would call GetNextBuffer to fetch the next available buffer instead of
>>> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
>>> out parameter.  When GetNextBuffer completes, you either get an error or
>>> upon success the out parameter is filled with a reference to the buffer.
>>>
>>
>> What if the capture becomes paused because it's out of buffers ? The
>> callback never comes ? If it comes then you also need a way to poll for the
>> restart. And then you increase latency because you need an extra round trip.
>> Also I have the same problems as above with a side effect on callback issue.
>>
>
> If the browser wishes to slow down the plugin, then the browser can suspend
> the callback.  When the browser wants to resume the plugin, then the browser
> can run the callback.
>

How does the plugin knows that it is suspended ? The plugin may want to
release some resources (e.g. a video encoder), or signal another party (e.g.
video chat) that it has been suspended. It's impossible to know if you just
don't call the callback. We have that problem today with
Graphics2D/Surface3D, that the only way to "know" is essentially by adding a
timeout, which causes problems, races etc.


>
>> Also, I'll make the same argument with InputEvents. Why don't we make a
>> PPB API that is GetNextInputEvent that takes a callback ? Same with
>> Scrollbar and Widget.
>>
>
> I really wanted to create a PPB_InputEventQueue and allow for polling input
> events (from any thread), but we have a bit of an issue going down that
> route.  Input events can be dispatched synchronously from the webkit thread,
> and it is important for the plugin to handle them "quickly."
>
> For PPB_Messaging, I also wanted it to work this way, but the push form of
> the API is just so much simpler.
>

Yes. Same here.


>   The key difference between input events and messaging is that you don't
> need a Start function.
>

  For those, you just implement the PPP interface, and that's enough to
> cause you to start receiving events.
>

That's not true, you have to call PPB_InputEvent.RequestInputEvents


> For video capture, you have to use a PPB interface to start the
> asynchronous process.
>

This is not the first nor the last time you need to deal with more than 1
interfaces for a particular service (including several we have frozen).
As I mention, if you don't implement the PPP interface and just use PPB,
nothing bad will happen (you just won't have any data).


>
> (Note that neither Widget nor Scrollbar are frozen.  We may revisit their
> design during API review.)
>

This is not frozen either, we can certainly revisit. It works this way, and
I have 2 plugins that use it, and it makes a lot of sense from their POV. I
should mention that I've been through several iterations of this API,
including the implementation of the plugins. One of them had a callback for
Start/Stop. The current implementation is by far the simplest from the
plugins pov.


>
>
>>
>>
>>>
>>> ReuseBuffer seems like it can stay the same.  And, the notion of stopping
>>> capture, might be better expressed with a Close method to parallel the Open
>>> method.
>>>
>>> Seems like this would not change your implementation that much since an
>>> IPC implementation already has to deal with queuing frames to be consumed by
>>> the plugin process.  The differences between push and pull are likely
>>> cosmetic.
>>>
>>
>> It becomes messy when you have multiple pieces of state you need to query
>> asynchronously, e.g. the "next" buffer and the paused/running status. You
>> need to keep track of the order of one wrt the other. So you need to keep a
>> queue of event and a state machine on the proxy so that you issue the
>> callbacks in the right order. Otherwise the plugin will get lost (e.g. gets
>> the paused callback, but still has buffers coming, but has no way to get all
>> pending buffers because it doesn't know which one is the last before it gets
>> paused).
>>
>
> ^^^ I had a hard time following the above.  I suspect some time in front of
> a whiteboard might help :-)
>

An example. Say the browser sends frames (1, 2 and 3) to the plugin, then it
needs to change resolution (because e.g. another page also requested the
camera and needed a different resolution), so it'll re-allocate a set of
buffers, and send those to the plugin, then sends more frames (4, 5 and 6)
at the new resolution.
Meanwhile, the plugin was busy drawing stuff or responding to events. If we
implement the GetDeviceInfo trivially, then if the plugin calls this when it
receives the frames 1, 2 and 3, it'll get from GetDeviceInfo the new
resolution instead of the old one. So instead the proxy has to keep an
ordered queue of frames and device info, so that GetDeviceInfo returns the
old resolution until frames 1, 2 and 3 have been received by the plugin.

Today, the proxy implementation is trivial. No extra queuing is necessary,
>> or should I say, is implicit with the message queue. I'll send the CL once
>> it's cleaned up.
>>
>
> I'd expect that you must have some rate-limiting as you wouldn't want to
> blast too much data at the plugin before it has a chance to consume it.
>  We've implemented rate limiting like this in many places in Chrome.  In the
> end it just ends up looking like a queue with a fixed size.  When the plugin
> consumes some data, the browser can put more data into the queue.
>

That already exists in the current code. There's a number of buffers, that
the plugin requests. After the browser sends frames to the plugin, the
buffer is marked "in use" and won't reuse it unless the plugin says it's ok
(it has processed it) by calling ReuseBuffer. If the browser is out of
"free" buffers, it stops sending frames.
Note, that has nothing to do with the proxy.

Antoine


>
> -Darin
>
>
>
>>
>> Antoine
>>
>>
>>>
>>> -Darin
>>>
>>>
>>>
>>>>
>>>>
>>>>> Brett
>>>>>
>>>>
>>>>
>>>
>>
>

piman

On Tue, Aug 2, 2011 at 2:03 PM, Darin Fisher <darin@chromium.org> wrote: > > > ...

9 years, 4 months ago (2011-08-02 21:58:56 UTC) #16

On Tue, Aug 2, 2011 at 2:03 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Tue, Aug 2, 2011 at 1:23 PM, Antoine Labour <piman@chromium.org> wrote:
>
>>
>>
>> On Tue, Aug 2, 2011 at 12:54 PM, Darin Fisher <darin@chromium.org> wrote:
>>
>>>
>>>
>>> On Tue, Aug 2, 2011 at 12:51 PM, Darin Fisher <darin@chromium.org>wrote:
>>>
>>>>
>>>>
>>>> On Tue, Aug 2, 2011 at 11:40 AM, Antoine Labour <piman@chromium.org>wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, Aug 2, 2011 at 11:07 AM, Brett Wilson <brettw@chromium.org>wrote:
>>>>>
>>>>>> On Tue, Aug 2, 2011 at 11:05 AM, Darin Fisher <darin@chromium.org>
>>>>>> wrote:
>>>>>> > On Tue, Aug 2, 2011 at 11:02 AM, <darin@chromium.org> wrote:
>>>>>> >>
>>>>>> >>
>>>>>> >>
>>>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>>>> >> File ppapi/c/dev/ppp_video_capture_dev.h (right):
>>>>>> >>
>>>>>> >>
>>>>>> >>
>>>>>>
http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
>>>>>> >> ppapi/c/dev/ppp_video_capture_dev.h:20: struct PPP_VideoCapture_Dev
>>>>>> {
>>>>>> >> it is necessary to have PPP_VideoCapture_Dev?  Can you instead make
>>>>>> >> PPB_VideoCapture_Dev provide getters for "device-info", "status",
>>>>>> >> "error" and "buffer-ready" state?  Then, it seems like you just
>>>>>> need a
>>>>>> >> way to get a notification when state changes occur.  That's what
>>>>>> >> PP_CompletionCallback is for.
>>>>>> >
>>>>>> > It's possible my suggestion is crazy.  Just looking for consistency
>>>>>> in
>>>>>> > Pepper API design.  We normally only use PPP_ for cases where the
>>>>>> plugin is
>>>>>> > not explicitly requesting something (i.e., sending input events to
>>>>>> the
>>>>>> > plugin).  In this case, the plugin asks to start / stop video
>>>>>> capture, and
>>>>>> > there is a way to get the captured video.  This feels a lot like
>>>>>> > PPB_URLLoader in that you can start it, get data, and stop it.  We
>>>>>> should
>>>>>> > probably try to use a consistent model for all of our APIs.
>>>>>>
>>>>>> I've been having the same thought. I think the new video decode API
>>>>>> uses the PPP approach, so we should figure this out soon. Antoine: do
>>>>>> you have an opinion on which way is better? We could also have a
>>>>>> struct of function pointers that we give to the PPB interface, but
>>>>>> don't actually have the browser do GetInterface for that name. The old
>>>>>> PPP_Class worked that way.
>>>>>>
>>>>>
>>>>> Because PP_CompletionCallbacks are one-offs and not cancellable,
>>>>> whereas all the functions in the PPP_VideoCapture_Dev can get called an
>>>>> arbitrary number of times in an arbitrary order, it will make it very
>>>>> awkward for the plugin to implement something efficient. Basically you'd
>>>>> need to have a PPB function "NotifyStateChanged" with a callback, and when
>>>>> that callback gets called, you'll need to inspect the entire state (was it
>>>>> an error ? was it a status change ? was it a buffer ? was it several of
the
>>>>> above ? In what order ?), and then immediately call NotifyStateChanged
again
>>>>> with the callback. It adds a lot of complicated logic in the browser
(which
>>>>> will need to keep track of state not yet communicated to the plugin), in
the
>>>>> proxy (which will need to serialize the "state" to avoid round trips), and
>>>>> in the plugin, and I'm not sure I see the benefit.
>>>>> I view things this way: PPB are service interfaces implemented by the
>>>>> browser. PPP are client interfaces that the browser calls into (like a
pure
>>>>> interface in C++). When you have 2-way communication you need both, like
the
>>>>> interfaces in the WebKit API (which you could also implement with
callbacks,
>>>>> but what for ?).
>>>>> Many other services are also already implemented this way with a
>>>>> PPB/PPP pair (Scrollbar, Widget, VideoDecoder etc.) so consistency-wise, I
>>>>> don't see a problem.
>>>>> Lastly, please note that if the PPP interface is designed in a way that
>>>>> if it isn't implemented (or implemented trivially), nothing bad happens.
>>>>>
>>>>>
>>>> Scrollbar and Widget have PPP interfaces for out-of-band events.  they
>>>> are for notifications like "invalidate" and "valueChanged".  they don't
>>>> really correspond to an asynchronous process that the plugin starts / stops
>>>> in the same way that they start / stop an URL load or a Video capture.
>>>>
>>>> Video decode seems to have the same issue as Video capture.
>>>>
>>>> It seems to me like you can replace OnDeviceInfo with a
>>>> PPB_VideoCapture::Open method that takes an out parameter that receives all
>>>> of the information passed to OnDeviceInfo.  It would also take a
>>>> CompletionCallback.  When Open completes, you either get an error or upon
>>>> success the out parameter is filled in with device info.
>>>>
>>>> Capturing a frame would be poll based instead of push based.  The plugin
>>>> would call GetNextBuffer to fetch the next available buffer instead of
>>>> waiting for OnBufferReady.  GetNextBuffer takes a CompletionCallback and an
>>>> out parameter.  When GetNextBuffer completes, you either get an error or
>>>> upon success the out parameter is filled with a reference to the buffer.
>>>>
>>>> ReuseBuffer seems like it can stay the same.  And, the notion of
>>>> stopping capture, might be better expressed with a Close method to parallel
>>>> the Open method.
>>>>
>>>> Seems like this would not change your implementation that much since an
>>>> IPC implementation already has to deal with queuing frames to be consumed
by
>>>> the plugin process.  The differences between push and pull are likely
>>>> cosmetic.
>>>>
>>>> -Darin
>>>>
>>>>
>>> To be clear, there is much more going on here than just a cosmetic
>>> API-consistency win.  The CompletionCallback approach creates an API that
>>> can be used from any thread.  On a background thread, you can pass "null" to
>>> block until the method call completes.  In this way, you get an API that
>>> doesn't need to be re-designed to be used on a background thread.
>>>
>>
>> That's only true if you wait on only one thing at a time. Otherwise, you
>> need to use the callback. Nobody has yet implemented out-of-thread PPAPI, so
>> I don't know if the plan is for callback to be called on the caller thread
>> or the main thread (both have issues), but the exact same can be done for
>> the PPP API.
>>
>>
> Yes, the idea is that you should be able to use these APIs to write
> "serial" code, like blocking file reads.  NaCl supports some FileIO methods
> on a background thread, but if you pass a completion callback to those
> methods, the work callbacks run on the main thread.  This is by design.
>
> Brett and I have talked about providing plugins with a PPB interface to
> setup a Pepper event queue on a background thread.  Since the plugin
> controls the construction of the thread, they would need to call a PPB
> "RunLoop" function to give us control over the thread's execution so we
> could schedule callbacks there.
>
> We would probably schedule any callbacks there that originated on the same
> thread.  If someone called FileIO::Read(callback) on a background thread
> with a registered RunLoop, then we would be able to dispatch the callback to
> that RunLoop.
>
> I'm pretty sure we would not be able to schedule PPP calls to a background
> thread though.  The idea with PPP calls is that they happen on the main
> thread.  There is no obvious connection between a PPB_Foo call made on a
> background thread and the plugin's implementation of a PPP_Foo interface.
>  This is probably related to Brett's suggestion of having PPB_VideoCapture
> be initialized with a function table in case we really had to have an array
> of callback functions.
>

One could register one set of PPP functions on each thread. Just a thought.
For this particular PPP api, it's trivial to hop threads anyway so it
doesn't even matter.

Antoine


>
> -Darin
>
>
>
>>  Antoine
>>
>>
>>>
>>> I really think we should try to see if this can work for anything high
>>> performance / streaming-like.
>>>
>>
>>> -Darin
>>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>> Brett
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

piman

http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_delegate_impl.cc File content/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_delegate_impl.cc#newcode367 content/renderer/pepper_plugin_delegate_impl.cc:367: video_capture_ = manager->AddDevice(1, handler_proxy_.get()); On 2011/08/02 16:22:28, wjia wrote: ...

9 years, 4 months ago (2011-08-03 00:44:40 UTC) #17

http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl.cc:367: video_capture_ =
manager->AddDevice(1, handler_proxy_.get());
On 2011/08/02 16:22:28, wjia wrote:
> Is there any plan on device management for pepper?

Yes, I wanted to look at that at some point. Added a todo. Very likely we'll
need an API to enumerate them (and query caps ?), and change the current API to
allow specifying a particular device.

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/pp_video_capture_dev.h
File ppapi/c/dev/pp_video_capture_dev.h (right):

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/pp_video_capture_de...
ppapi/c/dev/pp_video_capture_dev.h:50: #endif  /*
PPAPI_C_DEV_PP_VIDEO_CAPTURE_DEV_H_ */
On 2011/08/02 17:17:44, brettw wrote:
> Can you clean up the whitespace around this?

Done.

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppb_video_capture_d...
File ppapi/c/dev/ppb_video_capture_dev.h (right):

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppb_video_capture_d...
ppapi/c/dev/ppb_video_capture_dev.h:39: * 4:2:0, one byte per pixel, tightly
packed (width x height Y values, then
On 2011/08/02 17:17:44, brettw wrote:
> 4:2:0 -> doesn't this mean there's no V component? Is that correct?

Nope, 4:2:0 means 2/4 ratio of U and V to Y on even lines, and 0/4 ratio of U
and V to Y on odd lines.
Don't ask me why...

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppb_video_capture_d...
ppapi/c/dev/ppb_video_capture_dev.h:55: * the requested resolution and frame
rate. |buffer_count| is the number of
On 2011/08/02 17:17:44, brettw wrote:
> Can you give some guidance on how to pick the buffer count? Without more info,
I
> don't know if I should pick 1 or 100, or what.

Done.

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
File ppapi/c/dev/ppp_video_capture_dev.h (right):

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
ppapi/c/dev/ppp_video_capture_dev.h:33: void (*OnDeviceInfo)(PP_Instance
instance,
On 2011/08/02 17:17:44, brettw wrote:
> Do these functions really need the instance? Normally I would expect just to
be
> given the resource.

All functions in other PPP interfaces that apply to a resource work this way, so
the first argument is consistency.
In theory, it doesn't have to be, because one could keep resource->instance
maps. However, that pattern needs to be replicated in both proxies as well as
the plugins. Map look-ups add a bit of complexity (code complexity as well as
run-time complexity), but we get this instance parameter for free (including in
the proxies). Lastly, we don't currently have a way to associate PPP
implementations to resources in the module, only to instances. Again could be
fixed, but since this comes for free, I tend to like it that way. If you feel
strongly about it, let me know.

http://codereview.chromium.org/7553003/diff/1/ppapi/c/dev/ppp_video_capture_d...
ppapi/c/dev/ppp_video_capture_dev.h:41: * one of the values from
PP_VideoCaptureDeviceInfo_Dev;
On 2011/08/02 16:22:28, wjia wrote:
> PP_VideoCaptureStatus_Dev?

Done.

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
File webkit/plugins/ppapi/ppb_video_capture_impl.cc (right):

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:33: ppp_videocapture_ =
On 2011/08/02 17:17:44, brettw wrote:
> I'd initialize this to null in the initializer list and then move this code to
> the Init function, because that's where you're actually checking that it was
> successfully retrieved.

Done.

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:95: return PP_ERROR_FAILED;
On 2011/08/02 16:22:28, wjia wrote:
> Is this a fatal error, or just warning? It might be ok for plugin to call
> StopCapture more than once. IMO, it does no real harm, just like pressing
"Esc"
> key more than once in vi.

It's essentially silent, except if the plugin looks at the result. So it's a
mechanism for the plugin writer to know that it's doing something wrong, but
without any other side effect.

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:166: media::VideoCapture*
capture,
On 2011/08/02 16:22:28, wjia wrote:
> This |capture| looks like an alien since PPB_VideoCapture_Impl doesn't know
> anything about it. would it be good to check something (capture and
> platform_video_capture_) since the buffer comes from |capture| and is returned
> to platform_video_capture_? One way is to use platform_video_capture_ as
> proxied_ in VideoCaptureProxy. 

So, I was kinda wondering about that, but decided not to do anything special.
I can add logic to pass in the PlatformVideoCapture here (and other calls), but
since |capture| is not used at all in practice, then I figured I'd skip the
extra ~200 lines or so.

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:167:
scoped_refptr<media::VideoCapture::VideoFrameBuffer> buffer) {
On 2011/08/02 16:22:28, wjia wrote:
> it would be good to check buffer (not a NULL pointer).

Is there a reason the VideoCaptureImpl would send NULL ? I don't see how that'd
possible from the code.
Or are you just asking for a DCHECK (I added one) ?

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:174: memcpy(buffers_[i].data,
buffer->memory_pointer, size);
On 2011/08/02 16:22:28, wjia wrote:
> would it be good to call "platform_video_capture_->FeedBuffer(buffer);" here
to
> return buffer faster to media layer? I hope plugin won't do some lengthy
> processing in OnBufferReady.

Done.

http://codereview.chromium.org/7553003/diff/1/webkit/plugins/ppapi/ppb_video_...
webkit/plugins/ppapi/ppb_video_capture_impl.cc:194: FreeBuffers();
On 2011/08/02 16:22:28, wjia wrote:
> is it guaranteed that no buffer is in use by plugin at this time?

The plugin is responsible for keeping its own references to the buffers.
FreeBuffers will simply release the browser-side references, so if the plugin
still has buffers in flight, it can keep them as long as it needs them, the
browser won't free them nor reuse them. When the plugin finally releases
references to its buffers, they will go away.
I renamed FreeBuffers into ReleaseBuffers to better describe the behavior.

wjia(left Chromium)

LGTM. http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_delegate_impl.cc File content/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/7553003/diff/1/content/renderer/pepper_plugin_delegate_impl.cc#newcode367 content/renderer/pepper_plugin_delegate_impl.cc:367: video_capture_ = manager->AddDevice(1, handler_proxy_.get()); On 2011/08/03 00:44:41, piman ...

9 years, 4 months ago (2011-08-03 02:40:00 UTC) #18

piman

PTAL for a few style and other fixes to make the trybots happy. Brett/Darin, any ...

9 years, 4 months ago (2011-08-03 21:45:38 UTC) #19

commit-bot: I haz the power

Try job failure for 7553003-14024 (retry) on linux_clang for step "compile" (clobber build). It's a ...

9 years, 4 months ago (2011-08-04 20:46:57 UTC) #22

Change committed as 95719

Issue 7553003: Video Capture Pepper API (Closed)

Description

Patch Set 1 #

Patch Set 2 : review comments #

Patch Set 3 : rebase #

Patch Set 4 : style & trybot fixes #

Patch Set 5 : more trybot issues #

Patch Set 6 : Make Clang happy #

Messages