Remove kIgnoreResolutionLimitsForAcceleratedVideoDecode flag

Remove kIgnoreResolutionLimitsForAcceleratedVideoDecode flag

Since decoder supports GetSupportedProfiles and v4l2 supports
ENUM_FRAMESIZES, this flag can be removed and use runtime detection to
get maximum resolution for a specified profile.

BUG=350197
TEST=manually test pass on peach_pit, peach_pi, nyan_big and veyron_jerry.

Committed: https://crrev.com/3d20a9d06d9dda27e069d07e9eb358a44633ca1c
Cr-Commit-Position: refs/heads/master@{#329395}

[henryhsu, 2015-04-20 10:54:21]

PTAL

[henryhsu, 2015-04-20 10:54:56]

henryhsu@chromium.org changed reviewers:
+ posciak@chromium.org, wuchengli@chromium.org

[wuchengli, 2015-04-20 14:05:16]

Please test on veyron.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:201: gfx::Size
V4L2Device::GetSupportedMaxResolution(
s/GetSupportedMaxResolution/GetMaxSupportedResolution/

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:203: unsigned int format) {
s/format/pixelformat/

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:205: v4l2_frmsizeenum fsize;
s/fsize/frame_size/ is more descriptive

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:208: int ret;
not needed

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:219:
max_resolution.SetSize(fsize.stepwise.max_width,
Why don't we need to check if this is the maximum like line 212-216? There will
be only one frame size?

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:221: }
No need to consider V4L2_FRMIVAL_TYPE_CONTINUOUS?

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:467:
profile.max_resolution.SetSize(1920, 1088);
This should be handled in V4L2Device::GetSupportedMaxResolution. Move this
there.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:302: if
(profile.max_resolution.IsEmpty()) {
remove

[henryhsu, 2015-04-21 05:56:28]

PTAL

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:201: gfx::Size
V4L2Device::GetSupportedMaxResolution(
On 2015/04/20 14:05:16, wuchengli wrote:
> s/GetSupportedMaxResolution/GetMaxSupportedResolution/

Done.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:203: unsigned int format) {
On 2015/04/20 14:05:16, wuchengli wrote:
> s/format/pixelformat/

Done.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:205: v4l2_frmsizeenum fsize;
On 2015/04/20 14:05:16, wuchengli wrote:
> s/fsize/frame_size/ is more descriptive

Done.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:208: int ret;
On 2015/04/20 14:05:16, wuchengli wrote:
> not needed

Done.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:219:
max_resolution.SetSize(fsize.stepwise.max_width,
On 2015/04/20 14:05:16, wuchengli wrote:
> Why don't we need to check if this is the maximum like line 212-216? There
will
> be only one frame size?

Because discrete type has many results. We need to query and increase index to
get different supported resolutions until query return failure.
But stepwise type just returns a region including minimum and maximum width and
height. So we can use maximum width and height directly.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.cc:221: }
On 2015/04/20 14:05:16, wuchengli wrote:
> No need to consider V4L2_FRMIVAL_TYPE_CONTINUOUS?

Done.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:467:
profile.max_resolution.SetSize(1920, 1088);
On 2015/04/20 14:05:16, wuchengli wrote:
> This should be handled in V4L2Device::GetSupportedMaxResolution. Move this
> there.

Done.

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:302: if
(profile.max_resolution.IsEmpty()) {
On 2015/04/20 14:05:16, wuchengli wrote:
> remove

Done.

[wuchengli, 2015-04-21 08:46:42]

https://codereview.chromium.org/1097913002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/chrome_restart_request.cc (right):

https://codereview.chromium.org/1097913002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/chrome_restart_request.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
For change description, nyan_big won't be 1920x1088 after the driver supports
this. It will be confusing when the driver fixes it and you submit this CL. You
can say "Test on the devices that do not support ENUM_FRAMESIZES."

https://codereview.chromium.org/1097913002/diff/20001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/20001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.h:38: static gfx::Size
GetMaxSupportedResolution(
This should be a member function, not static. But does this work for image
processor?

[Pawel Osciak, 2015-04-22 08:50:56]

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:206: if (frame_size.type ==
V4L2_FRMIVAL_TYPE_DISCRETE) {
This define is for framerate enumeration. The correct define for frame sizes is
V4L2_FRMSIZE_TYPE_DISCRETE. Same for other defines.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:207: if (frame_size.discrete.width >
Perhaps merge into the if above?

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:208: static_cast<unsigned
int>(max_resolution.width()) &&
I'd prefer base::checked_cast on frame_size.discrete.width to int.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:211:
max_resolution.SetSize(frame_size.discrete.width,
If you get say 640x360 and next 640x480, I think this will ignore 640x480? Do we
need s/&&/||/ ?

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:217: frame_size.stepwise.max_height);
You could break here to avoid an additional ioctl call.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:221: LOG(ERROR) <<
"GetMaxSupportedResolution failed for format "
s/format/fourcc/

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:223: max_resolution.SetSize(1920, 1088);
Please also say that we are falling back to 1088p in the log message.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.h:110: gfx::Size
GetMaxSupportedResolution(unsigned int pixelformat);
s/unsigned int/uint32_t/

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:581: if
(max_resolution.width() >= 4096 && max_resolution.height() >= 2160)
This should be > 1080p, not > 4k.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2504:
V4L2SliceVideoDecodeAccelerator::GetSupportedProfiles() {
Could this method be unified with V4L2VDA::GetSupporteProfiles(), taking a
vector of fourccs as an argument to handle different pixelformats supported by
each VDA?

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2516: // Get
maximum resolution for each format
I feel this comment doesn't really add anything, the code is self-explanatory.
I'd suggest removing it.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:461: // Get maximum
resolution for each format
Ditto.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:299: // Get maximum
resolution for each format
And here as well.

[Pawel Osciak, 2015-04-22 10:01:38]

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:211:
max_resolution.SetSize(frame_size.discrete.width,
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> If you get say 640x360 and next 640x480, I think this will ignore 640x480? Do
we
> need s/&&/||/ ?

Sorry, not this, but probably s/>/>=/ ?

[henryhsu, 2015-04-23 03:56:43]

PTAL

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:206: if (frame_size.type ==
V4L2_FRMIVAL_TYPE_DISCRETE) {
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> This define is for framerate enumeration. The correct define for frame sizes
is
> V4L2_FRMSIZE_TYPE_DISCRETE. Same for other defines.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:207: if (frame_size.discrete.width >
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> Perhaps merge into the if above?

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:208: static_cast<unsigned
int>(max_resolution.width()) &&
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> I'd prefer base::checked_cast on frame_size.discrete.width to int.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:211:
max_resolution.SetSize(frame_size.discrete.width,
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> If you get say 640x360 and next 640x480, I think this will ignore 640x480? Do
we
> need s/&&/||/ ?

Use || may have the case: first is 640x480, second is 1080x360.
Is the case possible?

I'd prefer to use ">=".

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:217: frame_size.stepwise.max_height);
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> You could break here to avoid an additional ioctl call.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:221: LOG(ERROR) <<
"GetMaxSupportedResolution failed for format "
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> s/format/fourcc/

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:223: max_resolution.SetSize(1920, 1088);
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> Please also say that we are falling back to 1088p in the log message.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.h:110: gfx::Size
GetMaxSupportedResolution(unsigned int pixelformat);
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> s/unsigned int/uint32_t/

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:581: if
(max_resolution.width() >= 4096 && max_resolution.height() >= 2160)
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> This should be > 1080p, not > 4k.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2504:
V4L2SliceVideoDecodeAccelerator::GetSupportedProfiles() {
On 2015/04/22 08:50:55, Pawel Osciak wrote:
> Could this method be unified with V4L2VDA::GetSupporteProfiles(), taking a
> vector of fourccs as an argument to handle different pixelformats supported by
> each VDA?

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2516: // Get
maximum resolution for each format
On 2015/04/22 08:50:56, Pawel Osciak wrote:
> I feel this comment doesn't really add anything, the code is self-explanatory.
> I'd suggest removing it.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:461: // Get maximum
resolution for each format
On 2015/04/22 08:50:56, Pawel Osciak wrote:
> Ditto.

Done.

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/40001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:299: // Get maximum
resolution for each format
On 2015/04/22 08:50:56, Pawel Osciak wrote:
> And here as well.

Done.

[wuchengli, 2015-04-23 07:11:45]

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:222: << pixelformat << ", fall back to
1088p";
1088p is not a common usage. To avoid confusion, s/1088p/1920x1088/

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:229:
V4L2Device::GetSupportedDecodeProfiles(std::vector<uint32_t> fourcc_formats) {
Let's move "const Type type_;" from GenericV4L2Device and TegraV4L2Device to
V4L2Device. So we can check |type_| here and make sure this is kDecoder.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.h:115: std::vector<uint32_t>
fourcc_formats);
const &.

s/fourcc_formats/pixelformats/. The naming of the arguments should be consistent
to GetMaxSupportedResolution().

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:581: if
(max_resolution.width() >= 1920 && max_resolution.height() >= 1080)
This should be >, not >=.

s/1080/1088/ because the default in GetSupportedDecodeProfiles is 1088.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2508: return
profiles;
return SupportedProfiles(). remove |profiles|

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:453: return profiles;
return SupportedProfiles(). remove |profiles|

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1771: if
(max_resolution.width() >= 1920 && max_resolution.height() >= 1080)
s/>=/>/

[henryhsu, 2015-04-23 08:21:54]

all done

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:222: << pixelformat << ", fall back to
1088p";
On 2015/04/23 07:11:44, wuchengli wrote:
> 1088p is not a common usage. To avoid confusion, s/1088p/1920x1088/

Done.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:229:
V4L2Device::GetSupportedDecodeProfiles(std::vector<uint32_t> fourcc_formats) {
On 2015/04/23 07:11:44, wuchengli wrote:
> Let's move "const Type type_;" from GenericV4L2Device and TegraV4L2Device to
> V4L2Device. So we can check |type_| here and make sure this is kDecoder.

Done.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.h:115: std::vector<uint32_t>
fourcc_formats);
On 2015/04/23 07:11:44, wuchengli wrote:
> const &.
> 
> s/fourcc_formats/pixelformats/. The naming of the arguments should be
consistent
> to GetMaxSupportedResolution().

Done.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:581: if
(max_resolution.width() >= 1920 && max_resolution.height() >= 1080)
On 2015/04/23 07:11:44, wuchengli wrote:
> This should be >, not >=.
> 
> s/1080/1088/ because the default in GetSupportedDecodeProfiles is 1088.

Done.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2508: return
profiles;
On 2015/04/23 07:11:44, wuchengli wrote:
> return SupportedProfiles(). remove |profiles|

Done.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:453: return profiles;
On 2015/04/23 07:11:44, wuchengli wrote:
> return SupportedProfiles(). remove |profiles|

Done.

https://codereview.chromium.org/1097913002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1771: if
(max_resolution.width() >= 1920 && max_resolution.height() >= 1080)
On 2015/04/23 07:11:44, wuchengli wrote:
> s/>=/>/

Done.

[wuchengli, 2015-04-24 02:45:40]

lgtm

https://codereview.chromium.org/1097913002/diff/80001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/80001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:234: CHECK_EQ(type_, kDecoder);
s/CHECK/DCHECK/. We should not crash the entire GPU process when VDA has a bug.
All other functionality are still working.

[henryhsu, 2015-04-24 03:01:49]

henryhsu@chromium.org changed reviewers:
+ piman@chromium.org, xhwang@chromium.org

[henryhsu, 2015-04-24 03:01:50]

xhwang@chromium.org: Please review changes in 

piman@chromium.org: Please review changes in 

Hi piman and xhwang,
Please take a look.
This CL will be landed after nyan and veyron series support ENUM_FRAMESIZES.

[henryhsu, 2015-04-24 03:13:40]

On 2015/04/24 03:01:50, henryhsu wrote:
> mailto:xhwang@chromium.org: Please review changes in 
> 
> mailto:piman@chromium.org: Please review changes in 
> 
> Hi piman and xhwang,
> Please take a look.
> This CL will be landed after nyan and veyron series support ENUM_FRAMESIZES.

Hi piman,
Please review chrome/ and content/ directories.

Hi xhwang,
Please review media/ directory.

Thank you.

[piman, 2015-04-24 03:16:29]

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:119: V4L2Device(Type type);
nit: explicit

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2510:
{V4L2_PIX_FMT_H264_SLICE, V4L2_PIX_FMT_VP8_FRAME}));
c++11-style initializer lists are currently forbidden by the style guide (see
http://chromium-cpp.appspot.com/).

Note: maybe GetSupportedDecodeProfiles can just take an array/length instead of
a std::vector? Avoids copies etc.

[henryhsu, 2015-04-24 05:25:46]

all done

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:119: V4L2Device(Type type);
On 2015/04/24 03:16:29, piman (Very slow to review) wrote:
> nit: explicit

Done.

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2510:
{V4L2_PIX_FMT_H264_SLICE, V4L2_PIX_FMT_VP8_FRAME}));
On 2015/04/24 03:16:29, piman (Very slow to review) wrote:
> c++11-style initializer lists are currently forbidden by the style guide (see
> http://chromium-cpp.appspot.com/).
> 
> Note: maybe GetSupportedDecodeProfiles can just take an array/length instead
of
> a std::vector? Avoids copies etc.

Done.

[xhwang, 2015-04-24 05:40:02]

media/ lgtm

[Pawel Osciak, 2015-04-24 05:55:51]

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:247: switch (fmtdesc.pixelformat) {
We may start missing something in the future if pixelformats and
fmtdesc.pixelformat match, but we don't have a case below for it. We should
probably add a default case here. Maybe something like this:

  default:
    NOTREACHED() << "Unhandled pixelformat " << fmtdesc.pixelformat;
    break;

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:253:
static_cast<media::VideoCodecProfile>(media_profile);
Is this an error/warning if we remove the cast?

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:259: profile.profile =
media::VP8PROFILE_ANY;
To be supercorrect and pedantic, we could consider:

while(...) {
  ...

  switch() {
    case ...H264...:
      min_profile = media::H264PROFILE_MIN;
      max_profile = media::H264PROFILE_MAX;
      break;

    case ...VP8...:
      min_profile = media::VP8PROFILE_MIN;
      max_profile = media::VP8PROFILE_MAX:
      break;

     etc...

    default:
      NOTREACHED() << ...
      continue;
  }

  for (profile = min; profile <= max; ++profile)
    profiles.push_back(profile);
}

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:121: const Type type_;
Nit: please add empty line above.

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2509: uint32_t
supported_formats[] = {
static const?

[henryhsu, 2015-04-24 07:07:29]

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:247: switch (fmtdesc.pixelformat) {
On 2015/04/24 05:55:51, Pawel Osciak wrote:
> We may start missing something in the future if pixelformats and
> fmtdesc.pixelformat match, but we don't have a case below for it. We should
> probably add a default case here. Maybe something like this:
> 
>   default:
>     NOTREACHED() << "Unhandled pixelformat " << fmtdesc.pixelformat;
>     break;

Done.

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:253:
static_cast<media::VideoCodecProfile>(media_profile);
On 2015/04/24 05:55:51, Pawel Osciak wrote:
> Is this an error/warning if we remove the cast?

Yes, we have invalid conversion error without the cast.

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:259: profile.profile =
media::VP8PROFILE_ANY;
On 2015/04/24 05:55:51, Pawel Osciak wrote:
> To be supercorrect and pedantic, we could consider:
> 
> while(...) {
>   ...
> 
>   switch() {
>     case ...H264...:
>       min_profile = media::H264PROFILE_MIN;
>       max_profile = media::H264PROFILE_MAX;
>       break;
> 
>     case ...VP8...:
>       min_profile = media::VP8PROFILE_MIN;
>       max_profile = media::VP8PROFILE_MAX:
>       break;
> 
>      etc...
> 
>     default:
>       NOTREACHED() << ...
>       continue;
>   }
> 
>   for (profile = min; profile <= max; ++profile)
>     profiles.push_back(profile);
> }

Done.

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/100001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:121: const Type type_;
On 2015/04/24 05:55:51, Pawel Osciak wrote:
> Nit: please add empty line above.

Done.

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2509: uint32_t
supported_formats[] = {
On 2015/04/24 05:55:51, Pawel Osciak wrote:
> static const?

Since this function is only called once, I think const is enough.

[Pawel Osciak, 2015-04-27 05:36:15]

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:237: profile.min_resolution.SetSize(16,
16);
Framesize API also supports minimum resolution, there is a min_width/height
member in v4l2_frmsizeenum.stepwise and we can figure out minimum values from
continuous as well. I think we should try to use that information.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:247: uint32_t min_profile, max_profile;
s/uint32_t/int/

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:266: break;
NOTREACHED is not a fatal failure in Release, so you need to "return profiles;"
from here. Otherwise we'll iterate over uninitialized values in the for loop
below.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:268: for (uint32_t media_profile =
min_profile; media_profile <= max_profile;
s/uint32_t/int/

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:113: // Get supported profiles for
decoder according to |pixelformats|.
Return supported profiles for decoder, including only profiles for given fourcc
|pixelformats|.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:115: const uint32_t num_formats, const
uint32_t pixelformats[]);
s/const uint32_t num_formats/size_t num_formats/

[henryhsu, 2015-04-27 07:23:51]

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:237: profile.min_resolution.SetSize(16,
16);
On 2015/04/27 05:36:15, Pawel Osciak wrote:
> Framesize API also supports minimum resolution, there is a min_width/height
> member in v4l2_frmsizeenum.stepwise and we can figure out minimum values from
> continuous as well. I think we should try to use that information.

Done.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:247: uint32_t min_profile, max_profile;
On 2015/04/27 05:36:15, Pawel Osciak wrote:
> s/uint32_t/int/

Done.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:266: break;
On 2015/04/27 05:36:14, Pawel Osciak wrote:
> NOTREACHED is not a fatal failure in Release, so you need to "return
profiles;"
> from here. Otherwise we'll iterate over uninitialized values in the for loop
> below.

Done.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:268: for (uint32_t media_profile =
min_profile; media_profile <= max_profile;
On 2015/04/27 05:36:14, Pawel Osciak wrote:
> s/uint32_t/int/

Done.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:113: // Get supported profiles for
decoder according to |pixelformats|.
On 2015/04/27 05:36:15, Pawel Osciak wrote:
> Return supported profiles for decoder, including only profiles for given
fourcc
> |pixelformats|.

Done.

https://codereview.chromium.org/1097913002/diff/140001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:115: const uint32_t num_formats, const
uint32_t pixelformats[]);
On 2015/04/27 05:36:15, Pawel Osciak wrote:
> s/const uint32_t num_formats/size_t num_formats/

I'd prefer to keep const here.

[piman, 2015-04-27 23:00:29]

lgtm

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2510:
V4L2_PIX_FMT_H264_SLICE, V4L2_PIX_FMT_VP8_FRAME};
nit: } on next line

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/120001/content/common/gpu/med...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:455:
V4L2_PIX_FMT_H264, V4L2_PIX_FMT_VP8, V4L2_PIX_FMT_VP9};
nit: } on next line.

[Pawel Osciak, 2015-04-28 01:35:33]

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:243: if (min_resolution->IsEmpty()) {
Hm, I'm wondering what we should do if the codec reports 0,0 as min
resolution...  But I feel 0,0, shouldn't be allowed, so this is correct. If 0,0
was ok, we wouldn't need a separate if here for this.

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:246: << ", fall back to 16x16.";
Please move this below next line and

s/<< ", fall back to 16x16.";/<< ", fall back to " <<
min_resolution->ToString();/

Similarly for max above.

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:247: min_resolution->SetSize(1920,
1088);
s/1920, 1088/16, 16/

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:112: void GetSupportedResolution(uint32_t
pixelformat, gfx::Size* max_resolution,
Sorry for nitpicking, but could we have min before max please? Just to make it
more natural. Thanks.

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:295: gfx::Size
min_resolution;
We should add this to VEA::SupportedProfile for consistency later.

[henryhsu, 2015-04-28 05:06:56]

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.cc (right):

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:246: << ", fall back to 16x16.";
On 2015/04/28 01:35:33, Pawel Osciak wrote:
> Please move this below next line and
> 
> s/<< ", fall back to 16x16.";/<< ", fall back to " <<
> min_resolution->ToString();/
> 
> Similarly for max above.

Done.

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.cc:247: min_resolution->SetSize(1920,
1088);
On 2015/04/28 01:35:33, Pawel Osciak wrote:
> s/1920, 1088/16, 16/

oops...Thank you.

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_device.h:112: void GetSupportedResolution(uint32_t
pixelformat, gfx::Size* max_resolution,
On 2015/04/28 01:35:33, Pawel Osciak wrote:
> Sorry for nitpicking, but could we have min before max please? Just to make it
> more natural. Thanks.

Done.

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1097913002/diff/160001/content/common/gpu/med...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:295: gfx::Size
min_resolution;
On 2015/04/28 01:35:33, Pawel Osciak wrote:
> We should add this to VEA::SupportedProfile for consistency later.

Hmm..Let us do it in next CL.

[henryhsu, 2015-05-12 08:35:26]

The CQ bit was checked by henryhsu@chromium.org

[henryhsu, 2015-05-12 08:35:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
wuchengli@chromium.org, xhwang@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1097913002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-12 08:35:47]

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

[commit-bot: I haz the power, 2015-05-12 13:05:02]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-12 13:06:04]

Patchset 11 (id:??) landed as
https://crrev.com/3d20a9d06d9dda27e069d07e9eb358a44633ca1c
Cr-Commit-Position: refs/heads/master@{#329395}