Pepper: Expose visible_rect to PPB_VideoDecoder.GetPicture.

Pepper: Expose visible_rect to PPB_VideoDecoder.GetPicture.
Adds an out parameter to GetPicture to return the subrectangle of
the picture texture that contains the picture.
Adds a new 0.3 version of PPB_VideoDecoder.

BUG=429071
NOPRESUBMIT=true
R=binji@chromium.org, dmichael@chromium.org, mpearson@chromium.org, tsepez@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/2d4bf3a9f724cfc78d43b6b247397ca76d39b3f4

[bbudge, 2014-11-04 21:44:39]

Patchset #1 (id:1) has been deleted

[bbudge, 2014-11-04 21:46:05]

bbudge@chromium.org changed reviewers:
+ binji@chromium.org, dmichael@chromium.org, tsepez@chromium.org

[bbudge, 2014-11-04 21:46:06]

binji for nacl_sdk
tsepez for IPC ppapi_messages.h

[bbudge, 2014-11-04 21:47:34]

bbudge@chromium.org changed reviewers:
+ mpearson@chromium.org

[bbudge, 2014-11-04 21:47:35]

mpearson@chromium.org: Please review changes in 

histograms.xml

[Mark P, 2014-11-04 22:20:23]

histograms.xml lgtm

[binji, 2014-11-04 22:28:23]

native_client_sdk lgtm

[Tom Sepez, 2014-11-05 00:12:25]

Messages LGTM.

[dmichael (off chromium), 2014-11-05 19:28:08]

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

https://codereview.chromium.org/703753002/diff/40001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:16: M40 = 0.3
What do you think about calling this one 1.0?

https://codereview.chromium.org/703753002/diff/40001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:186: [out] PP_VideoPicture picture,
We could just make a separate PP_VideoPicture_0_2 struct and use it here... 
even if the IDL generator doesn't allow versioning non-API structs. We could
even use a (0.3) PP_VideoPicture inside and copy to the right
PP_VideoPicture_0_2 at the last minute.

Looking at this, IMHO, the added complexity for us isn't that much, and the most
recent stable API would look better. WDYT?

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder.cc
File ppapi/cpp/video_decoder.cc (right):

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:105: pp_resource(), cc.output(),
cc.pp_completion_callback());
Is there some reasonable default we can use for visible_rect in this case? Like
if the result is PP_OK, set it to the texture size from the video picture?

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder.h
File ppapi/cpp/video_decoder.h (right):

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.h:136: const
CompletionCallbackWithOutput<PP_VideoPicture>& callback);
It seems pretty confusing that we have a callback with output, but *also* an
out-param. What do you think of making a small struct here or using std::pair?

[bbudge, 2014-11-05 22:41:48]

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

https://codereview.chromium.org/703753002/diff/40001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:16: M40 = 0.3
On 2014/11/05 19:28:08, dmichael wrote:
> What do you think about calling this one 1.0?

A little gun-shy now, but OK. Done.

https://codereview.chromium.org/703753002/diff/40001/ppapi/api/ppb_video_deco...
ppapi/api/ppb_video_decoder.idl:186: [out] PP_VideoPicture picture,
On 2014/11/05 19:28:08, dmichael wrote:
> We could just make a separate PP_VideoPicture_0_2 struct and use it here... 
> even if the IDL generator doesn't allow versioning non-API structs. We could
> even use a (0.3) PP_VideoPicture inside and copy to the right
> PP_VideoPicture_0_2 at the last minute.
> 
> Looking at this, IMHO, the added complexity for us isn't that much, and the
most
> recent stable API would look better. WDYT?

Redid the patch to define a new version of PP_VideoFrame.

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder.cc
File ppapi/cpp/video_decoder.cc (right):

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:105: pp_resource(), cc.output(),
cc.pp_completion_callback());
On 2014/11/05 19:28:08, dmichael wrote:
> Is there some reasonable default we can use for visible_rect in this case?
Like
> if the result is PP_OK, set it to the texture size from the video picture?

Good idea. Done.

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder.h
File ppapi/cpp/video_decoder.h (right):

https://codereview.chromium.org/703753002/diff/40001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.h:136: const
CompletionCallbackWithOutput<PP_VideoPicture>& callback);
On 2014/11/05 19:28:08, dmichael wrote:
> It seems pretty confusing that we have a callback with output, but *also* an
> out-param. What do you think of making a small struct here or using std::pair?

Using a new struct instead.

[dmichael (off chromium), 2014-11-05 23:06:05]

nits, o/w lgtm

https://codereview.chromium.org/703753002/diff/60001/ppapi/c/ppb_video_decoder.h
File ppapi/c/ppb_video_decoder.h (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/c/ppb_video_decode...
ppapi/c/ppb_video_decoder.h:17: #include "ppapi/c/pp_rect.h"
Any idea why the IDL generator added these? Seems weird

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder.cc
File ppapi/cpp/video_decoder.cc (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:33: struct CallbackData_0_1 {
An overarching comment about the strategy here might help. I like it, but 6
months from now when we have to look at it again, it might save us some trouble
understanding it :)

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:34: CallbackData_0_1(const
CompletionCallbackWithOutput<PP_VideoPicture>& cc)
super nitty nit: explicit

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:35: : original_picture(cc.output()),
nit: May be good to also add:
picture(),
...to get default initialization?

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:52: // Return an empty rect, since we have no
knowledge of texture size.
You mean a full-size rect? I.e., the same size as the texture?

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder.h
File ppapi/cpp/video_decoder.h (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.h:18: struct PP_Rect;
I don't think this is needed

https://codereview.chromium.org/703753002/diff/60001/ppapi/thunk/interfaces_p...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/thunk/interfaces_p...
ppapi/thunk/interfaces_ppb_public_stable.h:97:
PROXIED_IFACE(PPB_VIDEOFRAME_INTERFACE_0_1, PPB_VideoFrame_0_1)
Why did you move this? It should still come after, right?

[bbudge, 2014-11-06 00:26:37]

https://codereview.chromium.org/703753002/diff/60001/ppapi/c/ppb_video_decoder.h
File ppapi/c/ppb_video_decoder.h (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/c/ppb_video_decode...
ppapi/c/ppb_video_decoder.h:17: #include "ppapi/c/pp_rect.h"
On 2014/11/05 23:06:04, dmichael wrote:
> Any idea why the IDL generator added these? Seems weird

Seems like a bug. They are added to pp_codecs.h so not needed here.

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder.cc
File ppapi/cpp/video_decoder.cc (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:33: struct CallbackData_0_1 {
On 2014/11/05 23:06:04, dmichael wrote:
> An overarching comment about the strategy here might help. I like it, but 6
> months from now when we have to look at it again, it might save us some
trouble
> understanding it :)

Done.

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:34: CallbackData_0_1(const
CompletionCallbackWithOutput<PP_VideoPicture>& cc)
On 2014/11/05 23:06:04, dmichael wrote:
> super nitty nit: explicit

Done.

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:35: : original_picture(cc.output()),
On 2014/11/05 23:06:04, dmichael wrote:
> nit: May be good to also add:
> picture(),
> ...to get default initialization?

It's a micro-optimization. It is an 'out' parameter and written on success. On
failure, it is never accessed.
WDYT?

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.cc:52: // Return an empty rect, since we have no
knowledge of texture size.
On 2014/11/05 23:06:04, dmichael wrote:
> You mean a full-size rect? I.e., the same size as the texture?

Gah, comment is stale. Done.

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder.h
File ppapi/cpp/video_decoder.h (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/cpp/video_decoder....
ppapi/cpp/video_decoder.h:18: struct PP_Rect;
On 2014/11/05 23:06:04, dmichael wrote:
> I don't think this is needed

Done.

https://codereview.chromium.org/703753002/diff/60001/ppapi/thunk/interfaces_p...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

https://codereview.chromium.org/703753002/diff/60001/ppapi/thunk/interfaces_p...
ppapi/thunk/interfaces_ppb_public_stable.h:97:
PROXIED_IFACE(PPB_VIDEOFRAME_INTERFACE_0_1, PPB_VideoFrame_0_1)
On 2014/11/05 23:06:04, dmichael wrote:
> Why did you move this? It should still come after, right?

Done.

[bbudge, 2014-11-06 18:40:03]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-11-06 18:41:02]

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

[commit-bot: I haz the power, 2014-11-06 18:48:25]

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

[commit-bot: I haz the power, 2014-11-06 18:48:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bbudge, 2014-11-06 19:15:35]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-11-06 19:16:49]

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

[commit-bot: I haz the power, 2014-11-06 19:23:52]

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

[commit-bot: I haz the power, 2014-11-06 19:23:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bbudge, 2014-11-06 21:17:28]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-11-06 21:18:16]

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

[commit-bot: I haz the power, 2014-11-06 21:23:15]

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

[commit-bot: I haz the power, 2014-11-06 21:23:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bbudge, 2014-11-06 21:55:11]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2014-11-06 21:56:32]

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

[commit-bot: I haz the power, 2014-11-06 22:01:38]

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

[commit-bot: I haz the power, 2014-11-06 22:01:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-11-06 22:33:33]

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

[bbudge, 2014-11-06 22:33:38]

Committed patchset #9 (id:180001) manually as
2d4bf3a9f724cfc78d43b6b247397ca76d39b3f4.