Pass decoded picture size from VDA to client

Pass decoded picture size from VDA to client

Some of the VDAs, like DXVA and AVDA, don't distinguish
between visible size and coded size. And given GVD is always
been used with container, we should keep using size from
config in GVD.

BUG=390048
TEST=Manually tested: flash player using youtube. html5
player with resolution 1280x720 and 1216x684, h264 and vp8.
apprtc with resolution 640x480 and 636x476. All tests are
performed on both daisy and link.
R=hshi@chromium.org, posciak@chromium.org

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

[kcwu, 2014-07-29 14:27:08]

PTAL.

Notes
1. I cannot reproduce crbug/360262 #31, thus I don't know it works or not.

2. I don't know how to handle mid stream resize on Android. See my TODO comment
in rtc_video_decoder.cc

3. this patch includes partial change of rtc_video_decoder.cc from
https://codereview.chromium.org/304593004/
This make testing easier for me. I will remove them later.

[Pawel Osciak, 2014-07-31 11:50:26]

Initial comment before I go more in depth on this review: please use gfx::Size
instead of width,height pair everywhere. gfx::Size is already serializable and
can be passed through IPC as well.

[kcwu, 2014-08-05 12:59:02]

On 2014/07/31 11:50:26, Pawel Osciak wrote:
> Initial comment before I go more in depth on this review: please use gfx::Size
> instead of width,height pair everywhere. gfx::Size is already serializable and
> can be passed through IPC as well.

Done. use gfx::Size instead.

[hshi1, 2014-08-08 17:42:00]

LGTM. Can you make this into M38? Thanks

[Pawel Osciak, 2014-08-10 00:02:22]

Does this compile? You did not change PictureReady() in VideoDecodeAccelerator
base class.

Also, please run git cl format. There are multiple formatting errors in this CL.

Please ensure this is tested on H264 and VP8 in both RTC (RtcVideoDecoder) and
video playback (GpuVideoDecoder) scenarios and with resolutions that are both
macroblock-aligned and not, and verify the output. Also on at least one x86 and
ARM platform (VAVDA and V4L2VDA). Please provide a TEST= line and describe how
this change was tested.

https://chromiumcodereview.appspot.com/426873004/diff/20001/content/common/gp...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/426873004/diff/20001/content/common/gp...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:377:
tfp_picture->size()));
Please file a bug for this and add:
TODO(posciak): Use visible size from decoder here instead (crbug.com/xxx).

https://chromiumcodereview.appspot.com/426873004/diff/20001/content/renderer/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/426873004/diff/20001/content/renderer/...
content/renderer/media/rtc_video_decoder.cc:208: // encoded size is unknown??
s/??/?/

Not sure what you mean here, VDA will always return size on PictureReady, be it
visible or (wrongly) coded size. Can you explain what you have in mind here
please?

https://chromiumcodereview.appspot.com/426873004/diff/20001/content/renderer/...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://chromiumcodereview.appspot.com/426873004/diff/20001/content/renderer/...
content/renderer/pepper/video_decoder_shim.cc:531: texture_size_));
We should be passing visible size here, not coded size. You should be able to
get visible size from VideoFrame that comes into OnOutputComplete.

https://chromiumcodereview.appspot.com/426873004/diff/20001/media/filters/gpu...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/426873004/diff/20001/media/filters/gpu...
media/filters/gpu_video_decoder.cc:420: gfx::Rect visible_rect(picture.size());
Please do not make any changes to this class. Unfortunately some of the VDAs,
notably DXVA and AVDA, don't distinguish between visible size and coded size, so
this would result in us using coded size for them here, which is not what we
want. We should keep using config information.

The only advantage of using the VDA-provided size here over the config size
would be if we were decoding a resolution changing stream without a container
(or with a broken container), but the former does not happen when we use this
class anyway, and the latter we can't help given the above.

Please only document that we are dropping picture.size() for the above reasons:
"Some of the VDAs don't support and thus don't provide us with visible size,
passing coded size instead, so drop this and use config information instead."

https://chromiumcodereview.appspot.com/426873004/diff/20001/media/video/pictu...
File media/video/picture.h (right):

https://chromiumcodereview.appspot.com/426873004/diff/20001/media/video/pictu...
media/video/picture.h:74: // Returns the size of the picture.
Please document that this size may be smaller than the size of the
PictureBuffer, i.e. that it is visible size of the Picture contained in the
PictureBuffer.

[Pawel Osciak, 2014-08-10 00:04:50]

Oh, forgot to add, please verify with both flash players and HTML5 player in the
scenarios I mentioned. And also please update the CL description with why we are
doing this, why in RTCVD and why not in GVD.
Thanks.

[hshi1, 2014-08-10 00:44:30]

On 2014/08/10 00:02:22, Pawel Osciak wrote:
> Does this compile? You did not change PictureReady() in VideoDecodeAccelerator
> base class.

It does compile for daisy (in fact I was testing end-to-end H264 with this CL).

[Pawel Osciak, 2014-08-10 00:58:27]

On 2014/08/10 00:44:30, hshi1 wrote:
> On 2014/08/10 00:02:22, Pawel Osciak wrote:
> > Does this compile? You did not change PictureReady() in
VideoDecodeAccelerator
> > base class.
> 
> It does compile for daisy (in fact I was testing end-to-end H264 with this
CL).

Right, sorry, my bad, Client::PictureReady still takes Picture, it's just the
message that got changed.

[kcwu, 2014-08-12 03:50:47]

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:208: // encoded size is unknown??
On 2014/08/10 00:02:21, Pawel Osciak wrote:
> s/??/?/
> 
> Not sure what you mean here, VDA will always return size on PictureReady, be
it
> visible or (wrongly) coded size. Can you explain what you have in mind here
> please?

1. Should I still use client provided size if available? 
2. According to line 200 and 214, on Android, it doesn't support mid stream
resize. So it relays on client "encoded size" to handle midstream resize. This
behavior will not work if encoded size is unknown.

[Pawel Osciak, 2014-08-12 04:05:07]

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:208: // encoded size is unknown??
On 2014/08/12 03:50:47, kcwu wrote:
> On 2014/08/10 00:02:21, Pawel Osciak wrote:
> > s/??/?/
> > 
> > Not sure what you mean here, VDA will always return size on PictureReady, be
> it
> > visible or (wrongly) coded size. Can you explain what you have in mind here
> > please?
> 
> 1. Should I still use client provided size if available? 

Given that some vdas can't provide visible size, I'd say yes, it should take
priority.

> 2. According to line 200 and 214, on Android, it doesn't support mid stream
> resize. So it relays on client "encoded size" to handle midstream resize. This
> behavior will not work if encoded size is unknown.

I think this patch doesn't change this though, because if we use the size
returned from vda on PictureReady instead, it would be too late anyway if it
couldn't handle the resize?

[kcwu, 2014-08-12 04:48:06]

https://codereview.chromium.org/426873004/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:377:
tfp_picture->size()));
On 2014/08/10 00:02:21, Pawel Osciak wrote:
> Please file a bug for this and add:
> TODO(posciak): Use visible size from decoder here instead (crbug.com/xxx).

Done.

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:208: // encoded size is unknown??
On 2014/08/12 04:05:07, Pawel Osciak wrote:
> On 2014/08/12 03:50:47, kcwu wrote:
> > 2. According to line 200 and 214, on Android, it doesn't support mid stream
> > resize. So it relays on client "encoded size" to handle midstream resize.
This
> > behavior will not work if encoded size is unknown.
> 
> I think this patch doesn't change this though, because if we use the size
> returned from vda on PictureReady instead, it would be too late anyway if it
> couldn't handle the resize?

Okay, then should I leave comment here to indicate this problem? (rephrased)

https://codereview.chromium.org/426873004/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_decoder_shim.cc:531: texture_size_));
On 2014/08/10 00:02:21, Pawel Osciak wrote:
> We should be passing visible size here, not coded size. You should be able to
> get visible size from VideoFrame that comes into OnOutputComplete.

I don't understand what you mean. texture_size_ here comes from
PendingFrame->size in OnOutputComplete (line 498). There is only one size in
PendingFrame.

https://codereview.chromium.org/426873004/diff/20001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/20001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:420: gfx::Rect visible_rect(picture.size());
On 2014/08/10 00:02:22, Pawel Osciak wrote:
> Please do not make any changes to this class. Unfortunately some of the VDAs,
> notably DXVA and AVDA, don't distinguish between visible size and coded size,
so
> this would result in us using coded size for them here, which is not what we
> want. We should keep using config information.
> 
> The only advantage of using the VDA-provided size here over the config size
> would be if we were decoding a resolution changing stream without a container
> (or with a broken container), but the former does not happen when we use this
> class anyway, and the latter we can't help given the above.
> 
> Please only document that we are dropping picture.size() for the above
reasons:
> "Some of the VDAs don't support and thus don't provide us with visible size,
> passing coded size instead, so drop this and use config information instead."

Done.

https://codereview.chromium.org/426873004/diff/20001/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/426873004/diff/20001/media/video/picture.h#ne...
media/video/picture.h:74: // Returns the size of the picture.
On 2014/08/10 00:02:22, Pawel Osciak wrote:
> Please document that this size may be smaller than the size of the
> PictureBuffer, i.e. that it is visible size of the Picture contained in the
> PictureBuffer.

Done.

[Pawel Osciak, 2014-08-12 04:59:24]

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:208: // encoded size is unknown??
On 2014/08/12 04:48:06, kcwu wrote:
> On 2014/08/12 04:05:07, Pawel Osciak wrote:
> > On 2014/08/12 03:50:47, kcwu wrote:
> > > 2. According to line 200 and 214, on Android, it doesn't support mid
stream
> > > resize. So it relays on client "encoded size" to handle midstream resize.
> This
> > > behavior will not work if encoded size is unknown.
> > 
> > I think this patch doesn't change this though, because if we use the size
> > returned from vda on PictureReady instead, it would be too late anyway if it
> > couldn't handle the resize?
> 
> Okay, then should I leave comment here to indicate this problem? (rephrased)

Yes please.

https://codereview.chromium.org/426873004/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_decoder_shim.cc:531: texture_size_));
On 2014/08/12 04:48:06, kcwu wrote:
> On 2014/08/10 00:02:21, Pawel Osciak wrote:
> > We should be passing visible size here, not coded size. You should be able
to
> > get visible size from VideoFrame that comes into OnOutputComplete.
> 
> I don't understand what you mean. texture_size_ here comes from
> PendingFrame->size in OnOutputComplete (line 498). There is only one size in
> PendingFrame.

OnOutputComplete gets a VF, which has both coded and visible size. We only use
coded size from it. We should keep using coded size for texture/buffer size, but
pass visible size to PictureReady.

[kcwu, 2014-08-12 07:31:12]

Addressed all comments. Ran git cl format as well.
I will do more testing now.

https://codereview.chromium.org/426873004/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/20001/content/renderer/pepper/...
content/renderer/pepper/video_decoder_shim.cc:531: texture_size_));
On 2014/08/12 04:59:24, Pawel Osciak wrote:
> On 2014/08/12 04:48:06, kcwu wrote:
> > On 2014/08/10 00:02:21, Pawel Osciak wrote:
> > > We should be passing visible size here, not coded size. You should be able
> to
> > > get visible size from VideoFrame that comes into OnOutputComplete.
> > 
> > I don't understand what you mean. texture_size_ here comes from
> > PendingFrame->size in OnOutputComplete (line 498). There is only one size in
> > PendingFrame.
> 
> OnOutputComplete gets a VF, which has both coded and visible size. We only use
> coded size from it. We should keep using coded size for texture/buffer size,
but
> pass visible size to PictureReady.

Ah, I was confused with VideoDecoderShim::DecoderImpl::OnOutputComplete and
VideoDecoderShim::OnOutputComplete.
Anyway, done.

[Pawel Osciak, 2014-08-12 08:50:41]

Please also update CL message as per my previous comment. Thanks.

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:199: // Note this may not work
because encoded size is not always available.
Please actually explain why and what the consequences are :)

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:369: CreateVideoFrame(picture, pb,
timestamp, size);
size is always zero... Looks like this variable was never used. Please remove it
from everywhere else.

https://codereview.chromium.org/426873004/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/80001/content/renderer/pepper/...
content/renderer/pepper/video_decoder_shim.cc:57: const gfx::Rect rect;
Please give more informative names. coded_size, visible_rect.

https://codereview.chromium.org/426873004/diff/80001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/80001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:428: // size, passing coded size instead, so
drop picture.size() and use config
s/provide us with visible size/provide us with visible size in picture.size/

s/drop picture.size()/drop it/

https://codereview.chromium.org/426873004/diff/80001/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/426873004/diff/80001/media/video/picture.h#ne...
media/video/picture.h:76: // size of the PictureBuffer, i.e. that it is visible
size of the
s/i.e. that it is/as it is the/

[kcwu, 2014-08-13 14:27:20]

Tested many scenarios, all work well.

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:199: // Note this may not work
because encoded size is not always available.
On 2014/08/12 08:50:40, Pawel Osciak wrote:
> Please actually explain why and what the consequences are :)

I don't know what to say here. Could you advise?

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:369: CreateVideoFrame(picture, pb,
timestamp, size);
On 2014/08/12 08:50:40, Pawel Osciak wrote:
> size is always zero... Looks like this variable was never used. Please remove
it
> from everywhere else.

Done.

https://codereview.chromium.org/426873004/diff/80001/content/renderer/pepper/...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/80001/content/renderer/pepper/...
content/renderer/pepper/video_decoder_shim.cc:57: const gfx::Rect rect;
On 2014/08/12 08:50:41, Pawel Osciak wrote:
> Please give more informative names. coded_size, visible_rect.

Done.

https://codereview.chromium.org/426873004/diff/80001/media/filters/gpu_video_...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/80001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:428: // size, passing coded size instead, so
drop picture.size() and use config
On 2014/08/12 08:50:41, Pawel Osciak wrote:
> s/provide us with visible size/provide us with visible size in picture.size/
> 
> s/drop picture.size()/drop it/

Done.

https://codereview.chromium.org/426873004/diff/80001/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/426873004/diff/80001/media/video/picture.h#ne...
media/video/picture.h:76: // size of the PictureBuffer, i.e. that it is visible
size of the
On 2014/08/12 08:50:41, Pawel Osciak wrote:
> s/i.e. that it is/as it is the/

Done.

[Pawel Osciak, 2014-08-14 07:15:36]

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:199: // Note this may not work
because encoded size is not always available.
On 2014/08/13 14:27:19, kcwu wrote:
> On 2014/08/12 08:50:40, Pawel Osciak wrote:
> > Please actually explain why and what the consequences are :)
> 
> I don't know what to say here. Could you advise?

Please say when encoded size is not available and what will happen in that case.

https://codereview.chromium.org/426873004/diff/100001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/426873004/diff/100001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.h:94: size_t size);
Unused.

https://codereview.chromium.org/426873004/diff/100001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.h:99: size_t size;  // buffer size
Unused.

https://codereview.chromium.org/426873004/diff/100001/content/renderer/pepper...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/100001/content/renderer/pepper...
content/renderer/pepper/video_decoder_shim.cc:51: const gfx::Size& size,
s/size/coded_size/

https://codereview.chromium.org/426873004/diff/100001/content/renderer/pepper...
content/renderer/pepper/video_decoder_shim.cc:52: const gfx::Rect& rect);
s/rect/visible_rect/

[kcwu, 2014-08-14 12:31:02]

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/80001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:199: // Note this may not work
because encoded size is not always available.
On 2014/08/14 07:15:36, Pawel Osciak wrote:
> On 2014/08/13 14:27:19, kcwu wrote:
> > On 2014/08/12 08:50:40, Pawel Osciak wrote:
> > > Please actually explain why and what the consequences are :)
> > 
> > I don't know what to say here. Could you advise?
> 
> Please say when encoded size is not available and what will happen in that
case.

per offline chat, the original code already silently handled. Revert my change.

https://codereview.chromium.org/426873004/diff/100001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/426873004/diff/100001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.h:94: size_t size);
On 2014/08/14 07:15:36, Pawel Osciak wrote:
> Unused.

Acknowledged.

https://codereview.chromium.org/426873004/diff/100001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.h:99: size_t size;  // buffer size
On 2014/08/14 07:15:36, Pawel Osciak wrote:
> Unused.

It is used in other case, rtc_video_decoder.cc:528

https://codereview.chromium.org/426873004/diff/100001/content/renderer/pepper...
File content/renderer/pepper/video_decoder_shim.cc (right):

https://codereview.chromium.org/426873004/diff/100001/content/renderer/pepper...
content/renderer/pepper/video_decoder_shim.cc:51: const gfx::Size& size,
On 2014/08/14 07:15:36, Pawel Osciak wrote:
> s/size/coded_size/

Done.

https://codereview.chromium.org/426873004/diff/100001/content/renderer/pepper...
content/renderer/pepper/video_decoder_shim.cc:52: const gfx::Rect& rect);
On 2014/08/14 07:15:36, Pawel Osciak wrote:
> s/rect/visible_rect/

Done.

[Pawel Osciak, 2014-08-15 04:03:29]

lgtm

[kcwu, 2014-08-15 04:16:53]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2014-08-15 04:20:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kcwu@chromium.org/426873004/120001

[kcwu, 2014-08-15 04:22:32]

The CQ bit was unchecked by kcwu@chromium.org

[kcwu, 2014-08-15 07:37:13]

add owner reviewers.
@jln, security review content/common/gpu/gpu_messages.h
@teravest: pepper file: content/renderer/pepper/video_decoder_shim.cc
@kbr:  gpu/*
@ddorwin: *accelerator* and other files not mentioned

[jln (very slow on Chromium), 2014-08-15 18:07:08]

https://codereview.chromium.org/426873004/diff/140001/media/video/picture.cc
File media/video/picture.cc (right):

https://codereview.chromium.org/426873004/diff/140001/media/video/picture.cc#...
media/video/picture.cc:30: size_(size) {
What happens here if an attacker supplies a bogus size via the IPC?

How could we make sure that this size is safe to use? Could we check that it is
within the bounds of the image? (We should do this as close to receiving the IPC
message as possible).

[Ken Russell (switch to Gerrit), 2014-08-15 22:59:26]

gpu/ portions LGTM assuming jln's security questions are addressed. One
question.

https://codereview.chromium.org/426873004/diff/140001/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/426873004/diff/140001/media/video/picture.h#n...
media/video/picture.h:77: // Picture contained in the PictureBuffer.
It's probably worth documenting how the picture is aligned within the
PictureBuffer if it's smaller than it -- i.e., upper left or lower left.

[Pawel Osciak, 2014-08-16 23:02:24]

On 2014/08/15 18:07:08, jln wrote:
> https://codereview.chromium.org/426873004/diff/140001/media/video/picture.cc
> File media/video/picture.cc (right):
> 
>
https://codereview.chromium.org/426873004/diff/140001/media/video/picture.cc#...
> media/video/picture.cc:30: size_(size) {
> What happens here if an attacker supplies a bogus size via the IPC?
> 
> How could we make sure that this size is safe to use? Could we check that it
is
> within the bounds of the image? (We should do this as close to receiving the
IPC
> message as possible).

The size comes from the more trusted process (GPU), but checking that it is
within PictureBuffer size is a good idea.

[kcwu, 2014-08-18 07:01:54]

On 2014/08/16 23:02:24, Pawel Osciak wrote:
> On 2014/08/15 18:07:08, jln wrote:
> > https://codereview.chromium.org/426873004/diff/140001/media/video/picture.cc
> > File media/video/picture.cc (right):
> > 
> >
>
https://codereview.chromium.org/426873004/diff/140001/media/video/picture.cc#...
> > media/video/picture.cc:30: size_(size) {
> > What happens here if an attacker supplies a bogus size via the IPC?
> > 
> > How could we make sure that this size is safe to use? Could we check that it
> is
> > within the bounds of the image? (We should do this as close to receiving the
> IPC
> > message as possible).
> 
> The size comes from the more trusted process (GPU), but checking that it is
> within PictureBuffer size is a good idea.

Yeah, the size comes from more trusted process (GPU) to less trusted process
(renderer).
I validated it in rtc_video_decoder.cc. I didn't do so for other clients
(GpuVideoDecoder, PPB_VideoDecoder_Impl and PepperVideoDecoderHost) since they
don't use the size.

[kcwu, 2014-08-18 07:02:04]

https://codereview.chromium.org/426873004/diff/140001/media/video/picture.h
File media/video/picture.h (right):

https://codereview.chromium.org/426873004/diff/140001/media/video/picture.h#n...
media/video/picture.h:77: // Picture contained in the PictureBuffer.
On 2014/08/15 22:59:26, Ken Russell wrote:
> It's probably worth documenting how the picture is aligned within the
> PictureBuffer if it's smaller than it -- i.e., upper left or lower left.

Done.

[Pawel Osciak, 2014-08-18 07:13:30]

We should verify in GVD as well. I guess we can't do that in ppapi client,
because it's the client of the plugin that knows about PB dimensions?

https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:363: bool valid_picture_size = (0 <=
picture.size().width() &&
You could use Rect::Contains() and Size::IsEmpty():

if (!picture.size().IsEmpty() &&
    gfx::Rect(pb.size()).Contains(gfx::Rect(picture.size())))

[kcwu, 2014-08-18 07:28:48]

https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:363: bool valid_picture_size = (0 <=
picture.size().width() &&
On 2014/08/18 07:13:30, Pawel Osciak wrote:
> You could use Rect::Contains() and Size::IsEmpty():
> 
> if (!picture.size().IsEmpty() &&
>     gfx::Rect(pb.size()).Contains(gfx::Rect(picture.size())))

I found Contains() cannot catch negative sizes.

[Pawel Osciak, 2014-08-18 07:38:25]

On 2014/08/18 07:28:48, kcwu wrote:
>
https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
> File content/renderer/media/rtc_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
> content/renderer/media/rtc_video_decoder.cc:363: bool valid_picture_size = (0
<=
> picture.size().width() &&
> On 2014/08/18 07:13:30, Pawel Osciak wrote:
> > You could use Rect::Contains() and Size::IsEmpty():
> > 
> > if (!picture.size().IsEmpty() &&
> >     gfx::Rect(pb.size()).Contains(gfx::Rect(picture.size())))
> 
> I found Contains() cannot catch negative sizes.

The constructor of SizeBase will not allow negative values and set 0 instead:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/geometry/si...

[kcwu, 2014-08-18 07:52:39]

Thanks. All comments are addressed. PTAL

https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/160001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:363: bool valid_picture_size = (0 <=
picture.size().width() &&
On 2014/08/18 07:13:30, Pawel Osciak wrote:
> You could use Rect::Contains() and Size::IsEmpty():
> 
> if (!picture.size().IsEmpty() &&
>     gfx::Rect(pb.size()).Contains(gfx::Rect(picture.size())))

Done.

[Pawel Osciak, 2014-08-18 07:59:03]

looks good, did you sanity test?

https://codereview.chromium.org/426873004/diff/200001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/200001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:367: << " from GPU";
Perhaps:

"Invalid picture size from VDA: " << pb.size().ToString() << " should fit in "
<< picture.size().ToString()

[kcwu, 2014-08-18 08:07:18]

Yes, tested flash,youtube, and apprtc on link again

https://codereview.chromium.org/426873004/diff/200001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/200001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:367: << " from GPU";
On 2014/08/18 07:59:03, Pawel Osciak wrote:
> Perhaps:
> 
> "Invalid picture size from VDA: " << pb.size().ToString() << " should fit in "
> << picture.size().ToString()

Done.
But you exchanged two sizes?

[Pawel Osciak, 2014-08-18 08:12:23]

https://codereview.chromium.org/426873004/diff/200001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/200001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:367: << " from GPU";
On 2014/08/18 08:07:18, kcwu wrote:
> On 2014/08/18 07:59:03, Pawel Osciak wrote:
> > Perhaps:
> > 
> > "Invalid picture size from VDA: " << pb.size().ToString() << " should fit in
"
> > << picture.size().ToString()
> 
> Done.
> But you exchanged two sizes?

Ah yes, sorry. Should be the other way around.

[teravest, 2014-08-18 14:14:13]

lgtm

[ddorwin, 2014-08-18 16:26:38]

https://codereview.chromium.org/426873004/diff/220001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/220001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:199: // Note this may not work
because encoded size is not always available.
Indentation?
Or is the indentation correct and you are referring to Android?

https://codereview.chromium.org/426873004/diff/220001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:365: LOG(ERROR) << "Invalid picture
size from VDA: " << picture.size().ToString()
Do we need this string in opt builds?

https://codereview.chromium.org/426873004/diff/220001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/220001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:428: LOG(ERROR) << "Invalid picture size from
VDA: " << picture.size().ToString()
Do we need this string in opt builds?

https://codereview.chromium.org/426873004/diff/220001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:436: // Some of the VDAs don't support and
thus don't provide us with visible
Does this comment mean we are handling such VDAs differently or ALWAYS doing
this because some VDAs can't support the preferred method?

[kcwu, 2014-08-18 17:33:06]

https://codereview.chromium.org/426873004/diff/220001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/220001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:199: // Note this may not work
because encoded size is not always available.
On 2014/08/18 16:26:38, ddorwin wrote:
> Indentation?
> Or is the indentation correct and you are referring to Android?

Oops, we decided not to add this comment per review comments. Seems forgot to
remove.
And yes, it was referring to Android.

https://codereview.chromium.org/426873004/diff/220001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:365: LOG(ERROR) << "Invalid picture
size from VDA: " << picture.size().ToString()
On 2014/08/18 16:26:38, ddorwin wrote:
> Do we need this string in opt builds?

Done.

https://codereview.chromium.org/426873004/diff/220001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/220001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:428: LOG(ERROR) << "Invalid picture size from
VDA: " << picture.size().ToString()
On 2014/08/18 16:26:38, ddorwin wrote:
> Do we need this string in opt builds?

Done.

https://codereview.chromium.org/426873004/diff/220001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:436: // Some of the VDAs don't support and
thus don't provide us with visible
On 2014/08/18 16:26:38, ddorwin wrote:
> Does this comment mean we are handling such VDAs differently or ALWAYS doing
> this because some VDAs can't support the preferred method?

Always doing this. The other consideration is GpuVideoDecoder always has
container so it's okay to use config size.

[ddorwin, 2014-08-18 18:50:08]

Thanks. One nit. LG, but I asked sandersd to take a look since he is more
familiar with this code. I think he had some questions.

https://codereview.chromium.org/426873004/diff/240001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/240001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:438: // size in picture.size, passing coded
size instead, so drop it and use
from previous PS: okay, just to be clear nit: ... so always drop...

[sandersd (OOO until July 31), 2014-08-19 00:02:51]

For VT VDA, the API actually does report a visible rect and natural size (I've
not tested them yet). Is there a particular reason that this CL adds a Size and
not a Rect?

For readability, I would prefer the variable was named visible_size.

Otherwise LGTM.

[ddorwin, 2014-08-19 00:08:13]

lgtm % sandersd's comments.

[jln (very slow on Chromium), 2014-08-19 00:11:34]

On 2014/08/18 07:01:54, kcwu wrote:
> On 2014/08/16 23:02:24, Pawel Osciak wrote:

> > The size comes from the more trusted process (GPU), but checking that it is
> > within PictureBuffer size is a good idea.
> 
> Yeah, the size comes from more trusted process (GPU) to less trusted process
> (renderer).

FYI, our security models have become sufficiently complex that we can't really
have a total order on process types like this. Compromising certain types of
renderers from the a compromised GPU process is worth it in certain situations.

> I validated it in rtc_video_decoder.cc. I didn't do so for other clients
> (GpuVideoDecoder, PPB_VideoDecoder_Impl and PepperVideoDecoderHost) since they
> don't use the size.

I'm worried about things like this since they can regress so easily. How could
we make sure it's impossible/difficult to shoot ourselves in the foot? It
worries me when every client has to think about such things. Could you at the
very least add comments where appropriate?

[kcwu, 2014-08-19 03:17:16]

On 2014/08/19 00:02:51, sandersd wrote:
> For VT VDA, the API actually does report a visible rect and natural size (I've
> not tested them yet). Is there a particular reason that this CL adds a Size
and
> not a Rect?
> 
> For readability, I would prefer the variable was named visible_size.
> 
> Otherwise LGTM.

Renamed to visible_size.

Regarding to VT VDA, I only found coded_size_ and texture_size_ in
vt_video_decode_accelerator. Where can I find the rect? And is it really
possible to have non-zero x,y rectangle?
I was assuming coded size is bigger than visible size due to optimization and it
is always aligned upper-left, isn't it?

[chromium-reviews, 2014-08-19 06:49:38]

> Regarding to VT VDA, I only found coded_size_ and texture_size_ in
> vt_video_decode_accelerator. Where can I find the rect?

While the VT API exposes it, the current code doesn't retrieve it. Don't
worry about changing it, I just wanted to mention out that VT can
eventually provide full data here.

> And is it really
> possible to have non-zero x,y rectangle?
> I was assuming coded size is bigger than visible size due to optimization
and it
> is always aligned upper-left, isn't it?

The H.264 SPS header specifies the crop in terms of left, right, top, and
bottom offsets, so it is certainly possible. I don't know how common it is.

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

[kcwu, 2014-08-19 09:45:49]

@sandersd
Thanks for your information. I changed visible_size to visible_rect.

@jln
I added comments in pepper_video_decoder_host.cc and ppb_video_decoder_impl.cc

https://codereview.chromium.org/426873004/diff/240001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/240001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:438: // size in picture.size, passing coded
size instead, so drop it and use
On 2014/08/18 18:50:08, ddorwin wrote:
> from previous PS: okay, just to be clear nit: ... so always drop...

Done.

[ddorwin, 2014-08-19 15:19:29]

content/renderer/media/ and media/ LGTM with nits.

https://codereview.chromium.org/426873004/diff/280001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/280001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:361: // Validate picture size from
GPU
nits:
s/size/rectangle/?
add a period.

https://codereview.chromium.org/426873004/diff/280001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/280001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:425: // Validate picture size from GPU
But we don't actually use the picture size, right? Is this for sanity/security
in some way? It's not clear why we bother, so you might note this.

Also, nits:
s/size/rectangle/?
add a period.

[kcwu, 2014-08-20 06:22:35]

@jln, please take another look.

https://codereview.chromium.org/426873004/diff/280001/content/renderer/media/...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/280001/content/renderer/media/...
content/renderer/media/rtc_video_decoder.cc:361: // Validate picture size from
GPU
On 2014/08/19 15:19:29, ddorwin wrote:
> nits:
> s/size/rectangle/?
> add a period.

Done.

https://codereview.chromium.org/426873004/diff/280001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/426873004/diff/280001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:425: // Validate picture size from GPU
On 2014/08/19 15:19:29, ddorwin wrote:
> But we don't actually use the picture size, right? Is this for sanity/security
> in some way? It's not clear why we bother, so you might note this.
> 
> Also, nits:
> s/size/rectangle/?
> add a period.

Done.

[jln (very slow on Chromium), 2014-08-20 22:40:51]

content/common/gpu/gpu_messages.h lgtm

[hshi1, 2014-08-20 22:43:36]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-08-20 22:45:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kcwu@chromium.org/426873004/300001

[commit-bot: I haz the power, 2014-08-20 23:41:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44440)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/49612)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 23:45:57]

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

[commit-bot: I haz the power, 2014-08-20 23:45:58]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[kcwu, 2014-08-21 08:15:01]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2014-08-21 08:16:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kcwu@chromium.org/426873004/300001

[kcwu, 2014-08-21 08:17:24]

The CQ bit was unchecked by kcwu@chromium.org

[kcwu, 2014-08-21 13:27:59]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2014-08-21 13:28:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kcwu@chromium.org/426873004/340001

[commit-bot: I haz the power, 2014-08-21 21:19:35]

Committed patchset #18 (340001) as 291187

[jennyz, 2014-08-21 23:01:24]

A revert of this CL (patchset #18) has been created in
https://codereview.chromium.org/485103007/ by jennyz@chromium.org.

The reason for reverting is: This cl broke the daisy (chromium) build.
http://build.chromium.org/p/chromiumos.chromium/builders/Daisy%20%28chromium%....

[hshi1, 2014-08-21 23:08:17]

On 2014/08/21 23:01:24, jennyz wrote:
> A revert of this CL (patchset #18) has been created in
> https://codereview.chromium.org/485103007/ by mailto:jennyz@chromium.org.
> 
> The reason for reverting is: This cl broke the daisy (chromium) build.
>
http://build.chromium.org/p/chromiumos.chromium/builders/Daisy%20%28chromium%....

kcwu@: unfortunately the build breakage is not caught by CQ trybots. The build
error is for ARM only (gpu/media/v4l2_video_decode_accelerator.cc is only built
for ARM).