Update PPB_VideoDecoder documentation.

Update PPB_VideoDecoder documentation.
The Decode function should not require the plugin to
maintain its buffer when it completes asynchronously.

BUG=281689
R=dmichael@chromium.org

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

[dmichael (off chromium), 2014-05-20 19:47:32]

lgtm

https://chromiumcodereview.appspot.com/291083003/diff/1/ppapi/api/ppb_video_d...
File ppapi/api/ppb_video_decoder.idl (right):

https://chromiumcodereview.appspot.com/291083003/diff/1/ppapi/api/ppb_video_d...
ppapi/api/ppb_video_decoder.idl:95: * returning PP_OK or by running |callback|
before calling Decode() again.
May be worth noting that otherwise it will fail with PP_ERROR_INPROGRESS

[bbudge, 2014-05-20 21:04:57]

https://codereview.chromium.org/291083003/diff/1/ppapi/api/ppb_video_decoder.idl
File ppapi/api/ppb_video_decoder.idl (right):

https://codereview.chromium.org/291083003/diff/1/ppapi/api/ppb_video_decoder....
ppapi/api/ppb_video_decoder.idl:95: * returning PP_OK or by running |callback|
before calling Decode() again.
On 2014/05/20 19:47:32, dmichael wrote:
> May be worth noting that otherwise it will fail with PP_ERROR_INPROGRESS

Documented this in the @return section.

[bbudge, 2014-05-20 21:05:01]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-05-20 21:05:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bbudge@chromium.org/291083003/60001

[commit-bot: I haz the power, 2014-05-21 00:02:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-21 01:05:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-21 01:05:18]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/7222)

[bbudge, 2014-05-21 01:43:20]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-05-21 01:43:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bbudge@chromium.org/291083003/60001

[bbudge, 2014-05-21 16:37:57]

The CQ bit was unchecked by bbudge@chromium.org

[bbudge, 2014-05-21 16:59:43]

Committed patchset #3 manually as r271917 (presubmit successful).

[igorc, 2014-05-21 19:07:05]

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
File ppapi/api/ppb_video_decoder.idl (right):

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:176: * calls to the decoder other than
GetPicture() and RecyclePicture() until
Just to clarify - am I not allowed to call Reset() while Flush() is pending? I
would prefer that Reset() is allowed, as the purpose of Reset() is to complete
"as quickly as possible".

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:185: * @param[in] callback A
<code>PP_CompletionCallback</code> to be called on
As we discussed - could you add a clarification that the callback cannot be
called recursively from a call such as Flush(), GetPicture(), etc?

The reasoning is that most clients will likely use mutexes around calls, and
would have to queue all callbacks to avoid random deadlocks when a callback is
invoked recursively.

[igorc, 2014-05-21 19:44:36]

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
File ppapi/api/ppb_video_decoder.idl (right):

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:170: [in] PP_VideoPicture picture);
Could you clarify which fields need to be filled out in PP_VideoPicture? I don't
have the original struct and would prefer to provide just the texture id, and
set everything else to zero.

[igorc, 2014-05-21 20:15:46]

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
File ppapi/api/ppb_video_decoder.idl (right):

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:191: int32_t Flush(
Could you also clarify the difference between PP_OK_COMPLETIONPENDING and PP_OK
for this and other calls? I found PP_OK_COMPLETIONPENDING in your example, but
not all calls are checking results there yet.

[igorc, 2014-05-21 20:34:29]

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
File ppapi/api/ppb_video_decoder.idl (right):

https://codereview.chromium.org/291083003/diff/80001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:95: * returning PP_OK or by running |callback|
before calling Decode() again.
And another one - why do we need two ways to successfully complete Decode()?
This makes me write more code than feels necessary... I checked with Elijah and
he said this feels a bit unusual.

For simplicity, I would just say that none of the functions that take callbacks
should ever return PP_OK - they either fail, or tell that callback is now
pending.

[dmichael (off chromium), 2014-05-21 20:54:20]

On Wed, May 21, 2014 at 2:34 PM, <igorc@chromium.org> wrote:

>
> https://codereview.chromium.org/291083003/diff/80001/
> ppapi/api/ppb_video_decoder.idl
> File ppapi/api/ppb_video_decoder.idl (right):
>
> https://codereview.chromium.org/291083003/diff/80001/
> ppapi/api/ppb_video_decoder.idl#newcode95
>
> ppapi/api/ppb_video_decoder.idl:95: * returning PP_OK or by running
> |callback| before calling Decode() again.
> And another one - why do we need two ways to successfully complete
> Decode()? This makes me write more code than feels necessary... I
> checked with Elijah and he said this feels a bit unusual.
>
> For simplicity, I would just say that none of the functions that take
> callbacks should ever return PP_OK - they either fail, or tell that
> callback is now pending.
>
By default, that's actually how it will work. The reason is that by
default, completion callbacks only work asynchronously. But if you want to
squeeze the most performance out, you can use an "OPTIONAL" completion
callback, which will complete *synchronously *if it can. That would allow
you to pipeline decodes. I.e., you call using an optional callback until it
returns PP_OK_COMPLETIONPENDING, at which point you have to wait until a
Decode completes to do the next one.

If you don't want that behavior, you *can* simply use the asynchronous path
all the time. That would actually be the default. But you'll probably add
latency to your decoding.

Here's some documentation on the optional vs required completion callbacks:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_complet...


>
> https://codereview.chromium.org/291083003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[igorc, 2014-05-21 22:54:19]

On 2014/05/21 20:54:20, dmichael wrote:
> On Wed, May 21, 2014 at 2:34 PM, <mailto:igorc@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/291083003/diff/80001/
> > ppapi/api/ppb_video_decoder.idl
> > File ppapi/api/ppb_video_decoder.idl (right):
> >
> > https://codereview.chromium.org/291083003/diff/80001/
> > ppapi/api/ppb_video_decoder.idl#newcode95
> >
> > ppapi/api/ppb_video_decoder.idl:95: * returning PP_OK or by running
> > |callback| before calling Decode() again.
> > And another one - why do we need two ways to successfully complete
> > Decode()? This makes me write more code than feels necessary... I
> > checked with Elijah and he said this feels a bit unusual.
> >
> > For simplicity, I would just say that none of the functions that take
> > callbacks should ever return PP_OK - they either fail, or tell that
> > callback is now pending.
> >
> By default, that's actually how it will work. The reason is that by
> default, completion callbacks only work asynchronously. But if you want to
> squeeze the most performance out, you can use an "OPTIONAL" completion
> callback, which will complete *synchronously *if it can. That would allow
> you to pipeline decodes. I.e., you call using an optional callback until it
> returns PP_OK_COMPLETIONPENDING, at which point you have to wait until a
> Decode completes to do the next one.
> 
> If you don't want that behavior, you *can* simply use the asynchronous path
> all the time. That would actually be the default. But you'll probably add
> latency to your decoding.

The doc says "The plugin should wait until the decoder signals completion by
returning PP_OK or by running |callback| before calling Decode() again."

It does not mention anything about optional callbacks, so my assumption
is that it will still run the callback, and I will get 2 notifications
to decode more data, one of which sounds incorrect.

I may not understand some common knowledge about PPAPI, but I'm still confused
on how to handle 2 successive successes of one operation : )

> 
> Here's some documentation on the optional vs required completion callbacks:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_complet...
> 
> 
> >
> > https://codereview.chromium.org/291083003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.