Move PPB_VideoDecoder from 'Dev' to Stable.

Move PPB_VideoDecoder from 'Dev' to Stable.

Notes for discussion:

1) This API provides a low level interface to Chrome's video decoders. Chrome's implementations differ by codec and platform. H264 and VPX expect very different formats, so the plugin must know details about these codecs. In particular, some parsing of the stream is required to extract the frames, and during Reset, the plugin may have to parse into the frames to find start points and make sure the decoder is in the proper state to resume. Some platforms may only provide software implementations. Software and hardware decoders have very different latency and throughput.

2) The PP_VideoPicture struct (in pp_codecs.idl) exposes low level details about the texture format of the decoded pictures so the plugin can render the textures. These are platform-specific.

  GL_TEXTURE_2D is the standard one.
  GL_TEXTURE_RECTANGLE_ARB will be used by Mac in the future (but is not yet used).
  GL_TEXTURE_EXTERNAL_OES is used on ARM ChromeOS systems.

Plugins should be prepared to handle all of them.

3) The plugin continually calls GetPicture to pump the decoder. There is a little weirdness with Flush, where we signal the end of stream by returning PP_ERROR_ABORTED to the plugin's last GetPicture call.

BUG=281689
Committed: https://crrev.com/71be5585c62dbb95d556ddc5ebc6e1f303cd1490
Cr-Commit-Position: refs/heads/master@{#295174}

[bbudge, 2014-09-06 01:00:37]

bbudge@chromium.org changed reviewers:
+ dmichael@chromium.org, igorc@chromium.org, teravest@chromium.org

[dmichael (off chromium), 2014-09-08 16:45:26]

> 2) The PP_VideoPicture struct (in pp_codecs.idl) exposes
> low level details about the texture format of the decoded
> pictures so the plugin can render the textures. These are
> platform-specific.
I don't know enough about OpenGLES, so this may be a stupid or impossible
suggestion. But is there any way to bury that complexity, either in the API or
with something in ppapi/utility, for a plugin that doesn't ever tinker with the
raw bytes of the texture? E.g., it seems like most plugins will only really do
transformations like scaling, but not actually need to understand the layout of
the pixels. So is there any way to abstract away the portion of the OpenGLES
goop that deals with the specific texture type? E.g., the "Shader" and
"Create...ProgramOnce" bits from the example. Can we somehow make something
similar part of the API? The goal would be to make the default/easy thing for a
plugin author to do is make something that works on all relevant platforms.

> 3) The plugin continually calls GetPicture to pump the
> decoder. There is a little weirdness with Flush, where we
> signal the end of stream by returning PP_ERROR_ABORTED to
> the plugin's last GetPicture call.
Hmm, prior to this PP_ERROR_ABORTED had a very specific meaning...  the resource
has been destroyed, so the _callback_ had been aborted. I think "PP_OK" is a
more common way to signal end-of-stream, while prior "successful" callbacks get
some positive value (like the size of the buffer). Would something like that
make sense here?

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

https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder....
ppapi/api/ppb_video_decoder.idl:15: M39 = 0.2
nit: I think you can just drop the [channel=dev] part instead of adding a new
line, since the milestone is the same? If that doesn't work, then ignore me.

[bbudge, 2014-09-09 22:21:17]

On 2014/09/08 16:45:26, dmichael wrote:
> > 2) The PP_VideoPicture struct (in pp_codecs.idl) exposes
> > low level details about the texture format of the decoded
> > pictures so the plugin can render the textures. These are
> > platform-specific.
> I don't know enough about OpenGLES, so this may be a stupid or impossible
> suggestion. But is there any way to bury that complexity, either in the API or
> with something in ppapi/utility, for a plugin that doesn't ever tinker with
the
> raw bytes of the texture? E.g., it seems like most plugins will only really do
> transformations like scaling, but not actually need to understand the layout
of
> the pixels. So is there any way to abstract away the portion of the OpenGLES
> goop that deals with the specific texture type? E.g., the "Shader" and
> "Create...ProgramOnce" bits from the example. Can we somehow make something
> similar part of the API? The goal would be to make the default/easy thing for
a
> plugin author to do is make something that works on all relevant platforms.

I think the plugin has to know the target in order to use the texture. I think
it would be hard to hide this in a way that wasn't overly restrictive.

> 
> > 3) The plugin continually calls GetPicture to pump the
> > decoder. There is a little weirdness with Flush, where we
> > signal the end of stream by returning PP_ERROR_ABORTED to
> > the plugin's last GetPicture call.
> Hmm, prior to this PP_ERROR_ABORTED had a very specific meaning...  the
resource
> has been destroyed, so the _callback_ had been aborted. I think "PP_OK" is a
> more common way to signal end-of-stream, while prior "successful" callbacks
get
> some positive value (like the size of the buffer). Would something like that
> make sense here?

It is pushing a little beyond existing usage, but FileIO, URLLoader, TCP, and
UDP abort callbacks when Close is called.

> 
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder.idl
> File ppapi/api/ppb_video_decoder.idl (right):
> 
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder....
> ppapi/api/ppb_video_decoder.idl:15: M39 = 0.2
> nit: I think you can just drop the [channel=dev] part instead of adding a new
> line, since the milestone is the same? If that doesn't work, then ignore me.

[bbudge, 2014-09-10 00:22:23]

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

https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder....
ppapi/api/ppb_video_decoder.idl:15: M39 = 0.2
On 2014/09/08 16:45:25, dmichael wrote:
> nit: I think you can just drop the [channel=dev] part instead of adding a new
> line, since the milestone is the same? If that doesn't work, then ignore me.

As discussed, I've removed the channel=dev labels for both versions.

0.1 is still added in thunk/interfaces_ppb_pubic_dev_channel.h, while 0.2 is
added in interfaces_ppb_public_stable.h.

[teravest, 2014-09-10 14:36:49]

Why do you want to remove the channel=dev annotation from the 0.1
version in the IDL?

Version 0.1 was a dev channel interface in M36, and that's history we
want to keep, from discussions I remember with Noel.

On Tue, Sep 9, 2014 at 6:22 PM,  <bbudge@chromium.org> wrote:
>
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder.idl
> File ppapi/api/ppb_video_decoder.idl (right):
>
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder....
> ppapi/api/ppb_video_decoder.idl:15: M39 = 0.2
> On 2014/09/08 16:45:25, dmichael wrote:
>>
>> nit: I think you can just drop the [channel=dev] part instead of
>
> adding a new
>>
>> line, since the milestone is the same? If that doesn't work, then
>
> ignore me.
>
> As discussed, I've removed the channel=dev labels for both versions.
>
> 0.1 is still added in thunk/interfaces_ppb_pubic_dev_channel.h, while
> 0.2 is added in interfaces_ppb_public_stable.h.
>
> https://codereview.chromium.org/540393003/

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

[bbudge, 2014-09-10 15:40:30]

On 2014/09/10 14:36:49, teravest wrote:
> Why do you want to remove the channel=dev annotation from the 0.1
> version in the IDL?
> 
> Version 0.1 was a dev channel interface in M36, and that's history we
> want to keep, from discussions I remember with Noel.

The generator strips the 0.1 definition when 0.2 is added as a stable version. I
can update chromium code to the new API but I don't want to break the ARC
plugin.

> 
> On Tue, Sep 9, 2014 at 6:22 PM,  <mailto:bbudge@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder.idl
> > File ppapi/api/ppb_video_decoder.idl (right):
> >
> >
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder....
> > ppapi/api/ppb_video_decoder.idl:15: M39 = 0.2
> > On 2014/09/08 16:45:25, dmichael wrote:
> >>
> >> nit: I think you can just drop the [channel=dev] part instead of
> >
> > adding a new
> >>
> >> line, since the milestone is the same? If that doesn't work, then
> >
> > ignore me.
> >
> > As discussed, I've removed the channel=dev labels for both versions.
> >
> > 0.1 is still added in thunk/interfaces_ppb_pubic_dev_channel.h, while
> > 0.2 is added in interfaces_ppb_public_stable.h.
> >
> > https://codereview.chromium.org/540393003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-09-10 15:41:41]

ok, lgtm

On Wed, Sep 10, 2014 at 9:40 AM,  <bbudge@chromium.org> wrote:
> On 2014/09/10 14:36:49, teravest wrote:
>>
>> Why do you want to remove the channel=dev annotation from the 0.1
>> version in the IDL?
>
>
>> Version 0.1 was a dev channel interface in M36, and that's history we
>> want to keep, from discussions I remember with Noel.
>
>
> The generator strips the 0.1 definition when 0.2 is added as a stable
> version. I
> can update chromium code to the new API but I don't want to break the ARC
> plugin.
>
>
>> On Tue, Sep 9, 2014 at 6:22 PM,  <mailto:bbudge@chromium.org> wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder.idl
>>
>> > File ppapi/api/ppb_video_decoder.idl (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/540393003/diff/1/ppapi/api/ppb_video_decoder....
>>
>> > ppapi/api/ppb_video_decoder.idl:15: M39 = 0.2
>> > On 2014/09/08 16:45:25, dmichael wrote:
>> >>
>> >> nit: I think you can just drop the [channel=dev] part instead of
>> >
>> > adding a new
>> >>
>> >> line, since the milestone is the same? If that doesn't work, then
>> >
>> > ignore me.
>> >
>> > As discussed, I've removed the channel=dev labels for both versions.
>> >
>> > 0.1 is still added in thunk/interfaces_ppb_pubic_dev_channel.h, while
>> > 0.2 is added in interfaces_ppb_public_stable.h.
>> >
>> > https://codereview.chromium.org/540393003/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://codereview.chromium.org/540393003/

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

[teravest, 2014-09-10 15:45:47]

lgtm

[dmichael (off chromium), 2014-09-10 20:11:14]

lgtm

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

https://codereview.chromium.org/540393003/diff/20001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:13: /** Though not labeled 'channel=dev', 0.1 is
a still a 'Dev' only API. */
Just adding more detail from offline discussions for posterity...  If 0.1 has
the "channel=dev" annotation, the IDL generator doesn't generate version 0.1 of
the struct.

That's supposed to be the right behavior, because we don't expect to support a
Dev Channel API that's older than the stable version. However, we whitelisted
0.1 for ARC. So while 0.2 is the version we're planning to ship to stable, we
still need to support 0.1 until ARC moves off of it and sufficient users get
updated to the new version.

Bill: Would you be willing to file a bug against ARC to move to version 0.2, and
a bug for us to deprecate 0.1?

[bbudge, 2014-09-10 20:45:02]

On 2014/09/10 20:11:14, dmichael wrote:
> lgtm
> 
>
https://codereview.chromium.org/540393003/diff/20001/ppapi/api/ppb_video_deco...
> File ppapi/api/ppb_video_decoder.idl (right):
> 
>
https://codereview.chromium.org/540393003/diff/20001/ppapi/api/ppb_video_deco...
> ppapi/api/ppb_video_decoder.idl:13: /** Though not labeled 'channel=dev', 0.1
is
> a still a 'Dev' only API. */
> Just adding more detail from offline discussions for posterity...  If 0.1 has
> the "channel=dev" annotation, the IDL generator doesn't generate version 0.1
of
> the struct.
> 
> That's supposed to be the right behavior, because we don't expect to support a
> Dev Channel API that's older than the stable version. However, we whitelisted
> 0.1 for ARC. So while 0.2 is the version we're planning to ship to stable, we
> still need to support 0.1 until ARC moves off of it and sufficient users get
> updated to the new version.
> 
> Bill: Would you be willing to file a bug against ARC to move to version 0.2,
and
> a bug for us to deprecate 0.1?

Of course. Done.
https://code.google.com/p/chromium/issues/detail?id=412926
https://code.google.com/p/chromium/issues/detail?id=412927

[bbudge, 2014-09-16 22:44:43]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-09-16 22:45:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/540393003/20001

[commit-bot: I haz the power, 2014-09-16 23:29:54]

Committed patchset #2 (id:20001) as b6318d41660ef15016c76701ef1c3d1f6229046e

[commit-bot: I haz the power, 2014-09-16 23:30:28]

Patchset 2 (id:??) landed as
https://crrev.com/71be5585c62dbb95d556ddc5ebc6e1f303cd1490
Cr-Commit-Position: refs/heads/master@{#295174}