V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674, b:37769238
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2926593002
Cr-Commit-Position: refs/heads/master@{#480416}
Committed: https://chromium.googlesource.com/chromium/src/+/744108776ca82c0a86b3049042e68daf3f2c82c2

[johnylin1, 2017-06-06 13:44:36]

Description was changed from

==========
V4L2SVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
==========

to

==========
V4L2SVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[johnylin1, 2017-06-06 13:46:42]

johnylin@chromium.org changed reviewers:
+ kcwu@chromium.org, owenlin@chromium.org, posciak@chromium.org,
wuchengli@chromium.org

[johnylin1, 2017-06-07 03:02:53]

Description was changed from

==========
V4L2SVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b/37288674, b/37769238
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[johnylin1, 2017-06-07 03:03:21]

Description was changed from

==========
V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b/37288674, b/37769238
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674, b:37769238
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Owen Lin, 2017-06-07 06:11:00]

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder....
media/gpu/h264_decoder.cc:1097: if (new_visible_rect.IsEmpty()) {
I think we shouldn't fail in the case. We can still try to use the coded size. 

Could we still follow the convention that empty rect means information not
available?

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder....
media/gpu/h264_decoder.cc:1123: if ((pic_size_ != new_pic_size) ||
(visible_rect_ != new_visible_rect) ||
We don't care about visible rect change here. Maybe just assign to visible_rect_
in l.1096.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/v4l2_slice_vi...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/v4l2_slice_vi...
media/gpu/v4l2_slice_video_decode_accelerator.cc:809:
DCHECK(!visible_rect_.IsEmpty());
As said in previous file, remove the check. 
Is it possible we change the visible size without changing the coded size. In
that case, we need to get the new visible_rect somewhere else.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/v4l2_slice_vi...
media/gpu/v4l2_slice_video_decode_accelerator.cc:830: if
(!gfx::Rect(coded_size_).Contains(visible_rect_)) {
use pic_size.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
media/gpu/vaapi_video_decode_accelerator.cc:430: gfx::Rect visible_rect =
decoder_->GetVisibleRect();
I am afraid you cannot get the visible size here. The decoder may already saw a
resolution change and switch to a new size.

I would suggest we add a visible_size in H264Picture and set it in the decoders.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vp9_decoder.c...
media/gpu/vp9_decoder.cc:118: if (new_pic_size != pic_size_ || new_render_rect
!= render_rect_) {
Same, we don't care about the visible crop here.

[kcwu, 2017-06-07 07:15:28]

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder....
media/gpu/h264_decoder.cc:1097: if (new_visible_rect.IsEmpty()) {
On 2017/06/07 06:10:59, Owen Lin wrote:
> I think we shouldn't fail in the case. We can still try to use the coded size.

> 
> Could we still follow the convention that empty rect means information not
> available?

Maybe check what spec say how to handle invalid size? Anyway, add warning log if
you don't return failure.

[johnylin1, 2017-06-07 15:38:38]

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder....
media/gpu/h264_decoder.cc:1097: if (new_visible_rect.IsEmpty()) {
On 2017/06/07 07:15:27, kcwu wrote:
> On 2017/06/07 06:10:59, Owen Lin wrote:
> > I think we shouldn't fail in the case. We can still try to use the coded
size.
> 
> > 
> > Could we still follow the convention that empty rect means information not
> > available?
> 
> Maybe check what spec say how to handle invalid size? Anyway, add warning log
if
> you don't return failure.

In H264Parser::GetVisibleRect() it already checked validity and return
base::nullopt if invlaid. That is, new_visible_rect is empty.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/h264_decoder....
media/gpu/h264_decoder.cc:1123: if ((pic_size_ != new_pic_size) ||
(visible_rect_ != new_visible_rect) ||
On 2017/06/07 06:10:59, Owen Lin wrote:
> We don't care about visible rect change here. Maybe just assign to
visible_rect_
> in l.1096.

Done.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/v4l2_slice_vi...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/v4l2_slice_vi...
media/gpu/v4l2_slice_video_decode_accelerator.cc:809:
DCHECK(!visible_rect_.IsEmpty());
On 2017/06/07 06:10:59, Owen Lin wrote:
> As said in previous file, remove the check. 
> Is it possible we change the visible size without changing the coded size. In
> that case, we need to get the new visible_rect somewhere else.

We may pass visible size from decoder to VDA along with OutputPicture(), and put
visible size information into DecodeSurface.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/v4l2_slice_vi...
media/gpu/v4l2_slice_video_decode_accelerator.cc:830: if
(!gfx::Rect(coded_size_).Contains(visible_rect_)) {
On 2017/06/07 06:10:59, Owen Lin wrote:
> use pic_size.

Done.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
media/gpu/vaapi_video_decode_accelerator.cc:430: gfx::Rect visible_rect =
decoder_->GetVisibleRect();
On 2017/06/07 06:11:00, Owen Lin wrote:
> I am afraid you cannot get the visible size here. The decoder may already saw
a
> resolution change and switch to a new size.
> 
> I would suggest we add a visible_size in H264Picture and set it in the
decoders.

I think it would be better to put visible_size in DecodeSurface.

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vp9_decoder.c...
media/gpu/vp9_decoder.cc:118: if (new_pic_size != pic_size_ || new_render_rect
!= render_rect_) {
On 2017/06/07 06:11:00, Owen Lin wrote:
> Same, we don't care about the visible crop here.

Done.

[Pawel Osciak, 2017-06-08 04:58:25]

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
media/gpu/vaapi_video_decode_accelerator.cc:430: gfx::Rect visible_rect =
decoder_->GetVisibleRect();
On 2017/06/07 15:38:37, johnylin1 wrote:
> On 2017/06/07 06:11:00, Owen Lin wrote:
> > I am afraid you cannot get the visible size here. The decoder may already
saw
> a
> > resolution change and switch to a new size.
> > 
> > I would suggest we add a visible_size in H264Picture and set it in the
> decoders.
> 
> I think it would be better to put visible_size in DecodeSurface.


Could you explain your reasoning please?

Owen is right, GetVisibleRect() would only be valid at the time of decoding the
particular picture, not at output time.

Moreover, it would be symmetrical to have the visible rect information somehow
contained within all the *Picture classes. For VP9, it's directly in the
Vp9FrameHeader, because the header syntax element contains it explicitly. For
H264, it's calculated from SPS, so it should probably be in H264Picture class
itself, which contains calculated values for each picture. For VP8, it's also
not in the header, so similarly we could put it in VP8Picture as well.

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/h264_decoder.h
File media/gpu/h264_decoder.h (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/h264_decoder....
media/gpu/h264_decoder.h:95: const gfx::Rect& visible_rect) = 0;
Please document |visible_rect| (especially the meaning of 0,0).

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp8_decoder.c...
media/gpu/vp8_decoder.cc:68: gfx::Size new_pic_size(curr_frame_hdr_->width,
curr_frame_hdr_->height);
Per spec, width/height in frame header may not be aligned to 16. I think we
should align them to 16x16 and use that as coded size, and the unaligned values
for visible size.

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp9_decoder.c...
media/gpu/vp9_decoder.cc:120: if (curr_frame_hdr_->render_width > 0 &&
Perhaps:

gfx::Rect new_render_rect(curr_frame_hdr_->render_width,
                          curr_frame_hdr_->render_height);
if (gfx::Rect(new_pic_size).Contains(new_render_rect))
  render_rect_ = new_render_rect;
else
  render_rect_ = gfx::Rect();

[Owen Lin, 2017-06-09 02:03:17]

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
media/gpu/vaapi_video_decode_accelerator.cc:430: gfx::Rect visible_rect =
decoder_->GetVisibleRect();
On 2017/06/08 04:58:24, Pawel Osciak wrote:
> On 2017/06/07 15:38:37, johnylin1 wrote:
> > On 2017/06/07 06:11:00, Owen Lin wrote:
> > > I am afraid you cannot get the visible size here. The decoder may already
> saw
> > a
> > > resolution change and switch to a new size.
> > > 
> > > I would suggest we add a visible_size in H264Picture and set it in the
> > decoders.
> > 
> > I think it would be better to put visible_size in DecodeSurface.
> 
> 
> Could you explain your reasoning please?
> 
> Owen is right, GetVisibleRect() would only be valid at the time of decoding
the
> particular picture, not at output time.
> 
> Moreover, it would be symmetrical to have the visible rect information somehow
> contained within all the *Picture classes. For VP9, it's directly in the
> Vp9FrameHeader, because the header syntax element contains it explicitly. For
> H264, it's calculated from SPS, so it should probably be in H264Picture class
> itself, which contains calculated values for each picture. For VP8, it's also
> not in the header, so similarly we could put it in VP8Picture as well.

I understand why we need this in DecodeSurface. H264Picture is not available in
OutputSurface.

Even though, adding visible size to H264Picture is still better than adding it
to decoders. From the viewpoint of a decoder, the visible size is not
consistent. It is hard to define what GetVisibleSize() returns. (Or you need
document it as only valid at ....) But adding it to H264Picture and set it
insides decoder makes things clear.

We just pass this info from H264Picture to DecodeSurface.

[johnylin1, 2017-06-12 13:41:53]

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/40001/media/gpu/vaapi_video_d...
media/gpu/vaapi_video_decode_accelerator.cc:430: gfx::Rect visible_rect =
decoder_->GetVisibleRect();
On 2017/06/09 02:03:17, Owen Lin wrote:
> On 2017/06/08 04:58:24, Pawel Osciak wrote:
> > On 2017/06/07 15:38:37, johnylin1 wrote:
> > > On 2017/06/07 06:11:00, Owen Lin wrote:
> > > > I am afraid you cannot get the visible size here. The decoder may
already
> > saw
> > > a
> > > > resolution change and switch to a new size.
> > > > 
> > > > I would suggest we add a visible_size in H264Picture and set it in the
> > > decoders.
> > > 
> > > I think it would be better to put visible_size in DecodeSurface.
> > 
> > 
> > Could you explain your reasoning please?
> > 
> > Owen is right, GetVisibleRect() would only be valid at the time of decoding
> the
> > particular picture, not at output time.
> > 
> > Moreover, it would be symmetrical to have the visible rect information
somehow
> > contained within all the *Picture classes. For VP9, it's directly in the
> > Vp9FrameHeader, because the header syntax element contains it explicitly.
For
> > H264, it's calculated from SPS, so it should probably be in H264Picture
class
> > itself, which contains calculated values for each picture. For VP8, it's
also
> > not in the header, so similarly we could put it in VP8Picture as well.
> 
> I understand why we need this in DecodeSurface. H264Picture is not available
in
> OutputSurface.
> 
> Even though, adding visible size to H264Picture is still better than adding it
> to decoders. From the viewpoint of a decoder, the visible size is not
> consistent. It is hard to define what GetVisibleSize() returns. (Or you need
> document it as only valid at ....) But adding it to H264Picture and set it
> insides decoder makes things clear.
> 
> We just pass this info from H264Picture to DecodeSurface.

My thought is same as Owen, we need to have visible_rect in DecodeSurface,
because H264Picture is not available in VDA.

After more tracings, I think maybe we could set visible_rect into H264Picture
when it has been created (same as VP8, VP9). Please see my new modifications on
h264_decoder.cc

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/h264_decoder.h
File media/gpu/h264_decoder.h (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/h264_decoder....
media/gpu/h264_decoder.h:95: const gfx::Rect& visible_rect) = 0;
On 2017/06/08 04:58:24, Pawel Osciak wrote:
> Please document |visible_rect| (especially the meaning of 0,0).

This is removed in newer patch, I document visible_rect in H264/VP8/VP9Picture
instead.

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp8_decoder.c...
media/gpu/vp8_decoder.cc:68: gfx::Size new_pic_size(curr_frame_hdr_->width,
curr_frame_hdr_->height);
On 2017/06/08 04:58:25, Pawel Osciak wrote:
> Per spec, width/height in frame header may not be aligned to 16. I think we
> should align them to 16x16 and use that as coded size, and the unaligned
values
> for visible size.

I thought we did the alignment while v4l2_slice_vda CreateOutputBuffer()
https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele...

Or you say we need to do the alignment here for VP8?

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp9_decoder.c...
media/gpu/vp9_decoder.cc:120: if (curr_frame_hdr_->render_width > 0 &&
On 2017/06/08 04:58:25, Pawel Osciak wrote:
> Perhaps:
> 
> gfx::Rect new_render_rect(curr_frame_hdr_->render_width,
>                           curr_frame_hdr_->render_height);
> if (gfx::Rect(new_pic_size).Contains(new_render_rect))
>   render_rect_ = new_render_rect;
> else
>   render_rect_ = gfx::Rect();

Done.

[Owen Lin, 2017-06-13 03:00:15]

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/h264_decoder...
media/gpu/h264_decoder.cc:1098: DVLOG(1) << "New visible rect: " <<
new_visible_rect.ToString();
level 1 is for error. Please use DVLOG(2).

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.cc:72: void SetVisibleRect(const
gfx::Rect& visible_rect) {
Simple getters/setters should use the set_visibe_rect() style.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.cc:439: << " is ready. visible rect: "
<< visible_rect.ToString();
Have you run git cl format?

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:117: // Due to specification render size should be set
as not larger than frame
Why we need to care about render_and_frame_size_different == 0 here?
It should be already handled in vp9_parser.

https://cs.chromium.org/chromium/src/media/filters/vp9_uncompressed_header_pa...

Besides, I am not sure if returning (0, 0) is better than returning pic_size. I
think pic_size is still better than coded_size.

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_picture.h
File media/gpu/vp9_picture.h (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_picture....
media/gpu/vp9_picture.h:30: // header, or set to gfx::Rect(0, 0) for indicating
the client using the
nit: I think we don't need to explain what to do in the client here.

Just "for indicating the information is invalid or not available"?

[johnylin1, 2017-06-13 07:10:30]

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.cc:439: << " is ready. visible rect: "
<< visible_rect.ToString();
On 2017/06/13 03:00:14, Owen Lin wrote:
> Have you run git cl format?

Yes, and this is as the tool expects.

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:117: // Due to specification render size should be set
as not larger than frame
On 2017/06/13 03:00:14, Owen Lin wrote:
> Why we need to care about render_and_frame_size_different == 0 here?
> It should be already handled in vp9_parser.
Yes, actually I want this check just to prevent if there is abnormal values of
parsed render_width/height. 
> 
>
https://cs.chromium.org/chromium/src/media/filters/vp9_uncompressed_header_pa...
> 
> Besides, I am not sure if returning (0, 0) is better than returning pic_size.
I
> think pic_size is still better than coded_size.

It looks also good to me. We could apply this on both VP9 and H264.
Pawel, what do you think?

[johnylin1, 2017-06-13 07:21:22]

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/h264_decoder...
media/gpu/h264_decoder.cc:1098: DVLOG(1) << "New visible rect: " <<
new_visible_rect.ToString();
On 2017/06/13 03:00:14, Owen Lin wrote:
> level 1 is for error. Please use DVLOG(2).

Done.

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.cc:72: void SetVisibleRect(const
gfx::Rect& visible_rect) {
On 2017/06/13 03:00:14, Owen Lin wrote:
> Simple getters/setters should use the set_visibe_rect() style.
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

[johnylin1, 2017-06-13 15:23:56]

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/80001/media/gpu/vp8_decoder.c...
media/gpu/vp8_decoder.cc:68: gfx::Size new_pic_size(curr_frame_hdr_->width,
curr_frame_hdr_->height);
On 2017/06/12 13:41:53, johnylin1 wrote:
> On 2017/06/08 04:58:25, Pawel Osciak wrote:
> > Per spec, width/height in frame header may not be aligned to 16. I think we
> > should align them to 16x16 and use that as coded size, and the unaligned
> values
> > for visible size.
> 
> I thought we did the alignment while v4l2_slice_vda CreateOutputBuffer()
>
https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele...
> 
> Or you say we need to do the alignment here for VP8?

Done, as following to spec.

[Owen Lin, 2017-06-14 02:28:25]

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:117: // Due to specification render size should be set
as not larger than frame
On 2017/06/13 07:10:30, johnylin1 wrote:
> On 2017/06/13 03:00:14, Owen Lin wrote:
> > Why we need to care about render_and_frame_size_different == 0 here?
> > It should be already handled in vp9_parser.
> Yes, actually I want this check just to prevent if there is abnormal values of
> parsed render_width/height. 

I see. Then I think we don't need to mention about the detail of VP9 spec here
since it doesn't provide more information, I mean render_width/height are
already following the spec and set properly in the vp9 parser. In this comment,
saying "For safety, check the validity of render size or ..." should be enough. 

> > 
> >
>
https://cs.chromium.org/chromium/src/media/filters/vp9_uncompressed_header_pa...
> > 
> > Besides, I am not sure if returning (0, 0) is better than returning
pic_size.
> I
> > think pic_size is still better than coded_size.
> 
> It looks also good to me. We could apply this on both VP9 and H264.
> Pawel, what do you think?

https://codereview.chromium.org/2926593002/diff/160001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/160001/media/gpu/vp8_decoder....
media/gpu/vp8_decoder.cc:77: uint16_t coded_width =
You may use base::bits::Align.

https://cs.chromium.org/chromium/src/base/bits.h?rcl=59571c968913ef4968f6fb9a...

But I would suggest to have another CL for it, since it sounds like a
independent issue.

[johnylin1, 2017-06-14 14:11:22]

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:117: // Due to specification render size should be set
as not larger than frame
On 2017/06/14 02:28:25, Owen Lin wrote:
> On 2017/06/13 07:10:30, johnylin1 wrote:
> > On 2017/06/13 03:00:14, Owen Lin wrote:
> > > Why we need to care about render_and_frame_size_different == 0 here?
> > > It should be already handled in vp9_parser.
> > Yes, actually I want this check just to prevent if there is abnormal values
of
> > parsed render_width/height. 
> 
> I see. Then I think we don't need to mention about the detail of VP9 spec here
> since it doesn't provide more information, I mean render_width/height are
> already following the spec and set properly in the vp9 parser. In this
comment,
> saying "For safety, check the validity of render size or ..." should be
enough. 
> 
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/media/filters/vp9_uncompressed_header_pa...
> > > 
> > > Besides, I am not sure if returning (0, 0) is better than returning
> pic_size.
> > I
> > > think pic_size is still better than coded_size.
> > 
> > It looks also good to me. We could apply this on both VP9 and H264.
> > Pawel, what do you think?
> 

Done.

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_picture.h
File media/gpu/vp9_picture.h (right):

https://codereview.chromium.org/2926593002/diff/120001/media/gpu/vp9_picture....
media/gpu/vp9_picture.h:30: // header, or set to gfx::Rect(0, 0) for indicating
the client using the
On 2017/06/13 03:00:14, Owen Lin wrote:
> nit: I think we don't need to explain what to do in the client here.
> 
> Just "for indicating the information is invalid or not available"?

Done.

https://codereview.chromium.org/2926593002/diff/160001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/160001/media/gpu/vp8_decoder....
media/gpu/vp8_decoder.cc:77: uint16_t coded_width =
On 2017/06/14 02:28:25, Owen Lin wrote:
> You may use base::bits::Align.
> 
>
https://cs.chromium.org/chromium/src/base/bits.h?rcl=59571c968913ef4968f6fb9a...
> 
> But I would suggest to have another CL for it, since it sounds like a
> independent issue. 

Sounds good to me. Let's create another CL then.

[Pawel Osciak, 2017-06-16 07:14:01]

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/h264_decoder...
media/gpu/h264_decoder.cc:1096: gfx::Rect new_visible_rect =
sps->GetVisibleRect().value_or(gfx::Rect());
Would it make sense to move this to l.1133 and also check against pic_size?

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/h264_decoder...
media/gpu/h264_decoder.cc:1360: curr_pic_->visible_rect = visible_rect_;
Could we perhaps move this to InitCurrPicture()?

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.h:152: gfx::Rect visible_rect,
Please update documentation.

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp8_picture.h
File media/gpu/vp8_picture.h (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp8_picture....
media/gpu/vp8_picture.h:25: // large as picture size.
Per spec: http://www.rfc-editor.org/rfc/rfc6386.txt (page 31), width/height may
not be aligned to 16x16, so the visible size may not be equal to coded size.

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:115: gfx::Rect
new_render_rect(curr_frame_hdr_->render_width,
Perhaps move this to after new_pic_size checks and verify against new_pic_size?

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:119: new_render_rect = gfx::Rect();
Please add a log for this error.

[johnylin1, 2017-06-16 08:15:10]

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/h264_decoder.cc
File media/gpu/h264_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/h264_decoder...
media/gpu/h264_decoder.cc:1096: gfx::Rect new_visible_rect =
sps->GetVisibleRect().value_or(gfx::Rect());
On 2017/06/16 07:14:00, Pawel Osciak wrote:
> Would it make sense to move this to l.1133 and also check against pic_size?

Moved to l.1133. sps->GetVisibleRect() already checked visible_size against
pic_size, so we don't need to check again.
https://cs.chromium.org/chromium/src/media/filters/h264_parser.cc?q=h264_pars...

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.h:152: gfx::Rect visible_rect,
On 2017/06/16 07:14:01, Pawel Osciak wrote:
> Please update documentation.

Done.

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp8_picture.h
File media/gpu/vp8_picture.h (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp8_picture....
media/gpu/vp8_picture.h:25: // large as picture size.
On 2017/06/16 07:14:01, Pawel Osciak wrote:
> Per spec: http://www.rfc-editor.org/rfc/rfc6386.txt (page 31), width/height
may
> not be aligned to 16x16, so the visible size may not be equal to coded size.

Done.

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:115: gfx::Rect
new_render_rect(curr_frame_hdr_->render_width,
On 2017/06/16 07:14:01, Pawel Osciak wrote:
> Perhaps move this to after new_pic_size checks and verify against
new_pic_size?

Done.

https://codereview.chromium.org/2926593002/diff/180001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:119: new_render_rect = gfx::Rect();
On 2017/06/16 07:14:01, Pawel Osciak wrote:
> Please add a log for this error.

Done.

[Owen Lin, 2017-06-16 09:07:33]

lgtm % nit

https://codereview.chromium.org/2926593002/diff/200001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/200001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:146: << new_render_rect.ToString()
How about DVLOG(2) since we don't failed on this case.

[johnylin1, 2017-06-16 11:43:53]

https://codereview.chromium.org/2926593002/diff/200001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/200001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:146: << new_render_rect.ToString()
On 2017/06/16 09:07:33, Owen Lin wrote:
> How about DVLOG(2) since we don't failed on this case.

Done.

[Pawel Osciak, 2017-06-19 06:27:41]

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.h:148: // Puts contents of |va_surface|
and set |visible_rect| into given |picture|,
visible_rect is not a part of picture. Could we perhaps say something like:

"Puts contents of |va_surface| into given |picture|, releases the surface and
passes the resulting picture to client to output the given |visible_rect| part
of it"

?

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp8_decoder....
media/gpu/vp8_decoder.cc:97: curr_pic_->visible_rect = gfx::Rect(pic_size_);
Since VP8Parser does not seem to be aligning to 16x16, I think we should make
visible_size use width/height from the header, while pic_size should be that
visible_size aligned to 16x16...

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:145: DVLOG(2) << "Render size exceeds picture size.
render size: "
s/2/1/ please.

[johnylin1, 2017-06-19 07:13:28]

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vaapi_video_...
File media/gpu/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vaapi_video_...
media/gpu/vaapi_video_decode_accelerator.h:148: // Puts contents of |va_surface|
and set |visible_rect| into given |picture|,
On 2017/06/19 06:27:40, Pawel Osciak wrote:
> visible_rect is not a part of picture. Could we perhaps say something like:
> 
> "Puts contents of |va_surface| into given |picture|, releases the surface and
> passes the resulting picture to client to output the given |visible_rect| part
> of it"
> 
> ?

Done.

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp8_decoder.cc
File media/gpu/vp8_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp8_decoder....
media/gpu/vp8_decoder.cc:97: curr_pic_->visible_rect = gfx::Rect(pic_size_);
On 2017/06/19 06:27:40, Pawel Osciak wrote:
> Since VP8Parser does not seem to be aligning to 16x16, I think we should make
> visible_size use width/height from the header, while pic_size should be that
> visible_size aligned to 16x16...

We will put to an individual CL as discussed in:
https://codereview.chromium.org/2926593002/diff/160001/media/gpu/vp8_decoder....

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp9_decoder.cc
File media/gpu/vp9_decoder.cc (right):

https://codereview.chromium.org/2926593002/diff/220001/media/gpu/vp9_decoder....
media/gpu/vp9_decoder.cc:145: DVLOG(2) << "Render size exceeds picture size.
render size: "
On 2017/06/19 06:27:40, Pawel Osciak wrote:
> s/2/1/ please.

Done.

[Pawel Osciak, 2017-06-19 07:19:05]

lgtm

[johnylin1, 2017-06-19 07:23:14]

The CQ bit was checked by johnylin@chromium.org

[johnylin1, 2017-06-19 07:23:15]

The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2926593002/#ps240001
(title: "V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client")

[commit-bot: I haz the power, 2017-06-19 07:23:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 08:08:55]

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

[commit-bot: I haz the power, 2017-06-19 08:08:56]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[johnylin1, 2017-06-19 12:11:36]

The CQ bit was checked by johnylin@chromium.org

[commit-bot: I haz the power, 2017-06-19 12:11:41]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 13:05:32]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497874296431890,
"parent_rev": "df32089dae6e21484be5afb43aedc33caab54f83", "commit_rev":
"744108776ca82c0a86b3049042e68daf3f2c82c2"}

[commit-bot: I haz the power, 2017-06-19 13:05:45]

Description was changed from

==========
V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674, b:37769238
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

The visible size (render size) should be obtained from header parsing of
decoders. Pass visible size format to Android container to get correct
rendering size.

BUG=chromium:402760, b:37288674, b:37769238
TEST=play video on ARC++ which has different on visible and coded size
(e.g. 320x180). Check the format decoded from Chrome has correct visible
and coded size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2926593002
Cr-Commit-Position: refs/heads/master@{#480416}
Committed:
https://chromium.googlesource.com/chromium/src/+/744108776ca82c0a86b3049042e6...
==========

[commit-bot: I haz the power, 2017-06-19 13:05:47]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/744108776ca82c0a86b3049042e6...