Introduce a client minimum picture pool size

Introduce a client minimum picture pool size

When using the video decoder PPAPI, the most recent version of the ARC extension requires there to be a certain minimum number of picture buffers in flight (allocated but not released). Without a larger pool, ARC video decoding will stall trying to allocate more decoded picture frames than are available by default.

This patch creates a new DEV interface version for the VideoDecoder PPAPI. The new interface simply adds a single new argument to VideoDecoder::Initialize() so that a PPAPI client indicate the minimum number of pictures it needs to function.

In order to implement this minium picture count, the meaning of the ProvidePictureBuffers() interface call used by the video decoder implementations has changed slightly. After making the call to ProvidePictureBuffers() with a given picture buffer size, the subsequent callback via AssignPictureBuffers() includes a std::vector of buffers that might be larger than requested.

I've adjusted the various implementations to handle this change -- most of them previously assumed and asserted that the count was the same.

In particular this meant moving some code around in the V4L2 implementations since they also do some internal allocations based on the number of picture buffers that actually end up being chosen.

BUG=485775
Committed: https://crrev.com/9dd55e546c4de8767e83fc4065fa2c8352abb47f
Cr-Commit-Position: refs/heads/master@{#344391}

[lpique, 2015-06-25 00:49:47]

lpique@chromium.org changed reviewers:
+ bbudge@chromium.org, dalecurtis@chromium.org, jar@chromium.org,
jln@chromium.org

[lpique, 2015-06-25 00:49:48]

PTAL.

[DaleCurtis, 2015-06-25 00:57:51]

dalecurtis@chromium.org changed reviewers:
+ posciak@chromium.org

[DaleCurtis, 2015-06-25 00:57:52]

+posciak since I think he'll have more informed opinions than I.

[DaleCurtis, 2015-06-25 01:01:27]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2015-06-25 01:01:43]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2015-06-25 01:01:44]

me -> sandersd@ too.

[DaleCurtis, 2015-06-25 01:01:53]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[bbudge, 2015-06-25 01:20:54]

High level comment: Is the min_picture_count a constant for ARC, or is it
determined by the application? I'm wondering if you can avoid versioning the
interface here, and just pass a larger number from the host to the decoder
implementation.

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

https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
We don't usually support older interface versions in the cpp wrapper, so you can
remove this overload.

[Pawel Osciak, 2015-06-25 01:44:16]

Could you explain where does the requirement come from please? Is this:
1) irrespective of stream requirements, just to keep the pipeline going, i.e.
(each) client(s) need(s) to hold a specified number of buffer(s)? For example,
display may need to always keep at least 1 buffer, because it always needs a
buffer to scan out from (irrespective of stream requirements).
2) or is this parsed from the container/stream minimum per-stream DPB size?

If it's 1), I'd prefer to make this an "additional buffer count", because 2) is
parsed/calculated separately in our codec impls and we would just add this up on
top of that. If it's 2), we'd likely not need it at all.

Could you also please give more details about the stall situation? It's decoder
impls that allocate buffers in VDA, what are the buffers that ARC allocates/how
does it allocate them, and how does that map to VDA buffers?

[ilja, 2015-06-25 02:41:04]

ihf@chromium.org changed reviewers:
+ xzhang@adobe.com

[lpique, 2015-06-25 17:48:44]

On 2015/06/25 01:20:54, bbudge wrote:
> High level comment: Is the min_picture_count a constant for ARC, or is it
> determined by the application? I'm wondering if you can avoid versioning the
> interface here, and just pass a larger number from the host to the decoder
> implementation.

It is a constant for ARC with Android L. I didn't want to make it a constant
here in case it changes again, perhaps because we decided to tweak it.

>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> File ppapi/cpp/video_decoder.h (right):
> 
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> We don't usually support older interface versions in the cpp wrapper, so you
can
> remove this overload.

I thought I remember seeing cases where old versions were supported. I'll remove
them from here though.

[lpique, 2015-06-25 19:26:44]

On 2015/06/25 01:44:16, Pawel Osciak wrote:
> Could you explain where does the requirement come from please? Is this:
> 1) irrespective of stream requirements, just to keep the pipeline going, i.e.
> (each) client(s) need(s) to hold a specified number of buffer(s)? For example,
> display may need to always keep at least 1 buffer, because it always needs a
> buffer to scan out from (irrespective of stream requirements).
> 2) or is this parsed from the container/stream minimum per-stream DPB size?
> 
> If it's 1), I'd prefer to make this an "additional buffer count", because 2)
is
> parsed/calculated separately in our codec impls and we would just add this up
on
> top of that. If it's 2), we'd likely not need it at all.

It is (1). ARC (and the Android code it runs) needs a minimum number of buffers
to keep the pipeline going.

It is why I made it a "minimum count" argument -- if the number of actual
buffers allocated is too low, video decoding stalls trying to allocate more
decode buffers than are available (they are all being held somewhere in the
Android pipeline).

Some of the code I observed for determining the actual number of buffers, such
as that in v4l2_video_decode_accelerator.cc, I found concerning, since the
number of buffers is being obtained by an IOCTL call presumably to a hardware
driver, and might vary from the constants used elsewhere. Specifying "one
additional buffer" might work for the video_decoder_shim.cc case (my linux
desktop), but not end up being enough buffers for other cases (such as a
Chromebook).

> Could you also please give more details about the stall situation? It's
decoder
> impls that allocate buffers in VDA, what are the buffers that ARC
allocates/how
> does it allocate them, and how does that map to VDA buffers?

Sure, the pipeline I see in the extension side of things looks like:

  ARC "hardware" video decoder component (PPAPI client) -> Android video
decoder/queues -> Android BufferQueueProducer -> Android WebView -> Android
BufferConsumer (feeding an Android surface view) -> Android Compositor ->
PPAPI/Chrome compositor -> (then back)

ARC gets the buffers as GLES2 texture names/handles through the
pp::VideoDecoder::GetPicture() call. The conversion go GLES2 textures happens
somewhere inside the Pepper API implementation (video_decoder_resource.cc), but
I didn't look to closely at this, but this hides the VDA buffers.

The GLES2 textures end up being associated with ARC-internal EGL images, which
in turn end up being what back Android GraphicsBuffer objects. When rendered,
they are unwrapped back to the GLES2 texture name, which is passed to the Pepper
Compositor interface for rendering to the actual ARC app window.

The Android video decoding code itself allocates a number of GraphicBuffer
objects with this code:

https://android.googlesource.com/platform/frameworks/av/+/master/media/libsta...
(From Line 749)
    // Use conservative allocation while also trying to reduce starvation
    //
    // 1. allocate at least nBufferCountMin + minUndequeuedBuffers - that is the
    //    minimum needed for the consumer to be able to work
    // 2. try to allocate two (2) additional buffers to reduce starvation from
    //    the consumer
    //    plus an extra buffer to account for incorrect minUndequeuedBufs
    for (OMX_U32 extraBuffers = 2 + 1; /* condition inside loop */;
extraBuffers--) {
        OMX_U32 newBufferCount =
            def.nBufferCountMin + *minUndequeuedBuffers + extraBuffers;

From my inspection, this ends up allocating seven graphics buffers for the
current version of Android. (The last version only allocated three, but the code
has changed)

In my experimentation, I compiled up a version of Chrome where
video_decoder_shim.cc allocated 20 decoder buffers, and observed that decoding
worked, and that there ended up being at most eight buffers allocated to the ARC
extension at any time (the seven requested, and one extra due to the concurrency
involved). Minimally I also found that having video_decoder_shim.cc allocate 6
buffers (One more than the current value of 5) was all that was needed to make
the Android decoding pipeline work, but I expect that minimum would not be good
to use (too much stalling).

[lpique, 2015-06-25 22:21:18]

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

https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
On 2015/06/25 01:20:53, bbudge wrote:
> We don't usually support older interface versions in the cpp wrapper, so you
can
> remove this overload.

Done.

[lpique, 2015-07-07 21:29:30]

On 2015/06/25 22:21:18, lpique wrote:
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> File ppapi/cpp/video_decoder.h (right):
> 
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> On 2015/06/25 01:20:53, bbudge wrote:
> > We don't usually support older interface versions in the cpp wrapper, so you
> can
> > remove this overload.
> 
> Done.

Ping.

[sandersd (OOO until July 31), 2015-07-09 22:36:25]

On 2015/07/07 21:29:30, lpique wrote:
> On 2015/06/25 22:21:18, lpique wrote:
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> > File ppapi/cpp/video_decoder.h (right):
> > 
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> > ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> > On 2015/06/25 01:20:53, bbudge wrote:
> > > We don't usually support older interface versions in the cpp wrapper, so
you
> > can
> > > remove this overload.
> > 
> > Done.
> 
> Ping.

Speaking for VTVideoDecodeAccelerator, I'm not a fan of the naming or bolt-on
nature of the change, but the implementation appears to be correct. I don't
foresee any errors with setting this bound, even when considering the logic for
frame reordering. There is a significant memory cost if the buffers are actually
used, but I suppose that's on ARC.

I would rather see an approach that clarified rather than complicated our method
for selecting the number of pictures -- all of the VDAs follow this same pattern
of minimum_for_pipeline + small_constant. Can we move this logic up a layer
instead of doing math in every VDA?

[lpique, 2015-07-13 18:53:03]

On 2015/07/09 22:36:25, sandersd wrote:
> On 2015/07/07 21:29:30, lpique wrote:
> > On 2015/06/25 22:21:18, lpique wrote:
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> > > File ppapi/cpp/video_decoder.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> > > ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> > > On 2015/06/25 01:20:53, bbudge wrote:
> > > > We don't usually support older interface versions in the cpp wrapper, so
> you
> > > can
> > > > remove this overload.
> > > 
> > > Done.
> > 
> > Ping.
> 
> Speaking for VTVideoDecodeAccelerator, I'm not a fan of the naming or bolt-on
> nature of the change, but the implementation appears to be correct. I don't
> foresee any errors with setting this bound, even when considering the logic
for
> frame reordering. There is a significant memory cost if the buffers are
actually
> used, but I suppose that's on ARC.
> 
> I would rather see an approach that clarified rather than complicated our
method
> for selecting the number of pictures -- all of the VDAs follow this same
pattern
> of minimum_for_pipeline + small_constant. Can we move this logic up a layer
> instead of doing math in every VDA?

I was certainly dismayed at how distributed the logic was for choosing the size
of the picture pool. I chose this approach as it was easier for me to change a
function signature so as to cause compilation to fail. Given my lack of
knowledge on this code, that seemed like the wiser course of action for me.

There seemed to be parts of the code that assumed a requested picture size
passed to ProvidePictureBuffers would match the size of the array of pictures
passed to AssignPictureBuffers. VTVideoDecodeAccelerator doesn't do so, but as a
counterexample consider that AndroidVideoDecodeAccelerator::AssignPictureBuffers
which returns an error if the array size is not the same. [A cursory examination
here looks like the error condition can be changed to simply assert that the
minimum size as provided]

I will however make the attempt to clean it up, and have the picture count
adjusted somewhere behind the ProvidePictureBuffers() call. I'm just worried
about missing something in doing so.

[lpique, 2015-07-14 23:48:04]

lpique@chromium.org changed reviewers:
+ dcaiafa@chromium.org

[lpique, 2015-07-15 16:20:54]

On 2015/07/13 18:53:03, lpique wrote:
> On 2015/07/09 22:36:25, sandersd wrote:
> > On 2015/07/07 21:29:30, lpique wrote:
> > > On 2015/06/25 22:21:18, lpique wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> > > > File ppapi/cpp/video_decoder.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> > > > ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> > > > On 2015/06/25 01:20:53, bbudge wrote:
> > > > > We don't usually support older interface versions in the cpp wrapper,
so
> > you
> > > > can
> > > > > remove this overload.
> > > > 
> > > > Done.
> > > 
> > > Ping.
> > 
> > Speaking for VTVideoDecodeAccelerator, I'm not a fan of the naming or
bolt-on
> > nature of the change, but the implementation appears to be correct. I don't
> > foresee any errors with setting this bound, even when considering the logic
> for
> > frame reordering. There is a significant memory cost if the buffers are
> actually
> > used, but I suppose that's on ARC.
> > 
> > I would rather see an approach that clarified rather than complicated our
> method
> > for selecting the number of pictures -- all of the VDAs follow this same
> pattern
> > of minimum_for_pipeline + small_constant. Can we move this logic up a layer
> > instead of doing math in every VDA?
> 
> I was certainly dismayed at how distributed the logic was for choosing the
size
> of the picture pool. I chose this approach as it was easier for me to change a
> function signature so as to cause compilation to fail. Given my lack of
> knowledge on this code, that seemed like the wiser course of action for me.
> 
> There seemed to be parts of the code that assumed a requested picture size
> passed to ProvidePictureBuffers would match the size of the array of pictures
> passed to AssignPictureBuffers. VTVideoDecodeAccelerator doesn't do so, but as
a
> counterexample consider that
AndroidVideoDecodeAccelerator::AssignPictureBuffers
> which returns an error if the array size is not the same. [A cursory
examination
> here looks like the error condition can be changed to simply assert that the
> minimum size as provided]
> 
> I will however make the attempt to clean it up, and have the picture count
> adjusted somewhere behind the ProvidePictureBuffers() call. I'm just worried
> about missing something in doing so.

Revised. PTAL.

[sandersd (OOO until July 31), 2015-07-15 19:25:22]

On 2015/07/15 16:20:54, lpique wrote:
> On 2015/07/13 18:53:03, lpique wrote:
> > On 2015/07/09 22:36:25, sandersd wrote:
> > > On 2015/07/07 21:29:30, lpique wrote:
> > > > On 2015/06/25 22:21:18, lpique wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> > > > > File ppapi/cpp/video_decoder.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> > > > > ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> > > > > On 2015/06/25 01:20:53, bbudge wrote:
> > > > > > We don't usually support older interface versions in the cpp
wrapper,
> so
> > > you
> > > > > can
> > > > > > remove this overload.
> > > > > 
> > > > > Done.
> > > > 
> > > > Ping.
> > > 
> > > Speaking for VTVideoDecodeAccelerator, I'm not a fan of the naming or
> bolt-on
> > > nature of the change, but the implementation appears to be correct. I
don't
> > > foresee any errors with setting this bound, even when considering the
logic
> > for
> > > frame reordering. There is a significant memory cost if the buffers are
> > actually
> > > used, but I suppose that's on ARC.
> > > 
> > > I would rather see an approach that clarified rather than complicated our
> > method
> > > for selecting the number of pictures -- all of the VDAs follow this same
> > pattern
> > > of minimum_for_pipeline + small_constant. Can we move this logic up a
layer
> > > instead of doing math in every VDA?
> > 
> > I was certainly dismayed at how distributed the logic was for choosing the
> size
> > of the picture pool. I chose this approach as it was easier for me to change
a
> > function signature so as to cause compilation to fail. Given my lack of
> > knowledge on this code, that seemed like the wiser course of action for me.
> > 
> > There seemed to be parts of the code that assumed a requested picture size
> > passed to ProvidePictureBuffers would match the size of the array of
pictures
> > passed to AssignPictureBuffers. VTVideoDecodeAccelerator doesn't do so, but
as
> a
> > counterexample consider that
> AndroidVideoDecodeAccelerator::AssignPictureBuffers
> > which returns an error if the array size is not the same. [A cursory
> examination
> > here looks like the error condition can be changed to simply assert that the
> > minimum size as provided]
> > 
> > I will however make the attempt to clean it up, and have the picture count
> > adjusted somewhere behind the ProvidePictureBuffers() call. I'm just worried
> > about missing something in doing so.
> 
> Revised. PTAL.

From the VDA side, it's not quite what I was expecting, but I'm a fan of this
design. I'd ask that you update the VideoDecodeAccelerator header to be clear
that the requested and assigned quantities can be different.

posciak@: I'm not familiar with the v4l2 code, can you take a look?

[lpique, 2015-07-15 21:00:16]

On 2015/07/15 19:25:22, sandersd wrote:
> On 2015/07/15 16:20:54, lpique wrote:
> > On 2015/07/13 18:53:03, lpique wrote:
> > > On 2015/07/09 22:36:25, sandersd wrote:
> > > > On 2015/07/07 21:29:30, lpique wrote:
> > > > > On 2015/06/25 22:21:18, lpique wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder.h
> > > > > > File ppapi/cpp/video_decoder.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1207043002/diff/30001/ppapi/cpp/video_decoder...
> > > > > > ppapi/cpp/video_decoder.h:78: const CompletionCallback& callback);
> > > > > > On 2015/06/25 01:20:53, bbudge wrote:
> > > > > > > We don't usually support older interface versions in the cpp
> wrapper,
> > so
> > > > you
> > > > > > can
> > > > > > > remove this overload.
> > > > > > 
> > > > > > Done.
> > > > > 
> > > > > Ping.
> > > > 
> > > > Speaking for VTVideoDecodeAccelerator, I'm not a fan of the naming or
> > bolt-on
> > > > nature of the change, but the implementation appears to be correct. I
> don't
> > > > foresee any errors with setting this bound, even when considering the
> logic
> > > for
> > > > frame reordering. There is a significant memory cost if the buffers are
> > > actually
> > > > used, but I suppose that's on ARC.
> > > > 
> > > > I would rather see an approach that clarified rather than complicated
our
> > > method
> > > > for selecting the number of pictures -- all of the VDAs follow this same
> > > pattern
> > > > of minimum_for_pipeline + small_constant. Can we move this logic up a
> layer
> > > > instead of doing math in every VDA?
> > > 
> > > I was certainly dismayed at how distributed the logic was for choosing the
> > size
> > > of the picture pool. I chose this approach as it was easier for me to
change
> a
> > > function signature so as to cause compilation to fail. Given my lack of
> > > knowledge on this code, that seemed like the wiser course of action for
me.
> > > 
> > > There seemed to be parts of the code that assumed a requested picture size
> > > passed to ProvidePictureBuffers would match the size of the array of
> pictures
> > > passed to AssignPictureBuffers. VTVideoDecodeAccelerator doesn't do so,
but
> as
> > a
> > > counterexample consider that
> > AndroidVideoDecodeAccelerator::AssignPictureBuffers
> > > which returns an error if the array size is not the same. [A cursory
> > examination
> > > here looks like the error condition can be changed to simply assert that
the
> > > minimum size as provided]
> > > 
> > > I will however make the attempt to clean it up, and have the picture count
> > > adjusted somewhere behind the ProvidePictureBuffers() call. I'm just
worried
> > > about missing something in doing so.
> > 
> > Revised. PTAL.
> 
> From the VDA side, it's not quite what I was expecting, but I'm a fan of this
> design. I'd ask that you update the VideoDecodeAccelerator header to be clear
> that the requested and assigned quantities can be different.
> 
> posciak@: I'm not familiar with the v4l2 code, can you take a look?

Thanks. I meant to add that and forgot. Done.

[sandersd (OOO until July 31), 2015-07-20 17:37:18]

With no response from Pawel, lgtm for content/common/gpu/media.

[lpique, 2015-07-21 19:22:21]

On 2015/07/20 17:37:18, sandersd wrote:
> With no response from Pawel, lgtm for content/common/gpu/media.

Thanks.

@bbudge - Ping?

[bbudge, 2015-07-22 02:02:47]

Sorry for tardiness. I think we want to make this 'Dev' at first.

https://codereview.chromium.org/1207043002/diff/90001/ppapi/api/ppb_video_dec...
File ppapi/api/ppb_video_decoder.idl (right):

https://codereview.chromium.org/1207043002/diff/90001/ppapi/api/ppb_video_dec...
ppapi/api/ppb_video_decoder.idl:17: M44 = 1.1
Usually we expose a new API to 'Dev' channel and review it before promoting to
'Stable'.

[lpique, 2015-08-07 20:15:38]

On 2015/07/22 02:02:47, bbudge wrote:
> Sorry for tardiness. I think we want to make this 'Dev' at first.
> 
>
https://codereview.chromium.org/1207043002/diff/90001/ppapi/api/ppb_video_dec...
> File ppapi/api/ppb_video_decoder.idl (right):
> 
>
https://codereview.chromium.org/1207043002/diff/90001/ppapi/api/ppb_video_dec...
> ppapi/api/ppb_video_decoder.idl:17: M44 = 1.1
> Usually we expose a new API to 'Dev' channel and review it before promoting to
> 'Stable'.

PTAL.

I think I've now done it correctly, after misunderstanding what making the
interface "Dev" entails -- going down the wrong path and modifying
ppb_video_decoder_dev.idl.

Things I expect I still must do before this can be submitted:

  1) Make it so ppapi/cpp/video_decoder.cc does not use the now DEV-only
interface by default. I only saw one example of an interface marked "Dev-Channel
Only" in ppapi/cpp/instance.h, and I suspect I need to create an overload of the
VideoDecoder::Initialize() function for that purpose, though that kind of
conflicts with the feedback I got from bbudge@ above in his first look.

  2) Update the Chrome version in ppb_video_decoder.idl for the new interface (I
kept the old M44, which is now stable. I'm  not sure if I can use 45, or if it
should be 46).

  3) Rebase to something closer to head (of course, but I wasn't sure how it
would impact the review to do so in the middle).

  4) Run the tryjobs again (just failed because the code is a bit too old).

Thanks!

[dcaiafa, 2015-08-11 17:37:12]

dcaiafa@chromium.org changed reviewers:
+ sergeyu@chromium.org

[dcaiafa, 2015-08-11 17:37:13]

+ sergeyu for remoting

[Sergey Ulanov, 2015-08-11 18:17:01]

https://codereview.chromium.org/1207043002/diff/110001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1207043002/diff/110001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:174: graphics_,
video_profile, PP_HARDWAREACCELERATION_WITHFALLBACK, 0,
What happens when a plugin calls GetPicture() while already holding certain
number previous frame? Can GetPicture() fail in that case because there are too
many frames allocated or will it delay completion until the previous frames are
returned?
Does it matter if the frames the plugin is holding are not sequential? I.e. if
there are 3 frames allocated total and the plugin holds frames N and N+2 can it
call GetPicture() to retrieve frame N+3 or does it have to return frame N first?

In either case, this class may call GetPicture() while holding previous 2
frames, so 0 is not correct here. It should be 3, if I understand the meaning
correctly.

[lpique, 2015-08-11 19:56:10]

https://codereview.chromium.org/1207043002/diff/110001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1207043002/diff/110001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:174: graphics_,
video_profile, PP_HARDWAREACCELERATION_WITHFALLBACK, 0,
On 2015/08/11 18:17:01, Sergey Ulanov wrote:
> What happens when a plugin calls GetPicture() while already holding certain
> number previous frame? Can GetPicture() fail in that case because there are
too
> many frames allocated or will it delay completion until the previous frames
are
> returned?

At present, the call to GetPictures() succeeds (PP_OK_COMPLETIONPENDING), but
the callback is not invoked until there are pictures available, which may or may
not happen.

I'm not sure returning an error immediately (or through the callback either) is
useful. The ARC (plugin) code can't force a release of pictures back to the
PPAPI, and all it would do is to retry the GetPictures() call again, and
busy-wait.

To correct that, I would add code that would force the same sort of wait in the
ARC driver, so that it only calls GetPicture() if it knows pictures are
available. But all that is doing is moving the wait.

> Does it matter if the frames the plugin is holding are not sequential? I.e. if
> there are 3 frames allocated total and the plugin holds frames N and N+2 can
it
> call GetPicture() to retrieve frame N+3 or does it have to return frame N
first?

The ARC code is written to handle decoded frames being returned in arbitrary
order.

> 
> In either case, this class may call GetPicture() while holding previous 2
> frames, so 0 is not correct here. It should be 3, if I understand the meaning
> correctly.

Ok, I will happily add a local constant declaring this part of the code requires
three.

I only used zero because it introduced the least amount of change. The
implementation takes this requested minimum and effectively does
"max(requested_by_client, required_by_decoder)", so the amount required by the
decoder would still be the count allocated. For the platforms I was testing ARC
on, I observed a picture pool size of five.

Done.

[Sergey Ulanov, 2015-08-11 23:10:20]

https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:175: kMinimumPictureCount,
Actually looking at the implementation it appears that this call will fail in
older versions of browser that don't support the new version of the interface.
So maybe detect here if the new interface is supported and pass 0 otherwise?

https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.h (right):

https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.h:100: static const uint32_t
kMinimumPictureCount = 3;
This doesn't need to be defined in the class. It can be just static const in the
.cc file.

[lpique, 2015-08-12 01:08:50]

https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
File remoting/client/plugin/pepper_video_renderer_3d.cc (right):

https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
remoting/client/plugin/pepper_video_renderer_3d.cc:175: kMinimumPictureCount,
On 2015/08/11 23:10:20, Sergey Ulanov wrote:
> Actually looking at the implementation it appears that this call will fail in
> older versions of browser that don't support the new version of the interface.
> So maybe detect here if the new interface is supported and pass 0 otherwise?

Yikes. I didn't even think about this being a plugin when responding to your
last feedback.

I'm going to have to deal with the fact that the new version of the API is
Dev-channel only as well. Perhaps it is better to use zero for now, and leave a
TODO behind to change it to three once the API is promoted out of dev.

[lpique, 2015-08-12 01:16:08]

On 2015/08/12 01:08:50, lpique wrote:
>
https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
> File remoting/client/plugin/pepper_video_renderer_3d.cc (right):
> 
>
https://codereview.chromium.org/1207043002/diff/130001/remoting/client/plugin...
> remoting/client/plugin/pepper_video_renderer_3d.cc:175: kMinimumPictureCount,
> On 2015/08/11 23:10:20, Sergey Ulanov wrote:
> > Actually looking at the implementation it appears that this call will fail
in
> > older versions of browser that don't support the new version of the
interface.
> > So maybe detect here if the new interface is supported and pass 0 otherwise?
> 
> Yikes. I didn't even think about this being a plugin when responding to your
> last feedback.
> 
> I'm going to have to deal with the fact that the new version of the API is
> Dev-channel only as well. Perhaps it is better to use zero for now, and leave
a
> TODO behind to change it to three once the API is promoted out of dev.

Okay, maybe its not that complicated, since I'm pretty sure the API would only
listed as supported on a dev-channel version of the browser anyway with my
patch. Just looking for support should be enough.

[Sergey Ulanov, 2015-08-12 17:14:52]

remoting lgtm

[bbudge, 2015-08-12 17:47:09]

Pepper lgtm w/nits

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1982: uint32_t /*min_picture_count */)
nit space after /*

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/video_deco...
File ppapi/proxy/video_decoder_resource.h (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource.h:179: static const uint32_t
kMaximumPictureCount = 100;
the constant can be defined in an anonymous namespace in the .cc file.

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
File ppapi/tests/test_video_decoder.cc (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
ppapi/tests/test_video_decoder.cc:12: const uint32_t kMaxRequestedPictureCount =
100;
This could break if the constant ever changes in the resource. Why not make it
much bigger?

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
ppapi/tests/test_video_decoder.cc:74: // picture buffers, if we can create a
Graphics3D resources and if we allow
nit s/resources/resource

[lpique, 2015-08-12 22:48:54]

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1982: uint32_t /*min_picture_count */)
On 2015/08/12 17:47:09, bbudge wrote:
> nit space after /*

Done.

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/video_deco...
File ppapi/proxy/video_decoder_resource.h (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource.h:179: static const uint32_t
kMaximumPictureCount = 100;
On 2015/08/12 17:47:09, bbudge wrote:
> the constant can be defined in an anonymous namespace in the .cc file.

Done.

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
File ppapi/tests/test_video_decoder.cc (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
ppapi/tests/test_video_decoder.cc:12: const uint32_t kMaxRequestedPictureCount =
100;
On 2015/08/12 17:47:09, bbudge wrote:
> This could break if the constant ever changes in the resource. Why not make it
> much bigger?

I wrote the tests to test the boundary case. Allocating the maximum allowed
should be possible, and allocating one more should fail. It seems like a
reasonable test to have.

What I would have like to have done was include
ppapi/proxy/video-decoder_resource.h, and use the "kMaximumPictureCount"
constant that was defined there. Unfortunately I ran into issues compiling the
code (the included headers required C++11 support when the tests are built
without the flag enabling it).

I considered defining an enum constant in the .idl, but that didn't seem like
the right thing to do. I also didn't observe any other place in the ppapi tree
where I could add a quick internal implementation constant/header file to be
shared between implementation and test.

The only other place that made sense was to add it to media/base/limits.h. But
then its a maximum that needs to be enforced in a few other places in the code,
and not just in the PPAPI host implementation. Also what would be the difference
between the new constant and the existing kMaxVideoFrames constant there?

For now I've left the value, but changed the name, and added comments both here
and in video_decoder_resource.cc that the constants should match. If someone
ignores the comment and changes one and not the other, the test will fail.

I'm open to suggestions for doing it in some other way.

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
ppapi/tests/test_video_decoder.cc:74: // picture buffers, if we can create a
Graphics3D resources and if we allow
On 2015/08/12 17:47:09, bbudge wrote:
> nit s/resources/resource

Done.

[bbudge, 2015-08-12 23:39:23]

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
File ppapi/tests/test_video_decoder.cc (right):

https://codereview.chromium.org/1207043002/diff/150001/ppapi/tests/test_video...
ppapi/tests/test_video_decoder.cc:12: const uint32_t kMaxRequestedPictureCount =
100;
On 2015/08/12 22:48:54, lpique wrote:
> On 2015/08/12 17:47:09, bbudge wrote:
> > This could break if the constant ever changes in the resource. Why not make
it
> > much bigger?
> 
> I wrote the tests to test the boundary case. Allocating the maximum allowed
> should be possible, and allocating one more should fail. It seems like a
> reasonable test to have.
> 
> What I would have like to have done was include
> ppapi/proxy/video-decoder_resource.h, and use the "kMaximumPictureCount"
> constant that was defined there. Unfortunately I ran into issues compiling the
> code (the included headers required C++11 support when the tests are built
> without the flag enabling it).
> 
> I considered defining an enum constant in the .idl, but that didn't seem like
> the right thing to do. I also didn't observe any other place in the ppapi tree
> where I could add a quick internal implementation constant/header file to be
> shared between implementation and test.
> 
> The only other place that made sense was to add it to media/base/limits.h. But
> then its a maximum that needs to be enforced in a few other places in the
code,
> and not just in the PPAPI host implementation. Also what would be the
difference
> between the new constant and the existing kMaxVideoFrames constant there?
> 
> For now I've left the value, but changed the name, and added comments both
here
> and in video_decoder_resource.cc that the constants should match. If someone
> ignores the comment and changes one and not the other, the test will fail.
> 
> I'm open to suggestions for doing it in some other way.

A comment to connect the two is fine. Thanks.

[lpique, 2015-08-13 01:10:20]

It looks like when I reran ppapi/generators/generator.py, many other files were
touched.

I also observed a presubmit warning:

  ** Presubmit Warnings **
  PPAPI_RELEASE has changed

  ***************
  #define PPAPI_RELEASE 46
  ***************

bbudge@: PTAAL.

[bbudge, 2015-08-13 01:23:00]

On 2015/08/13 01:10:20, lpique wrote:
> It looks like when I reran ppapi/generators/generator.py, many other files
were
> touched.
> 
> I also observed a presubmit warning:
> 
>   ** Presubmit Warnings **
>   PPAPI_RELEASE has changed
> 
>   ***************
>   #define PPAPI_RELEASE 46
>   ***************
> 
> bbudge@: PTAAL.

The generator really shouldn't touch all those files. Can we straighten this out
tomorrow?

[lpique, 2015-08-13 19:33:26]

On 2015/08/13 01:23:00, bbudge wrote:
> On 2015/08/13 01:10:20, lpique wrote:
> > It looks like when I reran ppapi/generators/generator.py, many other files
> were
> > touched.
> > 
> > I also observed a presubmit warning:
> > 
> >   ** Presubmit Warnings **
> >   PPAPI_RELEASE has changed
> > 
> >   ***************
> >   #define PPAPI_RELEASE 46
> >   ***************
> > 
> > bbudge@: PTAAL.
> 
> The generator really shouldn't touch all those files. Can we straighten this
out
> tomorrow?

Done in patch set 14 (with a minor exception in
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c where two comments
didn't drop out when I rebased on top of a clean regenerate patch locally).

As mentioned, 23eca7d introduced a clang-format change which causes the
generator to write a bunch of changed files.

[bbudge, 2015-08-13 20:53:39]

Still LGTM. You should also change
native_client_sdk/src/examples/api/video_decode as you did for ppapi/examples,
and get LGTM from binji or sbc

[lpique, 2015-08-13 22:29:08]

lpique@chromium.org changed reviewers:
+ sbc@google.com

[lpique, 2015-08-13 22:30:38]

On 2015/08/13 20:53:39, bbudge wrote:
> Still LGTM. You should also change
> native_client_sdk/src/examples/api/video_decode as you did for ppapi/examples,
> and get LGTM from binji or sbc

@sbc: ptal at the trivial change to
native_client_sdk/src/examples/api/video_decode (matches change made to
ppapi/examples/video_decode)

[Sam Clegg, 2015-08-13 22:36:15]

sbc@chromium.org changed reviewers:
+ sbc@chromium.org

[Sam Clegg, 2015-08-13 22:36:16]

lgtm

[lpique, 2015-08-14 18:45:34]

The CQ bit was checked by lpique@chromium.org

[lpique, 2015-08-14 18:45:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, sergeyu@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1207043002/#ps270001
(title: "Updated native_client_sdk/src/examples/api/video_decode to match")

[commit-bot: I haz the power, 2015-08-14 18:46:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207043002/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207043002/270001

[commit-bot: I haz the power, 2015-08-14 18:55:24]

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

[commit-bot: I haz the power, 2015-08-14 18:55:26]

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

[lpique, 2015-08-14 19:12:02]

On 2015/08/14 18:55:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

I learned I need to run "git cl presubmit" to make sure I have all the LGTM's I
need.

jln@ and jar@ -- PTAL.

[lpique, 2015-08-14 19:13:24]

On 2015/08/14 19:12:02, lpique wrote:
> On 2015/08/14 18:55:26, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> I learned I need to run "git cl presubmit" to make sure I have all the LGTM's
I
> need.
> 
> jln@ and jar@ -- PTAL.

Sorry, should have listed the files.

jln@ -- ppapi/proxy/ppapi_messages.h
jar@ -- tools/metrics/histograms/histograms.xml

PTAL.

[lpique, 2015-08-17 19:20:19]

lpique@chromium.org changed reviewers:
+ asvitkine@google.com
- jar@chromium.org

[lpique, 2015-08-17 19:23:18]

On 2015/08/14 19:13:24, lpique wrote:
> On 2015/08/14 19:12:02, lpique wrote:
> > On 2015/08/14 18:55:26, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > I learned I need to run "git cl presubmit" to make sure I have all the
LGTM's
> I
> > need.
> > 
> > jln@ and jar@ -- PTAL.
> 
> Sorry, should have listed the files.
> 
> jln@ -- ppapi/proxy/ppapi_messages.h
> jar@ -- tools/metrics/histograms/histograms.xml
> 
> PTAL.

s/jar/asvitkine since jar@ seems to be entirely off Chromium. (Should he be
removed from the tools/metrics/OWNERS file?)

asvitkine@, could you review my change to
tools/metrics/histograms/histograms.xml please?

[jln (very slow on Chromium), 2015-08-17 21:44:14]

ppapi/proxy/video_decoder_resource.h mostly 'rubberstamp lgtm', but please be
paranoid and at the very least check that the two values that must match
actually match in a test.

Be very careful about an attacker messing with |min_picture_count| and what this
could mean for security.

[jln (very slow on Chromium), 2015-08-17 21:44:39]

(forgot the comments)

https://chromiumcodereview.appspot.com/1207043002/diff/270001/ppapi/proxy/vid...
File ppapi/proxy/video_decoder_resource.cc (right):

https://chromiumcodereview.appspot.com/1207043002/diff/270001/ppapi/proxy/vid...
ppapi/proxy/video_decoder_resource.cc:37: const uint32_t kMaximumPictureCount =
100;
What happens if they don't match?

Could you expose a GetMaximumPictureCount() so that you can test that they match
at least?

https://chromiumcodereview.appspot.com/1207043002/diff/270001/ppapi/proxy/vid...
ppapi/proxy/video_decoder_resource.cc:390: DCHECK(num_textures >=
min_picture_count_);
Is this correct as a DCHECK? What happens if an attacker sends a malicious
min_picture_count?

[lpique, 2015-08-17 23:57:07]

Thanks for your review.

https://chromiumcodereview.appspot.com/1207043002/diff/270001/ppapi/proxy/vid...
File ppapi/proxy/video_decoder_resource.cc (right):

https://chromiumcodereview.appspot.com/1207043002/diff/270001/ppapi/proxy/vid...
ppapi/proxy/video_decoder_resource.cc:37: const uint32_t kMaximumPictureCount =
100;
On 2015/08/17 21:44:39, jln (slow on Chromium) wrote:
> What happens if they don't match?

The test in test_video_decoder.cc will fail, since it tries to use a count of
100, expecting it to pass, and a count of 101 expecting it to fail.

> Could you expose a GetMaximumPictureCount() so that you can test that they
match
> at least?

This question gave me the most pause.

Yes I could use this function to elimiante the constant from the test (it would
just use the returned value, and try calling Initialize() with the returned
count and one more than the returned count).

But it is not really useful otherwise. The limit of 100 I chose is purely
arbitrary. For the purposes of my work on ARC (a client of this API) I only
actually need a minimum picture count of 8. I chose a larger  value in case that
changes, but 100 is pretty extreme.

For video decoding, a normal client will want to minimize this value. Large
values represent more memory being allocated, as well as more latency (ARC, or
really the Android code it uses has additional logic to sync things up despite
having a larger decode queue)

A normal client would have some simple known fixed constant need, and I think it
is perfectly fine if it tries to call Initialize() with this value, and just
asserts that it returns PP_OK. Being able to call GetMaximumPictureCount() might
be useful for displaying an error message, but its not really useful otherwise.
If the limit is too small, the only way to increase it is to rebuild Chrome with
a larger value.

https://chromiumcodereview.appspot.com/1207043002/diff/270001/ppapi/proxy/vid...
ppapi/proxy/video_decoder_resource.cc:390: DCHECK(num_textures >=
min_picture_count_);
On 2015/08/17 21:44:39, jln (slow on Chromium) wrote:
> Is this correct as a DCHECK?

I think so. With the rest of my changes, this should never be trigger, as later
code ensures that num_textures >= min_picture_count_.

I left this here in case some introduces a new code path that does not respect
the minimum requested, in which case the test for allocating the maximum allowed
minimum picture count should trigger the DCHECK (unless I am mistaken about the
DCHECK's being enabled for the test, which is possible)

If this DCHECK is compiled out, and new code is written in the host that does
not honor the requested minimum picture count (num_textures <
min_picture_count), the worst that happens is that the client may try to
allocate a larger number of textures than the interface can provide, and the
client ends up waiting on the callback from GetPicture() to complete.

> What happens if an attacker sends a malicious min_picture_count?

The check I added at line 137 is supposed to restrict the set of values to a
reasonable range.

Without that check, the client can request say MAXINT, causing the host to try
to allocate memory for MAXINT picture frames. That should fail due to running
out of memory, but I certainly didn't expect the host code to handle it
correctly.

However looking back on it now, I'm fairly certain that I need to move the
validation to pepper_video_decoder_host.cc, as I think the client could bypass
the check here. (I saw the other validation here, and so I added it here.)

[bbudge, 2015-08-18 00:48:13]

https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
File ppapi/proxy/video_decoder_resource.cc (right):

https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource.cc:390: DCHECK(num_textures >=
min_picture_count_);
On 2015/08/17 23:57:07, lpique wrote:
> On 2015/08/17 21:44:39, jln (slow on Chromium) wrote:
> > Is this correct as a DCHECK?
> 
> I think so. With the rest of my changes, this should never be trigger, as
later
> code ensures that num_textures >= min_picture_count_.
> 
> I left this here in case some introduces a new code path that does not respect
> the minimum requested, in which case the test for allocating the maximum
allowed
> minimum picture count should trigger the DCHECK (unless I am mistaken about
the
> DCHECK's being enabled for the test, which is possible)
> 
> If this DCHECK is compiled out, and new code is written in the host that does
> not honor the requested minimum picture count (num_textures <
> min_picture_count), the worst that happens is that the client may try to
> allocate a larger number of textures than the interface can provide, and the
> client ends up waiting on the callback from GetPicture() to complete.
> 
> > What happens if an attacker sends a malicious min_picture_count?
> 
> The check I added at line 137 is supposed to restrict the set of values to a
> reasonable range.
> 
> Without that check, the client can request say MAXINT, causing the host to try
> to allocate memory for MAXINT picture frames. That should fail due to running
> out of memory, but I certainly didn't expect the host code to handle it
> correctly.
> 
> However looking back on it now, I'm fairly certain that I need to move the
> validation to pepper_video_decoder_host.cc, as I think the client could bypass
> the check here. (I saw the other validation here, and so I added it here.)

You definitely have to validate anything you receive from the plugin in the
renderer process. Any validation done in the resource (plugin process) is purely
for convenience and can easily be bypassed by an attacker. Good catch jln.

[lpique, 2015-08-18 00:55:00]

On 2015/08/18 00:48:13, bbudge wrote:
>
https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
> File ppapi/proxy/video_decoder_resource.cc (right):
> 
>
https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
> ppapi/proxy/video_decoder_resource.cc:390: DCHECK(num_textures >=
> min_picture_count_);
> On 2015/08/17 23:57:07, lpique wrote:
> > On 2015/08/17 21:44:39, jln (slow on Chromium) wrote:
> > > Is this correct as a DCHECK?
> > 
> > I think so. With the rest of my changes, this should never be trigger, as
> later
> > code ensures that num_textures >= min_picture_count_.
> > 
> > I left this here in case some introduces a new code path that does not
respect
> > the minimum requested, in which case the test for allocating the maximum
> allowed
> > minimum picture count should trigger the DCHECK (unless I am mistaken about
> the
> > DCHECK's being enabled for the test, which is possible)
> > 
> > If this DCHECK is compiled out, and new code is written in the host that
does
> > not honor the requested minimum picture count (num_textures <
> > min_picture_count), the worst that happens is that the client may try to
> > allocate a larger number of textures than the interface can provide, and the
> > client ends up waiting on the callback from GetPicture() to complete.
> > 
> > > What happens if an attacker sends a malicious min_picture_count?
> > 
> > The check I added at line 137 is supposed to restrict the set of values to a
> > reasonable range.
> > 
> > Without that check, the client can request say MAXINT, causing the host to
try
> > to allocate memory for MAXINT picture frames. That should fail due to
running
> > out of memory, but I certainly didn't expect the host code to handle it
> > correctly.
> > 
> > However looking back on it now, I'm fairly certain that I need to move the
> > validation to pepper_video_decoder_host.cc, as I think the client could
bypass
> > the check here. (I saw the other validation here, and so I added it here.)
> 
> You definitely have to validate anything you receive from the plugin in the
> renderer process. Any validation done in the resource (plugin process) is
purely
> for convenience and can easily be bypassed by an attacker. Good catch jln.

Moved in patchset 16.

[lpique, 2015-08-18 18:43:43]

lpique@chromium.org changed reviewers:
+ jwd@google.com
- asvitkine@google.com

[bbudge, 2015-08-19 18:47:06]

Still LGTM with a request to move the constant definition.

https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
File ppapi/proxy/video_decoder_resource.cc (right):

https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource.cc:137: if (min_picture_count >
kMaximumPictureCount)
You could keep this if you move the constant definition. See my comment on the
host.

https://codereview.chromium.org/1207043002/diff/270001/ppapi/proxy/video_deco...
ppapi/proxy/video_decoder_resource.cc:390: DCHECK(num_textures >=
min_picture_count_);
On 2015/08/18 00:48:13, bbudge wrote:
> On 2015/08/17 23:57:07, lpique wrote:
> > On 2015/08/17 21:44:39, jln (slow on Chromium) wrote:
> > > Is this correct as a DCHECK?
> > 
> > I think so. With the rest of my changes, this should never be trigger, as
> later
> > code ensures that num_textures >= min_picture_count_.
> > 
> > I left this here in case some introduces a new code path that does not
respect
> > the minimum requested, in which case the test for allocating the maximum
> allowed
> > minimum picture count should trigger the DCHECK (unless I am mistaken about
> the
> > DCHECK's being enabled for the test, which is possible)
> > 
> > If this DCHECK is compiled out, and new code is written in the host that
does
> > not honor the requested minimum picture count (num_textures <
> > min_picture_count), the worst that happens is that the client may try to
> > allocate a larger number of textures than the interface can provide, and the
> > client ends up waiting on the callback from GetPicture() to complete.
> > 
> > > What happens if an attacker sends a malicious min_picture_count?
> > 
> > The check I added at line 137 is supposed to restrict the set of values to a
> > reasonable range.
> > 
> > Without that check, the client can request say MAXINT, causing the host to
try
> > to allocate memory for MAXINT picture frames. That should fail due to
running
> > out of memory, but I certainly didn't expect the host code to handle it
> > correctly.
> > 
> > However looking back on it now, I'm fairly certain that I need to move the
> > validation to pepper_video_decoder_host.cc, as I think the client could
bypass
> > the check here. (I saw the other validation here, and so I added it here.)
> 
> You definitely have to validate anything you receive from the plugin in the
> renderer process. Any validation done in the resource (plugin process) is
purely
> for convenience and can easily be bypassed by an attacker. Good catch jln.

The DCHECKs here are OK for validating proxy correctness. They aren't a security
check, since this is a message from a more trusted (renderer) process.
'min_picture_count_' is completely under the control of the untrusted process.

https://codereview.chromium.org/1207043002/diff/290001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_decoder_host.cc (right):

https://codereview.chromium.org/1207043002/diff/290001/content/renderer/peppe...
content/renderer/pepper/pepper_video_decoder_host.cc:40: const uint32_t
kMaximumPictureCount = 100;
I forgot that we had a place for constants like this. Could you move this to
ppapi/proxy/video_decoder_constants.h ? That would allow you to share the
definition with the proxy (resource) if you wanted to validate there too.

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...

[lpique, 2015-08-19 19:42:16]

lpique@chromium.org changed required reviewers:
+ bbudge@chromium.org, jwd@google.com

[lpique, 2015-08-19 19:42:17]

Thanks.

https://codereview.chromium.org/1207043002/diff/290001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_decoder_host.cc (right):

https://codereview.chromium.org/1207043002/diff/290001/content/renderer/peppe...
content/renderer/pepper/pepper_video_decoder_host.cc:40: const uint32_t
kMaximumPictureCount = 100;
On 2015/08/19 18:47:06, bbudge wrote:
> I forgot that we had a place for constants like this. Could you move this to
> ppapi/proxy/video_decoder_constants.h ? That would allow you to share the
> definition with the proxy (resource) if you wanted to validate there too.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...

Sure. I saw it when looking for a location to share the constant with the test.
I've moved it there, and added back the validation in the resource proxy.

[jar (doing other things), 2015-08-19 20:58:58]

jar@chromium.org changed reviewers:
+ jar@chromium.org

[jar (doing other things), 2015-08-19 20:58:59]

One line change in histograms.xml LGTM

[lpique, 2015-08-19 22:13:35]

The CQ bit was checked by lpique@chromium.org

[lpique, 2015-08-19 22:13:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org, sergeyu@chromium.org, jln@chromium.org, sbc@chromium.org,
bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1207043002/#ps310001
(title: "Moved constant to shared header, validate min_picture_size now in
resource proxy as well as host co…")

[commit-bot: I haz the power, 2015-08-19 22:14:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207043002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207043002/310001

[commit-bot: I haz the power, 2015-08-19 22:14:19]

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

[commit-bot: I haz the power, 2015-08-19 22:14:20]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[lpique, 2015-08-19 22:22:03]

lpique@chromium.org changed required reviewers:
- bbudge@chromium.org, jwd@google.com

[lpique, 2015-08-19 22:23:26]

The CQ bit was checked by lpique@chromium.org

[commit-bot: I haz the power, 2015-08-19 22:24:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207043002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1207043002/310001

[commit-bot: I haz the power, 2015-08-20 01:07:02]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2015-08-20 01:07:33]

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