Add VDA supported profile to GPUInfo.

Add VDA supported profile to GPUInfo

Different CrOS devices have different supported decode profiles.
Add VDA supported profile to GPUInfo so this can be detected
in runtime. This is also the preparation of removing
--ignore-resolution-limits-for-accelerated-video-decode flag.

BUG=350197
TEST=Play a video on Exynos and Intel CrOS devices and make sure
Media.GpuVideoDecoderInitializeStatus is 0.

Committed: https://crrev.com/d118544f749bae18a188141edba216fed9163eba
Cr-Commit-Position: refs/heads/master@{#324596}

[henryhsu, 2014-12-18 10:49:41]

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

[henryhsu, 2014-12-18 10:49:42]

PTAL

[wuchengli, 2014-12-19 03:35:18]

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:98:
STATIC_ASSERT_ENUM_MATCH(VIDEO_CODEC_PROFILE_UNKNOWN);
Duplicating all these is bad. How about adding a
content/common/gpu/client/gpu_video_accelerator_util.h/cc?

STATIC_ASSERT_ENUM_MATCH...

VideoCodecProfile
GpuVideoAcceleratorUtil::ConvertGpuToMediaProfile(gpu::VideoCodecProfile) {
  return static_cast<media::VideoCodecProfile>(gpu_profiles[i].profile);
}

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:25: const int
kMaxDecodeFrameWidth = 1280;
Why this? Originally android VDA doesn't have this limitation. We should not
change it. How do you know the limitation of VDA is the same as Android VEA?
Remove this.

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:99: {
media::kCodecVP8, media::VP8PROFILE_ANY },
See line 127. Only VP8 is supported. Please combine the code there and this
function.

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:111:
media::MEDIA_CODEC_ENCODER)) {
This should be decoder. You need to test this on an actual Android device.

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:324: profile.profile =
media::VIDEO_CODEC_PROFILE_UNKNOWN;
We shouldn't return unknown. Find the right profile. Remove this.

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:332:
profiles.push_back(profile);
We cannot return unknown.

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:355:
static_cast<gpu::VideoCodecProfile>(media_profiles[i].profile);
This can also go to gpu_video_accelerator_unit.h/.cc.
gpu_video_accelerator_util.h can be in content/common/gpu/media instead.

Or maybe we want to move all ConvertMediaToGpuProfiles and
ConvertGpuToMediaProfiles to gpu_video_accelerator_util.h?

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:180: std::vector<VAConfigAttrib>
required_attribs = GetRequiredAttribs(kEncode);
This is only for encode. For decode, you have to pass kDecode.

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:187: va_profile, VAEntrypointEncSlice,
required_attribs)) {
For decode, you have to pass VAEntrypointVLD.

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

https://codereview.chromium.org/795633005/diff/60001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:139: if
(!IsCodedSizeSupported(config.coded_size())) {
Combine IsCodedSizeSupported and IsProfileSupported to IsConfigSupported(const
VideoDecoderConfig& config) here should be better. The loop is the same. Also,
we should not assign config_ = config before IsProfileSupported passes.

https://codereview.chromium.org/795633005/diff/60001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:163:
status_cb.Run(DECODER_ERROR_NOT_SUPPORTED);
PIPELINE_ERROR_INITIALIZATION_FAILED? Please find which error code makes the
most sense given not supported should be ruled out by IsCodedSizeSupported and
IsProfileSupported.

https://codereview.chromium.org/795633005/diff/60001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:604: for (size_t i = 0; i <
supported_profiles.size(); i++) {
You have to check the corresponding profile. You cannot return true on a
mismatched profile.

[wuchengli, 2014-12-19 11:21:57]

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/60001/content/common/gpu/media...
content/common/gpu/media/android_video_decode_accelerator.cc:93:
AndroidVideoDecodeAccelerator::GetSupportedProfiles() {
As discussed, it becomes a huge change to support GetSupportedProfiles in
Windows, Mac, and Android VDA (especially Windows). We don't need profiles to
remove --ignore-resolution-limits-for-accelerated-video-decode. The renderer
(gpu_video_decoder.cc) doesn't need it now. Let's use GetSupportedResolution and
see what piman says.

[wuchengli, 2014-12-22 06:18:42]

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:83:
GpuVideoDecodeAcceleratorHost::GetSupportedResolution() {
not used. Right? Remove this. Move ConvertGpuToMediaResolution to
renderer_gpu_video_accelerator_factories.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:89: return
resolution;
just return media::VideoDecodeAccelerator::SupportedResolution(). gfx::Size
constructor sets the default size to 0,0.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:43:
SupportedResolution GetSupportedResolution();
vda.h doesn't have this and this is not used. Right? Remove.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_encode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_encode_accelerator_host.h:57:
std::vector<media::VideoEncodeAccelerator::SupportedProfile>
nit: FYI. This is a override. So it's not ambiguous.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
Please check with Pawel if 4096x2160 is correct.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:150: // static
Shouldn't modify an unrelated file unless you need to change the logic. Revert
this file. Also I'd prefer to revert gpu_video_encode_accelerator.h/cc and
gpu_video_encode_accelerator_host.h/cc. This CL is about the decoder and those
files only have style changes.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.cc#n...
gpu/config/gpu_info.cc:23:
enumerator->BeginVideoDecodeAcceleratorSupportedResolution();
Not a vector. Remove BeginVideoDecodeAcceleratorSupportedResolution and
EndVideoDecodeAcceleratorSupportedResolution.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:58: // Specification of an decoding profile supported by a
hardware decoder.
s/an/a/. Not a profile. Comment should be updated.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:60: gfx::Size max;
min should be the first to be consistent with vda::SupportedResolution.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:246: virtual void
EndVideoDecodeAcceleratorSupportedResolution() = 0;
Remove these. Only a vector need them.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info_unit...
File gpu/config/gpu_info_unittest.cc (right):

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info_unit...
gpu/config/gpu_info_unittest.cc:38: gfx::Size(0, 0));
I don't think this can go wrong. gfx::Size sets them to 0,0 in the constructor.
Remove?

[henryhsu, 2014-12-22 08:34:14]

all done. PTAL

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:83:
GpuVideoDecodeAcceleratorHost::GetSupportedResolution() {
On 2014/12/22 06:18:41, wuchengli wrote:
> not used. Right? Remove this. Move ConvertGpuToMediaResolution to
> renderer_gpu_video_accelerator_factories.

Done.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:89: return
resolution;
On 2014/12/22 06:18:41, wuchengli wrote:
> just return media::VideoDecodeAccelerator::SupportedResolution(). gfx::Size
> constructor sets the default size to 0,0.

Not used anymore

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:43:
SupportedResolution GetSupportedResolution();
On 2014/12/22 06:18:42, wuchengli wrote:
> vda.h doesn't have this and this is not used. Right? Remove.

Done.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_video_encode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_video_encode_accelerator_host.h:57:
std::vector<media::VideoEncodeAccelerator::SupportedProfile>
On 2014/12/22 06:18:42, wuchengli wrote:
> nit: FYI. This is a override. So it's not ambiguous. 

Done.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
On 2014/12/22 06:18:42, wuchengli wrote:
> Please check with Pawel if 4096x2160 is correct.

Yes. 4k is 4096 x 2160.

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:150: // static
On 2014/12/22 06:18:42, wuchengli wrote:
> Shouldn't modify an unrelated file unless you need to change the logic. Revert
> this file. Also I'd prefer to revert gpu_video_encode_accelerator.h/cc and
> gpu_video_encode_accelerator_host.h/cc. This CL is about the decoder and those
> files only have style changes.

Done.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.cc#n...
gpu/config/gpu_info.cc:23:
enumerator->BeginVideoDecodeAcceleratorSupportedResolution();
On 2014/12/22 06:18:42, wuchengli wrote:
> Not a vector. Remove BeginVideoDecodeAcceleratorSupportedResolution and
> EndVideoDecodeAcceleratorSupportedResolution.

Done.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:58: // Specification of an decoding profile supported by a
hardware decoder.
On 2014/12/22 06:18:42, wuchengli wrote:
> s/an/a/. Not a profile. Comment should be updated.

Done.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:60: gfx::Size max;
On 2014/12/22 06:18:42, wuchengli wrote:
> min should be the first to be consistent with vda::SupportedResolution.

Done.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info.h#ne...
gpu/config/gpu_info.h:246: virtual void
EndVideoDecodeAcceleratorSupportedResolution() = 0;
On 2014/12/22 06:18:42, wuchengli wrote:
> Remove these. Only a vector need them.

Done.

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info_unit...
File gpu/config/gpu_info_unittest.cc (right):

https://codereview.chromium.org/795633005/diff/80001/gpu/config/gpu_info_unit...
gpu/config/gpu_info_unittest.cc:38: gfx::Size(0, 0));
On 2014/12/22 06:18:42, wuchengli wrote:
> I don't think this can go wrong. gfx::Size sets them to 0,0 in the
constructor.
> Remove?

Done.

[wuchengli, 2014-12-22 09:39:25]

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
On 2014/12/22 08:34:13, henryhsu wrote:
> On 2014/12/22 06:18:42, wuchengli wrote:
> > Please check with Pawel if 4096x2160 is correct.
> 
> Yes. 4k is 4096 x 2160.
I meant is 4096 x 2160 really the exact limit of both Intel and ARM chromebooks?
Please check the spec of exynos and nyan. Also find out the exact limit of
Intel.

https://codereview.chromium.org/795633005/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:13: #include
"gpu/config/gpu_info.h"
remove

https://codereview.chromium.org/795633005/diff/100001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/795633005/diff/100001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:266: return
ConvertGpuToMediaResolution( gpu_channel_host_->gpu_info()
Both GetVideoDecodeAcceleratorSupportedResolution and
ConvertGpuToMediaResolution are small functions. ConvertGpuToMediaResolution is
not used by other classes. You can just merge them, remove
ConvertGpuToMediaResolution and do it in
GetVideoDecodeAcceleratorSupportedResolution.

https://codereview.chromium.org/795633005/diff/100001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/100001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:22: const gpu::VideoDecodeAcceleratorSupportedResolution
resolution) {
const &

[henryhsu, 2014-12-23 02:52:37]

all done

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/80001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
On 2014/12/22 09:39:24, wuchengli wrote:
> On 2014/12/22 08:34:13, henryhsu wrote:
> > On 2014/12/22 06:18:42, wuchengli wrote:
> > > Please check with Pawel if 4096x2160 is correct.
> > 
> > Yes. 4k is 4096 x 2160.
> I meant is 4096 x 2160 really the exact limit of both Intel and ARM
chromebooks?
> Please check the spec of exynos and nyan. Also find out the exact limit of
> Intel.

Nyan series resolution is 3840x2160. So I think 4096x2160 is enough.

https://codereview.chromium.org/795633005/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:13: #include
"gpu/config/gpu_info.h"
On 2014/12/22 09:39:24, wuchengli wrote:
> remove

Done.

https://codereview.chromium.org/795633005/diff/100001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/795633005/diff/100001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:266: return
ConvertGpuToMediaResolution( gpu_channel_host_->gpu_info()
On 2014/12/22 09:39:24, wuchengli wrote:
> Both GetVideoDecodeAcceleratorSupportedResolution and
> ConvertGpuToMediaResolution are small functions. ConvertGpuToMediaResolution
is
> not used by other classes. You can just merge them, remove
> ConvertGpuToMediaResolution and do it in
> GetVideoDecodeAcceleratorSupportedResolution.

Done.

https://codereview.chromium.org/795633005/diff/100001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/100001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:22: const gpu::VideoDecodeAcceleratorSupportedResolution
resolution) {
On 2014/12/22 09:39:24, wuchengli wrote:
> const &

Done.

[wuchengli, 2014-12-23 03:37:20]

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:324:
resolution.min.SetSize(16, 16);
Moving this line to non-windows case (line 334) is more symmetric.

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:340:
CommandLine::ForCurrentProcess()->HasSwitch(
Please test this and make sure HasSwitch is working here in GPU process.

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:347: // For non-Windows
and non-ChromeOs platform.
Please add comments to explain kMaxDimension is not the actual limit and those
platforms do not support setting the precise values.

s/ChromeOs/ChromeOS/

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:261:
media_resolution.max = gpu_resolution.max;
nit: let's follow the declaration order. Assign min first and then max.

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:16: #include
"gpu/config/gpu_info.h"
remove

https://codereview.chromium.org/795633005/diff/120001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/120001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:20: void
EnumerateVideoDecodeAcceleratorSupportedResolution(
According to the style guide, the input parameter should be the first. Please
also help change the order of EnumerateGPUDevice and
EnumerateVideoEncodeAcceleratorSupportedProfile.

void EnumerateVideoDecodeAcceleratorSupportedResolution(const
gpu::VideoDecodeAcceleratorSupportedResolution& resolution,
gpu::GPUInfo::Enumerator* enumerator)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

[henryhsu, 2014-12-23 05:52:06]

all done

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:324:
resolution.min.SetSize(16, 16);
On 2014/12/23 03:37:20, wuchengli wrote:
> Moving this line to non-windows case (line 334) is more symmetric.

Done.

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:340:
CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/12/23 03:37:20, wuchengli wrote:
> Please test this and make sure HasSwitch is working here in GPU process.

Tested on peach_pi and this switch is 1.

https://codereview.chromium.org/795633005/diff/120001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:347: // For non-Windows
and non-ChromeOs platform.
On 2014/12/23 03:37:20, wuchengli wrote:
> Please add comments to explain kMaxDimension is not the actual limit and those
> platforms do not support setting the precise values.
> 
> s/ChromeOs/ChromeOS/

Done.

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:261:
media_resolution.max = gpu_resolution.max;
On 2014/12/23 03:37:20, wuchengli wrote:
> nit: let's follow the declaration order. Assign min first and then max.

Done.

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/120001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:16: #include
"gpu/config/gpu_info.h"
On 2014/12/23 03:37:20, wuchengli wrote:
> remove

Done.

https://codereview.chromium.org/795633005/diff/120001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/120001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:20: void
EnumerateVideoDecodeAcceleratorSupportedResolution(
On 2014/12/23 03:37:20, wuchengli wrote:
> According to the style guide, the input parameter should be the first. Please
> also help change the order of EnumerateGPUDevice and
> EnumerateVideoEncodeAcceleratorSupportedProfile.
> 
> void EnumerateVideoDecodeAcceleratorSupportedResolution(const
> gpu::VideoDecodeAcceleratorSupportedResolution& resolution,
> gpu::GPUInfo::Enumerator* enumerator)
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

Done.

[wuchengli, 2014-12-23 07:52:09]

Looking good except one comment. Please start adding owners for review. Also ask
sandersd for the maximum resolution for VtVDA on Mac.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
Can you let me know the exact max resolution of nyan, pi, and Intel CrOS again?

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:347: // specified. Set
kMaxDimension to pass the coded_size check in render
Non-Windows and non-ChromeOS are only Android and Mac. I don't think Android can
support more than 4k. For Mac, please ask sandersd for the correct values.
Setting kMaxDimension is still ugly.

[henryhsu, 2014-12-23 10:23:48]

henryhsu@chromium.org changed reviewers:
+ kenrb@chromium.org, piman@chromium.org, scherkus@chromium.org

[henryhsu, 2014-12-23 10:23:49]

kenrb@chromium.org: Please review changes in 

scherkus@chromium.org: Please review changes in 

piman@chromium.org: Please review changes in

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
On 2014/12/23 07:52:09, wuchengli wrote:
> Can you let me know the exact max resolution of nyan, pi, and Intel CrOS
again?

nyan and pi maximum resolution is 3840x2160. But Exynos 5 Octa 5422 can decode
up to 8192x8192. Pawel suggested to use 4096x2160.

[wuchengli, 2014-12-23 11:07:26]

On 2014/12/23 10:23:49, henryhsu wrote:
> mailto:kenrb@chromium.org: Please review changes in 
> 
> mailto:scherkus@chromium.org: Please review changes in 
> 
> mailto:piman@chromium.org: Please review changes in
> 
>
https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
> content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
> resolution.max.SetSize(4096, 2160);
> On 2014/12/23 07:52:09, wuchengli wrote:
> > Can you let me know the exact max resolution of nyan, pi, and Intel CrOS
> again?
> 
> nyan and pi maximum resolution is 3840x2160. But Exynos 5 Octa 5422 can decode
> up to 8192x8192. Pawel suggested to use 4096x2160.
- I don't understand. pi is 5422. You said pi max resolution 3840x2160 and 5422
can decode 8192x8192.
- Why are we using 4096x2160 when all devices that can decode 4k support only
3840x2160?

[piman, 2014-12-23 19:02:14]

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:331: #else
Does this #else clause also apply to mac? Should that be explicit?

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:341:
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55);
This is a horrible hack to base on the CPU (which can be completely independent
of decoder hardware). Can't we query the decoder API (v4l2/vaapi/...) for the
maximum size?

[Pawel Osciak, 2014-12-24 01:07:57]

On 2014/12/23 19:02:14, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
> content/common/gpu/media/gpu_video_decode_accelerator.cc:331: #else
> Does this #else clause also apply to mac? Should that be explicit?
> 
>
https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
> content/common/gpu/media/gpu_video_decode_accelerator.cc:341:
> ((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55);
> This is a horrible hack to base on the CPU (which can be completely
independent
> of decoder hardware). Can't we query the decoder API (v4l2/vaapi/...) for the
> maximum size?

We had this in for years, this was just moved around here. That said, we'd love
to query, but there is no way to do that for Intel driver at the moment. We've
been thinking about adding something, but it's not that easy, because detailed
specification on each of the tens if not hundreds of skews is not easy to get.
As for V4L2, we have a way, will be added in subsequent CLs.

[piman, 2014-12-24 01:27:18]

On Tue, Dec 23, 2014 at 5:07 PM, <posciak@chromium.org> wrote:

> On 2014/12/23 19:02:14, piman (Very slow to review) wrote:
>
> https://codereview.chromium.org/795633005/diff/140001/
> content/common/gpu/media/gpu_video_decode_accelerator.cc
>
>> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
>>
>
>
> https://codereview.chromium.org/795633005/diff/140001/
> content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode331
>
>> content/common/gpu/media/gpu_video_decode_accelerator.cc:331: #else
>> Does this #else clause also apply to mac? Should that be explicit?
>>
>
>
> https://codereview.chromium.org/795633005/diff/140001/
> content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode341
>
>> content/common/gpu/media/gpu_video_decode_accelerator.cc:341:
>> ((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55);
>> This is a horrible hack to base on the CPU (which can be completely
>>
> independent
>
>> of decoder hardware). Can't we query the decoder API (v4l2/vaapi/...) for
>> the
>> maximum size?
>>
>
> We had this in for years, this was just moved around here. That said, we'd
> love
> to query, but there is no way to do that for Intel driver at the moment.
> We've
> been thinking about adding something, but it's not that easy, because
> detailed
> specification on each of the tens if not hundreds of skews is not easy to
> get.
> As for V4L2, we have a way, will be added in subsequent CLs.
>

I don't think this makes any sense on !chromeos.
Even on chromeos, this seems bogus, if we use e.g. some discreet decoding
hardware. I think this should be fed by some use flag from the board if it
can't be queried.

We've had this discussion before about the encoder profiles...


> https://codereview.chromium.org/795633005/
>

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

[Pawel Osciak, 2014-12-24 01:35:12]

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:155:
IPC_STRUCT_TRAITS_BEGIN(gpu::VideoDecodeAcceleratorSupportedResolution)
Would it make sense to call this Capabilities and add profiles/formats here
later on as well to reduce the number of IPCs?

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
On 2014/12/23 10:23:49, henryhsu wrote:
> On 2014/12/23 07:52:09, wuchengli wrote:
> > Can you let me know the exact max resolution of nyan, pi, and Intel CrOS
> again?
> 
> nyan and pi maximum resolution is 3840x2160. But Exynos 5 Octa 5422 can decode
> up to 8192x8192. Pawel suggested to use 4096x2160.

Where is 3840x2160 coming from? If this is the case, then we can't use 4096.

As for 8192x8192 it's the upper limit of the resolution setting hardware blocks
can handle, but this is not at 30fps. So we should stay at 4k.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:346: // For non-Windows
and non-ChromeOS platform, the maximum resolution is not
s/platform/platforms/

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:350:
media::limits::kMaxDimension, media::limits::kMaxDimension);
This is too much. 32767 is way way too much :)
Looks like for other platforms we've been using 1920x1088 after all. Let's keep
it that way please.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:70:
GetSupportedResolution();
And this, perhaps make it into a more generic caps structure/call?

[Pawel Osciak, 2014-12-24 02:03:59]

On 2014/12/24 01:27:18, piman (Very slow to review) wrote:
> On Tue, Dec 23, 2014 at 5:07 PM, <mailto:posciak@chromium.org> wrote:
> 
> > On 2014/12/23 19:02:14, piman (Very slow to review) wrote:
> >
> > https://codereview.chromium.org/795633005/diff/140001/
> > content/common/gpu/media/gpu_video_decode_accelerator.cc
> >
> >> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/795633005/diff/140001/
> > content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode331
> >
> >> content/common/gpu/media/gpu_video_decode_accelerator.cc:331: #else
> >> Does this #else clause also apply to mac? Should that be explicit?
> >>
> >
> >
> > https://codereview.chromium.org/795633005/diff/140001/
> > content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode341
> >
> >> content/common/gpu/media/gpu_video_decode_accelerator.cc:341:
> >> ((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55);
> >> This is a horrible hack to base on the CPU (which can be completely
> >>
> > independent
> >
> >> of decoder hardware). Can't we query the decoder API (v4l2/vaapi/...) for
> >> the
> >> maximum size?
> >>
> >
> > We had this in for years, this was just moved around here. That said, we'd
> > love
> > to query, but there is no way to do that for Intel driver at the moment.
> > We've
> > been thinking about adding something, but it's not that easy, because
> > detailed
> > specification on each of the tens if not hundreds of skews is not easy to
> > get.
> > As for V4L2, we have a way, will be added in subsequent CLs.
> >
> 
> I don't think this makes any sense on !chromeos.

Matching CPUID? Platforms like Mac and Windows could use this too, since the
hardware is the same.

> Even on chromeos, this seems bogus, if we use e.g. some discreet decoding
> hardware. I think this should be fed by some use flag from the board if it
> can't be queried.

Henry is working on a followup CL to support multiple codec hardware right now,
which will address that.
And then all this resolution matching should go into VDA impls, I agree.

> We've had this discussion before about the encoder profiles...

For encoder, we query profiles from hardware, Intel driver was fixed.

> 
> > https://codereview.chromium.org/795633005/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[wuchengli, 2014-12-24 02:53:23]

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:155:
IPC_STRUCT_TRAITS_BEGIN(gpu::VideoDecodeAcceleratorSupportedResolution)
On 2014/12/24 01:35:11, Pawel Osciak wrote:
> Would it make sense to call this Capabilities and add profiles/formats here
> later on as well to reduce the number of IPCs?
I think the point is whether profiles can have different min and max
resolutions. If they can't, Capabilities is OK. If they can, SupportedProfile is
better and we can change the name when adding profiles in the future.
SupportedProfile will be more consistent with VEA.

[henryhsu, 2014-12-24 10:02:19]

henryhsu@chromium.org changed reviewers:
+ sandersd@chromium.org

[henryhsu, 2014-12-24 10:02:21]

Hi sandersd,
Could you help confirm what the maximum resolution is for mac? We moved some
code from gpu_video_decoder.cc to gpu_video_decoder_accelerator.cc. So we need
to set a maximum resolution for all mac devices to pass coded_size check in
render process.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:331: #else
On 2014/12/23 19:02:13, piman (Very slow to review) wrote:
> Does this #else clause also apply to mac? Should that be explicit?

Yes, include mac. I'll modify the below comment to specify mac.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:341:
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55);
On 2014/12/23 19:02:13, piman (Very slow to review) wrote:
> This is a horrible hack to base on the CPU (which can be completely
independent
> of decoder hardware). Can't we query the decoder API (v4l2/vaapi/...) for the
> maximum size?

We may keep this until v4l2 and vaapi support maximum resolution query.
We will do v4l2 part in the future. vaapi should wait library to support it.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:344:
resolution.max.SetSize(4096, 2160);
On 2014/12/24 01:35:11, Pawel Osciak wrote:
> On 2014/12/23 10:23:49, henryhsu wrote:
> > On 2014/12/23 07:52:09, wuchengli wrote:
> > > Can you let me know the exact max resolution of nyan, pi, and Intel CrOS
> > again?
> > 
> > nyan and pi maximum resolution is 3840x2160. But Exynos 5 Octa 5422 can
decode
> > up to 8192x8192. Pawel suggested to use 4096x2160.
> 
> Where is 3840x2160 coming from? If this is the case, then we can't use 4096.
> 
> As for 8192x8192 it's the upper limit of the resolution setting hardware
blocks
> can handle, but this is not at 30fps. So we should stay at 4k.

Oh sorry, 3840x2160 is the source resolution. I agree to stay at 4k here.

https://codereview.chromium.org/795633005/diff/140001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:350:
media::limits::kMaxDimension, media::limits::kMaxDimension);
On 2014/12/24 01:35:11, Pawel Osciak wrote:
> This is too much. 32767 is way way too much :)
> Looks like for other platforms we've been using 1920x1088 after all. Let's
keep
> it that way please.

In the past, we return true when resolution is less than 1920x1088. But we don't
check the resolution when hw_large_video_support is true. 

Hi sandersd, Could you help confirm what the maximum resolution is for mac?

[wuchengli, 2014-12-26 09:37:07]

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:90:
resolution.max.SetSize(1920, 1088);
Better to ask someone from chrome media team or Android team to review this.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:523:
DXVAVideoDecodeAccelerator::GetSupportedResolution() {
Be consistent with the order in the header file. Move this after
GetSurfaceInternalFormat.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:334: #else
What about ozone? Also, else should be NOT_IMPLEMENTED

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

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:210:
V4L2VideoDecodeAccelerator::GetSupportedResolution() {
Use the same order in the header file.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:213: //
V4L2VideoDecodeAccelerator.
Update the comment. This is already V4L2VDA.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:256:
media::VideoDecodeAccelerator::SupportedResolution
Move this to match the order in the header file.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:249: if
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
Mac only uses Intel and no ARM?

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.h:56: static
media::VideoDecodeAccelerator::SupportedResolution
Move this after Output(). Let's not mix the order of static and non-static
functions.

[henryhsu, 2014-12-26 10:25:17]

henryhsu@chromium.org changed reviewers:
+ qinmin@chromium.org

[henryhsu, 2014-12-26 10:25:19]

Hi qinmin,
Could you help review android part? Thank you.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:90:
resolution.max.SetSize(1920, 1088);
On 2014/12/26 09:37:07, wuchengli wrote:
> Better to ask someone from chrome media team or Android team to review this.

Add qinmin@chromium.org as reviewer

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:523:
DXVAVideoDecodeAccelerator::GetSupportedResolution() {
On 2014/12/26 09:37:07, wuchengli wrote:
> Be consistent with the order in the header file. Move this after
> GetSurfaceInternalFormat.

Done.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:334: #else
On 2014/12/26 09:37:07, wuchengli wrote:
> What about ozone? Also, else should be NOT_IMPLEMENTED

Done.

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

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:210:
V4L2VideoDecodeAccelerator::GetSupportedResolution() {
On 2014/12/26 09:37:07, wuchengli wrote:
> Use the same order in the header file.

Done.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:213: //
V4L2VideoDecodeAccelerator.
On 2014/12/26 09:37:07, wuchengli wrote:
> Update the comment. This is already V4L2VDA.

Done.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:256:
media::VideoDecodeAccelerator::SupportedResolution
On 2014/12/26 09:37:07, wuchengli wrote:
> Move this to match the order in the header file.

Done.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:249: if
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
On 2014/12/26 09:37:07, wuchengli wrote:
> Mac only uses Intel and no ARM?

keep this first and wait sandersd to confirm maximum resolution.

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/160001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.h:56: static
media::VideoDecodeAccelerator::SupportedResolution
On 2014/12/26 09:37:07, wuchengli wrote:
> Move this after Output(). Let's not mix the order of static and non-static
> functions.

Done.

[kenrb, 2015-01-02 14:26:57]

ipc lgtm

[qinmin, 2015-01-02 22:47:10]

lgtm on android_video_decoder_accelerator

[henryhsu, 2015-01-05 10:48:50]

Hi piman and sandersd,
Please take a look.
Thanks.

[sandersd (OOO until July 31), 2015-01-05 19:12:04]

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:926:
resolution.min.SetSize(16, 16);
Below 480P, VideoToolbox uses an internal software decoder. 480P may be a good
minimum for VT.

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:928: // Ivy Bridge+
platforms can support more than 1920x1080.
As far as I know, this has no bearing on the resolution supported by
VideoToolbox (I don't know what does either, it's not documented and I don't
have all the test hardware available yet).

[piman, 2015-01-05 21:41:07]

On Tue, Dec 23, 2014 at 6:03 PM, <posciak@chromium.org> wrote:

> On 2014/12/24 01:27:18, piman (Very slow to review) wrote:
>
>  On Tue, Dec 23, 2014 at 5:07 PM, <mailto:posciak@chromium.org> wrote:
>>
>
>  > On 2014/12/23 19:02:14, piman (Very slow to review) wrote:
>> >
>> > https://codereview.chromium.org/795633005/diff/140001/
>> > content/common/gpu/media/gpu_video_decode_accelerator.cc
>> >
>> >> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/795633005/diff/140001/
>> > content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode331
>> >
>> >> content/common/gpu/media/gpu_video_decode_accelerator.cc:331: #else
>> >> Does this #else clause also apply to mac? Should that be explicit?
>> >>
>> >
>> >
>> > https://codereview.chromium.org/795633005/diff/140001/
>> > content/common/gpu/media/gpu_video_decode_accelerator.cc#newcode341
>> >
>> >> content/common/gpu/media/gpu_video_decode_accelerator.cc:341:
>> >> ((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55);
>> >> This is a horrible hack to base on the CPU (which can be completely
>> >>
>> > independent
>> >
>> >> of decoder hardware). Can't we query the decoder API (v4l2/vaapi/...)
>> for
>> >> the
>> >> maximum size?
>> >>
>> >
>> > We had this in for years, this was just moved around here. That said,
>> we'd
>> > love
>> > to query, but there is no way to do that for Intel driver at the moment.
>> > We've
>> > been thinking about adding something, but it's not that easy, because
>> > detailed
>> > specification on each of the tens if not hundreds of skews is not easy
>> to
>> > get.
>> > As for V4L2, we have a way, will be added in subsequent CLs.
>> >
>>
>
>  I don't think this makes any sense on !chromeos.
>>
>
> Matching CPUID? Platforms like Mac and Windows could use this too, since
> the
> hardware is the same.


Who says you're using the CPU's integrated decoder as opposed to, say, the
discrete GPU's?


>
>  Even on chromeos, this seems bogus, if we use e.g. some discreet decoding
>> hardware. I think this should be fed by some use flag from the board if it
>> can't be queried.
>>
>
> Henry is working on a followup CL to support multiple codec hardware right
> now,
> which will address that.
> And then all this resolution matching should go into VDA impls, I agree.
>
>  We've had this discussion before about the encoder profiles...
>>
>
> For encoder, we query profiles from hardware, Intel driver was fixed.
>
>
>  > https://codereview.chromium.org/795633005/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/795633005/
>

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

[piman, 2015-01-05 21:45:22]

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:934: if
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
NAK. Please pass this on the command line, or have VAAPI provide a way to query
this.

[henryhsu, 2015-01-13 05:40:44]

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:934: if
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
On 2015/01/05 21:45:22, piman (Very slow to review) wrote:
> NAK. Please pass this on the command line, or have VAAPI provide a way to
query
> this.

We have many devices using VAAPI and VAAPI doesn't support to query supported
resolution. Ivy Bridge+ supports 4K decode. So I think to use CPU detection only
for x86 CrOS is fine and it is used for a long time. We can keep it until VAAPI
supports resolution querying.

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:926:
resolution.min.SetSize(16, 16);
On 2015/01/05 19:12:04, sandersd wrote:
> Below 480P, VideoToolbox uses an internal software decoder. 480P may be a good
> minimum for VT.

Done.

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:928: // Ivy Bridge+
platforms can support more than 1920x1080.
On 2015/01/05 19:12:04, sandersd wrote:
> As far as I know, this has no bearing on the resolution supported by
> VideoToolbox (I don't know what does either, it's not documented and I don't
> have all the test hardware available yet).

The code of cpu detection is for chromeos. Could you give a suggestion to set
maximum resolution for mac? 1080p or 4k?
If I set maximum resolution to 4k but hardware cannot handle 4k, what happen
with VideoToolbox? Will it use software decode or try another resolution?

[piman, 2015-01-13 07:33:37]

On Mon, Jan 12, 2015 at 9:40 PM, <henryhsu@chromium.org> wrote:

>
> https://codereview.chromium.org/795633005/diff/180001/
> content/common/gpu/media/vaapi_video_decode_accelerator.cc
> File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):
>
> https://codereview.chromium.org/795633005/diff/180001/
> content/common/gpu/media/vaapi_video_decode_accelerator.cc#newcode934
> content/common/gpu/media/vaapi_video_decode_accelerator.cc:934: if
> ((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
> On 2015/01/05 21:45:22, piman (Very slow to review) wrote:
>
>> NAK. Please pass this on the command line, or have VAAPI provide a way
>>
> to query
>
>> this.
>>
>
> We have many devices using VAAPI and VAAPI doesn't support to query
> supported resolution. Ivy Bridge+ supports 4K decode. So I think to use
> CPU detection only for x86 CrOS is fine and it is used for a long time.
> We can keep it until VAAPI supports resolution querying.
>

Sorry. This code doesn't belong here.


>
> https://codereview.chromium.org/795633005/diff/180001/
> content/common/gpu/media/vt_video_decode_accelerator.cc
> File content/common/gpu/media/vt_video_decode_accelerator.cc (right):
>
> https://codereview.chromium.org/795633005/diff/180001/
> content/common/gpu/media/vt_video_decode_accelerator.cc#newcode926
> content/common/gpu/media/vt_video_decode_accelerator.cc:926:
> resolution.min.SetSize(16, 16);
> On 2015/01/05 19:12:04, sandersd wrote:
>
>> Below 480P, VideoToolbox uses an internal software decoder. 480P may
>>
> be a good
>
>> minimum for VT.
>>
>
> Done.
>
> https://codereview.chromium.org/795633005/diff/180001/
> content/common/gpu/media/vt_video_decode_accelerator.cc#newcode928
> content/common/gpu/media/vt_video_decode_accelerator.cc:928: // Ivy
> Bridge+ platforms can support more than 1920x1080.
> On 2015/01/05 19:12:04, sandersd wrote:
>
>> As far as I know, this has no bearing on the resolution supported by
>> VideoToolbox (I don't know what does either, it's not documented and I
>>
> don't
>
>> have all the test hardware available yet).
>>
>
> The code of cpu detection is for chromeos. Could you give a suggestion
> to set maximum resolution for mac? 1080p or 4k?
> If I set maximum resolution to 4k but hardware cannot handle 4k, what
> happen with VideoToolbox? Will it use software decode or try another
> resolution?
>
> https://codereview.chromium.org/795633005/
>

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

[sandersd (OOO until July 31), 2015-01-13 18:02:15]

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:926:
resolution.min.SetSize(16, 16);
On 2015/01/13 05:40:44, henryhsu wrote:
> On 2015/01/05 19:12:04, sandersd wrote:
> > Below 480P, VideoToolbox uses an internal software decoder. 480P may be a
good
> > minimum for VT.
> 
> Done.

We have done additional testing, and most hardware is actually using 360P as the
cutoff. I don't see much difference between the two, but let's go with 360P for
now (now being the period of time before this hits stable, I'll re-evaluate
before launch).

https://codereview.chromium.org/795633005/diff/180001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:928: // Ivy Bridge+
platforms can support more than 1920x1080.
On 2015/01/13 05:40:44, henryhsu wrote:
> On 2015/01/05 19:12:04, sandersd wrote:
> > As far as I know, this has no bearing on the resolution supported by
> > VideoToolbox (I don't know what does either, it's not documented and I don't
> > have all the test hardware available yet).
> 
> The code of cpu detection is for chromeos. Could you give a suggestion to set
> maximum resolution for mac? 1080p or 4k?
> If I set maximum resolution to 4k but hardware cannot handle 4k, what happen
> with VideoToolbox? Will it use software decode or try another resolution?

I'm not certain, all the hardware we have tested handled 4k just fine. Let's go
with 4k for now.

[robert.bradford, 2015-01-27 12:27:58]

https://codereview.chromium.org/795633005/diff/220001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/220001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:749: if
((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
I agree with piman@ here. This seems very dangerous to me.

There is a VaSurfaceAttribMaxWidth & VaSurfaceAttribMaxHeight which are
unfortunately not implemented in the intel driver but I will try and garner some
support for implementing.

[wuchengli, 2015-01-27 12:55:53]

On 2015/01/27 12:27:58, robert.bradford wrote:
>
https://codereview.chromium.org/795633005/diff/220001/content/common/gpu/medi...
> File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/795633005/diff/220001/content/common/gpu/medi...
> content/common/gpu/media/vaapi_video_decode_accelerator.cc:749: if
> ((cpu.vendor_name() == "GenuineIntel") && cpu.model() >= 55)
> I agree with piman@ here. This seems very dangerous to me.
> 
> There is a VaSurfaceAttribMaxWidth & VaSurfaceAttribMaxHeight which are
> unfortunately not implemented in the intel driver but I will try and garner
some
> support for implementing.
Henry is adding the max resolution support in libva and libva-intel-driver.
Please review them if you are interested.
https://chromium-review.googlesource.com/#/c/242783/
https://codereview.chromium.org/872623002/

[henryhsu, 2015-03-14 09:30:16]

Since vaapi can query profile and resolution, we decide to change
GetSupportedResolution to GetSupportedProfile for decoder. This change is only
for CrOS. For other platforms, we still keep the resolution checking in
gpu_video_decoder.cc instead of modifying each VDA of WinOS, Android, and MacOS.
And we add a variable named support_query_profile in decoder profile structure
to indicate that the platform can get supported profile for decoder or not.

Please take a look.

[wuchengli, 2015-03-16 02:53:20]

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

https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:237: case
V4L2_PIX_FMT_VP8:
Need VP9.

https://codereview.chromium.org/795633005/diff/240001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/240001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:65:
GetVideoDecodeAcceleratorSupportedProfiles() override;
Use scoped_ptr or add a boolean to indicate if the supported profiles is
supported.

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

https://codereview.chromium.org/795633005/diff/240001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:600: coded_size, gfx::Size(640, 480),
gfx::Size(1920, 1088));
I prefer separating the changes of other platforms to another CL. So this main
CL won't be reverted if it breaks Mac.

https://codereview.chromium.org/795633005/diff/240001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/240001/media/video/video_decod...
media/video/video_decode_accelerator.h:28: gfx::Size max_resolution;
Need documentation. And explain this is inclusive.

https://codereview.chromium.org/795633005/diff/240001/media/video/video_decod...
media/video/video_decode_accelerator.h:30: bool support_query_profile;
This should not be associated with every SupportedProfile. Put this outside
SupportedProfile in gpu_info.h.

[Pawel Osciak, 2015-03-16 03:02:50]

Please also add this to V4L2SliceVDA. Thanks.

[henryhsu, 2015-03-16 03:08:01]

On 2015/03/16 03:02:50, Pawel Osciak wrote:
> Please also add this to V4L2SliceVDA. Thanks.

V4L2SliceVDA::GetSupportedProfiles is exactly the same as V4L2VDA.
Do we need a copy of this function?
In GetSupportedProfiles in gpu_video_decode_accelerator.cc, I think to add the
profiles of V4L2VDA is enough.
Thought?

[Pawel Osciak, 2015-03-16 03:26:39]

On 2015/03/16 03:08:01, henryhsu wrote:
> On 2015/03/16 03:02:50, Pawel Osciak wrote:
> > Please also add this to V4L2SliceVDA. Thanks.
> 
> V4L2SliceVDA::GetSupportedProfiles is exactly the same as V4L2VDA.

Not exactly, it supports different H264 and VP8 formats than V4L2VDA.

> Do we need a copy of this function?

Yes, because 1) the formats are different and 2) we don't have to have both on
all systems always (even though now we do).

> In GetSupportedProfiles in gpu_video_decode_accelerator.cc, I think to add the
> profiles of V4L2VDA is enough.
> Thought?

[wuchengli, 2015-03-16 03:30:10]

https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:395:
GpuVideoDecodeAccelerator::GetSupportedProfiles() {
You need to combine the supported profiles from different VDA. Ask Pawel how to
de-dup. For example, does driver make sure one profile is supported on one VDA?
Or does GVDA only append the profile from the first VDA that supports it?

[Pawel Osciak, 2015-03-16 03:35:46]

On 2015/03/16 03:30:10, wuchengli wrote:
>
https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
> File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
> content/common/gpu/media/gpu_video_decode_accelerator.cc:395:
> GpuVideoDecodeAccelerator::GetSupportedProfiles() {
> You need to combine the supported profiles from different VDA. Ask Pawel how
to
> de-dup. For example, does driver make sure one profile is supported on one
VDA?
> Or does GVDA only append the profile from the first VDA that supports it?

I think it should be enough to just do what we are doing right now, i.e.
instantiate VDAs one by one in the order we have now. This would only have to
ensure each VideoCodecProfile is inserted only once into the list of supported
profiles.

[henryhsu, 2015-03-16 08:31:18]

all done. PTAL

https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:395:
GpuVideoDecodeAccelerator::GetSupportedProfiles() {
On 2015/03/16 03:30:10, wuchengli wrote:
> You need to combine the supported profiles from different VDA. Ask Pawel how
to
> de-dup. For example, does driver make sure one profile is supported on one
VDA?
> Or does GVDA only append the profile from the first VDA that supports it?

Done. ConvertMediaToGpuDecodeProfiles function also eliminates duplicate
profiles

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

https://codereview.chromium.org/795633005/diff/240001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:237: case
V4L2_PIX_FMT_VP8:
On 2015/03/16 02:53:20, wuchengli wrote:
> Need VP9.

Done.

https://codereview.chromium.org/795633005/diff/240001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/240001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:65:
GetVideoDecodeAcceleratorSupportedProfiles() override;
On 2015/03/16 02:53:20, wuchengli wrote:
> Use scoped_ptr or add a boolean to indicate if the supported profiles is
> supported.

Done.

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

https://codereview.chromium.org/795633005/diff/240001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:600: coded_size, gfx::Size(640, 480),
gfx::Size(1920, 1088));
On 2015/03/16 02:53:20, wuchengli wrote:
> I prefer separating the changes of other platforms to another CL. So this main
> CL won't be reverted if it breaks Mac.

Done.

https://codereview.chromium.org/795633005/diff/240001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/240001/media/video/video_decod...
media/video/video_decode_accelerator.h:28: gfx::Size max_resolution;
On 2015/03/16 02:53:20, wuchengli wrote:
> Need documentation. And explain this is inclusive.

Done.

https://codereview.chromium.org/795633005/diff/240001/media/video/video_decod...
media/video/video_decode_accelerator.h:30: bool support_query_profile;
On 2015/03/16 02:53:20, wuchengli wrote:
> This should not be associated with every SupportedProfile. Put this outside
> SupportedProfile in gpu_info.h.

Done.

[wuchengli, 2015-03-18 08:02:38]

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:12: #include
"content/common/gpu/media/gpu_video_accelerator_util.h"
not needed?

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:13: #include
"gpu/config/gpu_info.h"
This shouldn't be needed.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:157: IPC_STRUCT_TRAITS_MEMBER(min_resolution)
max_resolution should be before min_resolution to be consistent with your other
files.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
remove (c). http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:54: for (size_t i = 0; i
< media_profiles.size(); i++) {
const auto&. Same for other for loops.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
remove (c)

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:18: static
std::vector<media::VideoDecodeAccelerator::SupportedProfile>
Are all four functions used?

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:22: // Convert media
profile to gpu profile and eliminate duplicate profile
s/Convert media/Convert decoder media/
s/profile/profiles/g for all "profile"

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:23: // element for
decoder.
s/element//. I don't think using "element" is more clear.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:29:
ConvertGpuToMediaEncodeProfiles(const std::vector<
Add function comment.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:397:
std::vector<media::VideoDecodeAccelerator::SupportedProfile> vda_profiles;
Move this inside #if defined(OS_CHROMEOS)

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:412: #endif //
defined(OS_CHROMEOS)
This looks as if line 411 is OS_CHROMEOS. How about moving
*support_query_profile = false; to the beginning of the function (line 396).

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:74:
GetSupportedProfiles(bool* support_query_profile);
Returning bool should be more common. Make
*vector<gpu::VideoDecodeAcceleratorSupportedProfile> as a function parameter.

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

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:230: case V4L2_PIX_FMT_H264:
As discussed, SliceVDA supports V4L2_PIX_FMT_VP8_FRAME and
V4L2_PIX_FMT_H264_SLICE. V4L2VDA supports V4L2_PIX_FMT_VP8, V4L2_PIX_FMT_H264,
and V4L2_PIX_FMT_VP9. GetSupportedDecodeProfiles should be implemented
differently in SliceVDA and V4L2VDA.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:46:
GetSupportedProfiles();
This function is not in media::VideoDecodeAccelerator. Move it after
CanDecodeOnIOThread() or before V4L2SliceVideoDecodeAccelerator().

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.h:89: static
std::vector<media::VideoDecodeAccelerator::SupportedProfile>
Move this after CanDecodeOnIOThread().

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

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:290: return
V4L2Device::GetSupportedEncodeProfiles();
Since VDA will have their own GetSupportedProfiles, GetSupportedEncodeProfiles
doesn't need to be moved to v4l2_device.cc. Revert the change.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.h:57: static
std::vector<media::VideoDecodeAccelerator::SupportedProfile>
Move it after CanDecodeOnIOThread.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.cc:204:
VaapiWrapper::GetSupportedDecodeProfiles() {
check switch::kDisableAcceleratedVideoDecode

https://codereview.chromium.org/795633005/diff/260001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/795633005/diff/260001/content/content_common....
content/content_common.gypi:331:
'common/gpu/media/gpu_video_accelerator_util.h',
Also update gn build file.

https://codereview.chromium.org/795633005/diff/260001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:65:
GetVideoDecodeAcceleratorSupportedProfiles(bool* support_query_profile)
bool
GetVideoDecodeAcceleratorSupportedProfiles(std::vector<media::VideoDecodeAccelerator::SupportedProfile>
*profiles) is better.

Change the other to bool
GetVideoEncodeAcceleratorSupportedProfiles(std::vector<media::VideoEncodeAccelerator::SupportedProfile>
*profiles) for consistency.

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:29: enumerator->AddBool("supportQueryProfile",
profile.support_query_profile);
remove

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:116: #endif
bool support_query_video_decode_profile;

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:63: bool support_query_profile;
Remove this. support_query_profile shouldn't be associated with every profile.

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:218: bool support_query_video_decode_profile;
s/profile/profiles/. Same for other places about this name.

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

https://codereview.chromium.org/795633005/diff/260001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:588: supported_profile.max_resolution)) {
Can you check with Pawel if max_resolution indicates coded size or visible size?
Thanks.

https://codereview.chromium.org/795633005/diff/260001/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/260001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:69: virtual
std::vector<VideoDecodeAccelerator::SupportedProfile>
bool
GetVideoDecodeAcceleratorSupportedProfiles(std::vector<VideoDecodeAccelerator::SupportedProfile>
*).

void
GetVideoEncodeAcceleratorSupportedProfiles(std::vector<VideoEncodeAccelerator::SupportedProfile>
*);

[henryhsu, 2015-03-18 11:06:10]

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:12: #include
"content/common/gpu/media/gpu_video_accelerator_util.h"
On 2015/03/18 08:02:36, wuchengli wrote:
> not needed?

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:13: #include
"gpu/config/gpu_info.h"
On 2015/03/18 08:02:37, wuchengli wrote:
> This shouldn't be needed.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:157: IPC_STRUCT_TRAITS_MEMBER(min_resolution)
On 2015/03/18 08:02:37, wuchengli wrote:
> max_resolution should be before min_resolution to be consistent with your
other
> files.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/03/18 08:02:37, wuchengli wrote:
> remove (c). http://www.chromium.org/developers/coding-style

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:54: for (size_t i = 0; i
< media_profiles.size(); i++) {
On 2015/03/18 08:02:37, wuchengli wrote:
> const auto&. Same for other for loops.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:1: // Copyright (c) 2015
The Chromium Authors. All rights reserved.
On 2015/03/18 08:02:37, wuchengli wrote:
> remove (c)

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:18: static
std::vector<media::VideoDecodeAccelerator::SupportedProfile>
On 2015/03/18 08:02:37, wuchengli wrote:
> Are all four functions used?

Yes.
ConvertGpuToMediaDecodeProfiles is used in
renderer_gpu_video_accelerator_factories.cc.
ConvertGpuToMediaEncodeProfiles is used in
renderer_gpu_video_accelerator_factories.cc and
gpu_video_encode_accelerator_host.cc.
ConvertMediaToGpuDecodeProfiles is used in gpu_video_decode_accelerator.cc.
ConvertMediaToGpuEncodeProfiles is used in gpu_video_encode_accelerator.cc.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:22: // Convert media
profile to gpu profile and eliminate duplicate profile
On 2015/03/18 08:02:37, wuchengli wrote:
> s/Convert media/Convert decoder media/
> s/profile/profiles/g for all "profile"

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:23: // element for
decoder.
On 2015/03/18 08:02:37, wuchengli wrote:
> s/element//. I don't think using "element" is more clear.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:29:
ConvertGpuToMediaEncodeProfiles(const std::vector<
On 2015/03/18 08:02:37, wuchengli wrote:
> Add function comment.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:397:
std::vector<media::VideoDecodeAccelerator::SupportedProfile> vda_profiles;
On 2015/03/18 08:02:37, wuchengli wrote:
> Move this inside #if defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:412: #endif //
defined(OS_CHROMEOS)
On 2015/03/18 08:02:37, wuchengli wrote:
> This looks as if line 411 is OS_CHROMEOS. How about moving
> *support_query_profile = false; to the beginning of the function (line 396).

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:74:
GetSupportedProfiles(bool* support_query_profile);
On 2015/03/18 08:02:37, wuchengli wrote:
> Returning bool should be more common. Make
> *vector<gpu::VideoDecodeAcceleratorSupportedProfile> as a function parameter.

Then we should change GetSupportedProfiles in encoder also for consistency.

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

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_device.cc:230: case V4L2_PIX_FMT_H264:
On 2015/03/18 08:02:37, wuchengli wrote:
> As discussed, SliceVDA supports V4L2_PIX_FMT_VP8_FRAME and
> V4L2_PIX_FMT_H264_SLICE. V4L2VDA supports V4L2_PIX_FMT_VP8, V4L2_PIX_FMT_H264,
> and V4L2_PIX_FMT_VP9. GetSupportedDecodeProfiles should be implemented
> differently in SliceVDA and V4L2VDA.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:46:
GetSupportedProfiles();
On 2015/03/18 08:02:37, wuchengli wrote:
> This function is not in media::VideoDecodeAccelerator. Move it after
> CanDecodeOnIOThread() or before V4L2SliceVideoDecodeAccelerator().

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.h:89: static
std::vector<media::VideoDecodeAccelerator::SupportedProfile>
On 2015/03/18 08:02:37, wuchengli wrote:
> Move this after CanDecodeOnIOThread().

Done.

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

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_encode_accelerator.cc:290: return
V4L2Device::GetSupportedEncodeProfiles();
On 2015/03/18 08:02:37, wuchengli wrote:
> Since VDA will have their own GetSupportedProfiles, GetSupportedEncodeProfiles
> doesn't need to be moved to v4l2_device.cc. Revert the change.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.h:57: static
std::vector<media::VideoDecodeAccelerator::SupportedProfile>
On 2015/03/18 08:02:37, wuchengli wrote:
> Move it after CanDecodeOnIOThread.

Done.

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/260001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.cc:204:
VaapiWrapper::GetSupportedDecodeProfiles() {
On 2015/03/18 08:02:38, wuchengli wrote:
> check switch::kDisableAcceleratedVideoDecode

Done.

https://codereview.chromium.org/795633005/diff/260001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/795633005/diff/260001/content/content_common....
content/content_common.gypi:331:
'common/gpu/media/gpu_video_accelerator_util.h',
On 2015/03/18 08:02:38, wuchengli wrote:
> Also update gn build file.

We don't have a gn file including these file names.

https://codereview.chromium.org/795633005/diff/260001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/260001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.h:65:
GetVideoDecodeAcceleratorSupportedProfiles(bool* support_query_profile)
On 2015/03/18 08:02:38, wuchengli wrote:
> bool
>
GetVideoDecodeAcceleratorSupportedProfiles(std::vector<media::VideoDecodeAccelerator::SupportedProfile>
> *profiles) is better.
> 
> Change the other to bool
>
GetVideoEncodeAcceleratorSupportedProfiles(std::vector<media::VideoEncodeAccelerator::SupportedProfile>
> *profiles) for consistency.

Done.

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:29: enumerator->AddBool("supportQueryProfile",
profile.support_query_profile);
On 2015/03/18 08:02:38, wuchengli wrote:
> remove

Done.

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.cc#...
gpu/config/gpu_info.cc:116: #endif
On 2015/03/18 08:02:38, wuchengli wrote:
> bool support_query_video_decode_profile;

Done.

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:63: bool support_query_profile;
On 2015/03/18 08:02:38, wuchengli wrote:
> Remove this. support_query_profile shouldn't be associated with every profile.

Done.

https://codereview.chromium.org/795633005/diff/260001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:218: bool support_query_video_decode_profile;
On 2015/03/18 08:02:38, wuchengli wrote:
> s/profile/profiles/. Same for other places about this name.

Done.

https://codereview.chromium.org/795633005/diff/260001/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/260001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:69: virtual
std::vector<VideoDecodeAccelerator::SupportedProfile>
On 2015/03/18 08:02:38, wuchengli wrote:
> bool
>
GetVideoDecodeAcceleratorSupportedProfiles(std::vector<VideoDecodeAccelerator::SupportedProfile>
> *).
> 
> void
>
GetVideoEncodeAcceleratorSupportedProfiles(std::vector<VideoEncodeAccelerator::SupportedProfile>
> *);

Done.

[wuchengli, 2015-03-19 06:19:25]

Looking good. Only small comments.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.cc (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:61: duplicate = true;
I prefer moving out the logic of removing duplicates. So all four functions here
can be symmetric. You can implement a
InsertWithoutDuplicate(vector<VideoDecodeAccelerator::SupportedProfile>,
std::vector<VideoDecodeAccelerator::SupportedProfile>) in
gpu_video_decode_accelerator.cc. Replace profiles.insert(profiles.end(),
vda_profiles.begin(), vda_profiles.end()); with InsertWithoutDuplicate() in line
404 of gpu_video_decode_accelerator.cc.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:24: // profiles for
decoder.
nit: s/for decoder//. Having decoder at the beginning should be clear enough.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:35: // profiles for
encoder.
s/for encoder//

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:276:
std::vector<GpuVideoDecodeAccelerator::CreateVDAFp> create_vda_fps;
Add comment to say the insert order here must be the same as
GetSupportedProfiles.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:401:
support_query_profile = true;
Add comment to say the insert order here must be the same as CreateVDAFps.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:414:
GpuVideoAcceleratorUtil::ConvertMediaToGpuDecodeProfiles(profiles);
We shouldn't touch |supported_profiles| if this function returns false. I think
setting |supported_profiles| to empty vector implies no profiles is supported.
Move this line to line 411.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:72: //
|supported_profiles|. Return true if platform supports to query decoder
Explain |supported_profiles| won't have duplicate when two VDA support the same
codec profile.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_encode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.h:62: //
|supported_profiles|.
Explain |supported_profiles| won't have duplicate when two VEA support the same
codec profile.

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

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:211:
V4L2VideoDecodeAccelerator::GetSupportedProfiles() {
Move this after CanDecodeOnIOThread to be consistent with the order in the
header file.

https://codereview.chromium.org/795633005/diff/320001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/795633005/diff/320001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:206:
*supported_profiles =
Prefer not to touch |support_profiles| if support_query_video_decode_profiles is
false.

https://codereview.chromium.org/795633005/diff/320001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/320001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:216: // Whether the platform supports to query video
decoder profiles.
Need more explanation about the relationship between
|support_query_video_decode_profiles| and
|video_decode_accelerator_supported_profiles|. Add something like "If true,
|video_decode_accelerator_supported_profiles| is the profiles supported by the
platform. If false, it is invalid."

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.cc (left):

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:121: ((cpu.vendor_name() == "GenuineIntel")
&& cpu.model() >= 55);
One concern. Does Mac accidentally use this and rely on this behavior?

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

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:580: bool support_query_profile =
s/profile/profiles/

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:586: IsCodedSizeSupported(coded_size,
Profile won't duplicate. So move IsCodedSizeSupported out of if clause. 

if (profile == supported_profile.profile) {
  return IsCodedSizeSupported(coded_size, ...);
}

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.h:131: bool IsProfileSupported(const
VideoCodecProfile& profile,
VideoCodecProfile is just an enum. Use VideoCodecProfile directly should be
enough.

https://codereview.chromium.org/795633005/diff/320001/media/renderers/mock_gp...
File media/renderers/mock_gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/320001/media/renderers/mock_gp...
media/renderers/mock_gpu_video_accelerator_factories.h:44:
MOCK_METHOD0(GetVideoEncodeAcceleratorSupportedProfiles,
This two should be updated. Try building the test.

[henryhsu, 2015-03-19 10:00:33]

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.cc (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.cc:61: duplicate = true;
On 2015/03/19 06:19:24, wuchengli wrote:
> I prefer moving out the logic of removing duplicates. So all four functions
here
> can be symmetric. You can implement a
> InsertWithoutDuplicate(vector<VideoDecodeAccelerator::SupportedProfile>,
> std::vector<VideoDecodeAccelerator::SupportedProfile>) in
> gpu_video_decode_accelerator.cc. Replace profiles.insert(profiles.end(),
> vda_profiles.begin(), vda_profiles.end()); with InsertWithoutDuplicate() in
line
> 404 of gpu_video_decode_accelerator.cc.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:24: // profiles for
decoder.
On 2015/03/19 06:19:24, wuchengli wrote:
> nit: s/for decoder//. Having decoder at the beginning should be clear enough.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:35: // profiles for
encoder.
On 2015/03/19 06:19:24, wuchengli wrote:
> s/for encoder//

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:276:
std::vector<GpuVideoDecodeAccelerator::CreateVDAFp> create_vda_fps;
On 2015/03/19 06:19:25, wuchengli wrote:
> Add comment to say the insert order here must be the same as
> GetSupportedProfiles.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:401:
support_query_profile = true;
On 2015/03/19 06:19:25, wuchengli wrote:
> Add comment to say the insert order here must be the same as CreateVDAFps.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:414:
GpuVideoAcceleratorUtil::ConvertMediaToGpuDecodeProfiles(profiles);
On 2015/03/19 06:19:24, wuchengli wrote:
> We shouldn't touch |supported_profiles| if this function returns false. I
think
> setting |supported_profiles| to empty vector implies no profiles is supported.
> Move this line to line 411.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:72: //
|supported_profiles|. Return true if platform supports to query decoder
On 2015/03/19 06:19:25, wuchengli wrote:
> Explain |supported_profiles| won't have duplicate when two VDA support the
same
> codec profile.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_encode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.h:62: //
|supported_profiles|.
On 2015/03/19 06:19:25, wuchengli wrote:
> Explain |supported_profiles| won't have duplicate when two VEA support the
same
> codec profile.

Done.

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

https://codereview.chromium.org/795633005/diff/320001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:211:
V4L2VideoDecodeAccelerator::GetSupportedProfiles() {
On 2015/03/19 06:19:25, wuchengli wrote:
> Move this after CanDecodeOnIOThread to be consistent with the order in the
> header file.

Done.

https://codereview.chromium.org/795633005/diff/320001/content/renderer/media/...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/795633005/diff/320001/content/renderer/media/...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:206:
*supported_profiles =
On 2015/03/19 06:19:25, wuchengli wrote:
> Prefer not to touch |support_profiles| if support_query_video_decode_profiles
is
> false.

Done.

https://codereview.chromium.org/795633005/diff/320001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/320001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:216: // Whether the platform supports to query video
decoder profiles.
On 2015/03/19 06:19:25, wuchengli wrote:
> Need more explanation about the relationship between
> |support_query_video_decode_profiles| and
> |video_decode_accelerator_supported_profiles|. Add something like "If true,
> |video_decode_accelerator_supported_profiles| is the profiles supported by the
> platform. If false, it is invalid."

Done.

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

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:580: bool support_query_profile =
On 2015/03/19 06:19:25, wuchengli wrote:
> s/profile/profiles/

Done.

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:586: IsCodedSizeSupported(coded_size,
On 2015/03/19 06:19:25, wuchengli wrote:
> Profile won't duplicate. So move IsCodedSizeSupported out of if clause. 
> 
> if (profile == supported_profile.profile) {
>   return IsCodedSizeSupported(coded_size, ...);
> }

Done.

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/795633005/diff/320001/media/filters/gpu_video...
media/filters/gpu_video_decoder.h:131: bool IsProfileSupported(const
VideoCodecProfile& profile,
On 2015/03/19 06:19:25, wuchengli wrote:
> VideoCodecProfile is just an enum. Use VideoCodecProfile directly should be
> enough.

Done.

[wuchengli, 2015-03-19 14:56:52]

wuchengli@chromium.org changed reviewers:
+ xhwang@chromium.org
- scherkus@chromium.org

[wuchengli, 2015-03-19 15:02:57]

LGTM with three minor comments. Please also rebase because GVDA has been
changed.

piman: please OWNER review for gpu/ and content/. Thanks.
xhwang: please OWNER review for media/. Thanks.

piman@ Now that VAAPI supports resolution querying, the workaround of using CPU
to decide maximum resolution is removed. PTAL.

https://codereview.chromium.org/795633005/diff/340001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/340001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:39: static void
UniqueInsertDecodeProfiles(
s/UniqueInsertDecodeProfiles/InsertUniqueDecodeProfiles/. The convention is the
verb should be at the beginning. Same for UniqueInsertEncodeProfiles().

https://codereview.chromium.org/795633005/diff/340001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:42: const
std::vector<media::VideoDecodeAccelerator::SupportedProfile>&
Input should be the first parameter. Output should be at the end.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:217: // only CrOS supports to query video decoder
supported profiles. If
Remove "Currently only CrOS supports to query video decoder supported
profiles.". When someone adds the support for another platform, they won't know
they should update the comments here. The rest of the comments should be clear
enough.

[piman, 2015-03-19 20:46:09]

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:221: bool support_query_video_decode_profiles;
Why do we do this?
If this is false, we hardcode the resolution on the client side and accept any
codec.
Could we instead hardcode the codecs&resolution on the service side, that way we
could be more exact (Mac and Windows only accept a subset of codecs, and
possibly Android as well).

[wuchengli, 2015-03-20 01:26:34]

On 2015/03/19 20:46:09, piman (Very slow to review) wrote:
> https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h
> File gpu/config/gpu_info.h (right):
> 
>
https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h#n...
> gpu/config/gpu_info.h:221: bool support_query_video_decode_profiles;
> Why do we do this?
> If this is false, we hardcode the resolution on the client side and accept any
> codec.
VDA::Initialize will still reject the unsupported profile.
> Could we instead hardcode the codecs&resolution on the service side, that way
we
> could be more exact (Mac and Windows only accept a subset of codecs, and
> possibly Android as well).
Yeah. We also want that. Can we do it in a follow-up CL? It's hard to make sure
this
works on four platforms at once. Android should be simple. Mac looks OK and
needs
trying. I'm not sure about Windows VDA because its Initialize uses some
constructor
parameters (|gl_context_|) that we don't have when GPU process starts.

[piman, 2015-03-20 03:06:27]

On Thu, Mar 19, 2015 at 6:26 PM, <wuchengli@chromium.org> wrote:

> On 2015/03/19 20:46:09, piman (Very slow to review) wrote:
>
>> https://codereview.chromium.org/795633005/diff/340001/gpu/
>> config/gpu_info.h
>> File gpu/config/gpu_info.h (right):
>>
>
>
> https://codereview.chromium.org/795633005/diff/340001/gpu/
> config/gpu_info.h#newcode221
>
>> gpu/config/gpu_info.h:221: bool support_query_video_decode_profiles;
>> Why do we do this?
>> If this is false, we hardcode the resolution on the client side and
>> accept any
>> codec.
>>
> VDA::Initialize will still reject the unsupported profile.
>
>> Could we instead hardcode the codecs&resolution on the service side, that
>> way
>>
> we
>
>> could be more exact (Mac and Windows only accept a subset of codecs, and
>> possibly Android as well).
>>
> Yeah. We also want that. Can we do it in a follow-up CL? It's hard to make
> sure
> this
> works on four platforms at once. Android should be simple. Mac looks OK and
> needs
> trying. I'm not sure about Windows VDA because its Initialize uses some
> constructor
> parameters (|gl_context_|) that we don't have when GPU process starts.
>

Not sure I follow.
In your current CL, you're basically accepting every codec with a hardcoded
min/max size. So at the very minimum, on the service side, you could just
add all of the codecs with the same hardcoded min/max sides, and get
exactly the same result.
Now DXVA explicitly accepts exactly 5 codecs, and the Mac one exactly 9
codecs, so as a trivial optimization you can add only those instead of all
of them. Android is a bit less trivial.

Then you don't need this complicated logic of making the choice sometimes
on the service and sometimes on the client.


> https://codereview.chromium.org/795633005/
>

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

[wuchengli, 2015-03-20 05:18:04]

On 2015/03/20 03:06:27, piman (Very slow to review) wrote:
> On Thu, Mar 19, 2015 at 6:26 PM, <mailto:wuchengli@chromium.org> wrote:
> 
> > On 2015/03/19 20:46:09, piman (Very slow to review) wrote:
> >
> >> https://codereview.chromium.org/795633005/diff/340001/gpu/
> >> config/gpu_info.h
> >> File gpu/config/gpu_info.h (right):
> >>
> >
> >
> > https://codereview.chromium.org/795633005/diff/340001/gpu/
> > config/gpu_info.h#newcode221
> >
> >> gpu/config/gpu_info.h:221: bool support_query_video_decode_profiles;
> >> Why do we do this?
> >> If this is false, we hardcode the resolution on the client side and
> >> accept any
> >> codec.
> >>
> > VDA::Initialize will still reject the unsupported profile.
> >
> >> Could we instead hardcode the codecs&resolution on the service side, that
> >> way
> >>
> > we
> >
> >> could be more exact (Mac and Windows only accept a subset of codecs, and
> >> possibly Android as well).
> >>
> > Yeah. We also want that. Can we do it in a follow-up CL? It's hard to make
> > sure
> > this
> > works on four platforms at once. Android should be simple. Mac looks OK and
> > needs
> > trying. I'm not sure about Windows VDA because its Initialize uses some
> > constructor
> > parameters (|gl_context_|) that we don't have when GPU process starts.
> >
> 
> Not sure I follow.
> In your current CL, you're basically accepting every codec with a hardcoded
> min/max size. So at the very minimum, on the service side, you could just
> add all of the codecs with the same hardcoded min/max sides, and get
> exactly the same result.
> Now DXVA explicitly accepts exactly 5 codecs, and the Mac one exactly 9
> codecs, so as a trivial optimization you can add only those instead of all
> of them. Android is a bit less trivial.
> 
> Then you don't need this complicated logic of making the choice sometimes
> on the service and sometimes on the client.
I see what you mean. So if GpuInfo claims to support a profile, VDA::Initialize
may still fail. I agree it's better than the current patchset. Henry will
move this to the service side.

[henryhsu, 2015-03-23 03:21:08]

PTAL.

https://codereview.chromium.org/795633005/diff/340001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/340001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:39: static void
UniqueInsertDecodeProfiles(
On 2015/03/19 15:02:57, wuchengli wrote:
> s/UniqueInsertDecodeProfiles/InsertUniqueDecodeProfiles/. The convention is
the
> verb should be at the beginning. Same for UniqueInsertEncodeProfiles().

Done.

https://codereview.chromium.org/795633005/diff/340001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:42: const
std::vector<media::VideoDecodeAccelerator::SupportedProfile>&
On 2015/03/19 15:02:57, wuchengli wrote:
> Input should be the first parameter. Output should be at the end.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...

Done.

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h
File gpu/config/gpu_info.h (right):

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:217: // only CrOS supports to query video decoder
supported profiles. If
On 2015/03/19 15:02:57, wuchengli wrote:
> Remove "Currently only CrOS supports to query video decoder supported
> profiles.". When someone adds the support for another platform, they won't
know
> they should update the comments here. The rest of the comments should be clear
> enough.

Done.

https://codereview.chromium.org/795633005/diff/340001/gpu/config/gpu_info.h#n...
gpu/config/gpu_info.h:221: bool support_query_video_decode_profiles;
On 2015/03/19 20:46:09, piman (Very slow to review) wrote:
> Why do we do this?
> If this is false, we hardcode the resolution on the client side and accept any
> codec.
> Could we instead hardcode the codecs&resolution on the service side, that way
we
> could be more exact (Mac and Windows only accept a subset of codecs, and
> possibly Android as well).

Done.

[wuchengli, 2015-03-23 08:28:08]

Please also rebase.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:561: // Supported
media profiles should be synced with Initialize function.
Actually this is different because Initialize uses
media::VideoCodecBridge::IsKnownUnaccelerated. I suggest add one more line to
make sure IsKnownUnaccelerated returns true. "be synced with" is not very clear.
If you calls IsKnownUnaccelerated here, I don't think you need to add that
comment.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:916:
DXVAVideoDecodeAccelerator::GetSupportedProfiles() {
Add a TODO to say we need to make sure the profile is really supported. Maybe
"TODO(henryhsu): refactor InitDecoder and ensure the profile is actually
supported."

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:917: // Supported
media profiles should be synced with Initialize function.
line 537 should also use |media_profiles| array to check and you don't need this
comment.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:918:
std::vector<media::VideoCodecProfile> media_profiles({
Just use an array.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:405:
GpuVideoAcceleratorUtil::InsertUniqueDecodeProfiles(vda_profiles, &profiles);
I think we should insert directly here without calling
InsertUniqueDecodeProfiles. Only line 411 and 415 need
InsertUniqueDecodeProfiles.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:424: NOTIMPLEMENTED()
<< "HW video decode acceleration get supported resolution"
This shouldn't be an error. Just remove.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:1061: // Supported media
profiles should be synced with Initialize function.
Like I said in android_video_decode_accelerator.cc, "be synced with" is not
clear. I think you can declare an array and use it in both places. This comment
won't be needed.

VideoCodecProfile[] kSupportedProfiles = {H264PROFILE_BASELINE,
H264PROFILE_MAIN, H264PROFILE_EXTENDED, ...};

[henryhsu, 2015-03-23 10:06:36]

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/android_video_decode_accelerator.cc:561: // Supported
media profiles should be synced with Initialize function.
On 2015/03/23 08:28:08, wuchengli wrote:
> Actually this is different because Initialize uses
> media::VideoCodecBridge::IsKnownUnaccelerated. I suggest add one more line to
> make sure IsKnownUnaccelerated returns true. "be synced with" is not very
clear.
> If you calls IsKnownUnaccelerated here, I don't think you need to add that
> comment.

Done.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:916:
DXVAVideoDecodeAccelerator::GetSupportedProfiles() {
On 2015/03/23 08:28:08, wuchengli wrote:
> Add a TODO to say we need to make sure the profile is really supported. Maybe
> "TODO(henryhsu): refactor InitDecoder and ensure the profile is actually
> supported."

Done.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:917: // Supported
media profiles should be synced with Initialize function.
On 2015/03/23 08:28:08, wuchengli wrote:
> line 537 should also use |media_profiles| array to check and you don't need
this
> comment.

Done.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:918:
std::vector<media::VideoCodecProfile> media_profiles({
On 2015/03/23 08:28:08, wuchengli wrote:
> Just use an array.

Done.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:405:
GpuVideoAcceleratorUtil::InsertUniqueDecodeProfiles(vda_profiles, &profiles);
On 2015/03/23 08:28:08, wuchengli wrote:
> I think we should insert directly here without calling
> InsertUniqueDecodeProfiles. Only line 411 and 415 need
> InsertUniqueDecodeProfiles.

Done.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:424: NOTIMPLEMENTED()
<< "HW video decode acceleration get supported resolution"
On 2015/03/23 08:28:08, wuchengli wrote:
> This shouldn't be an error. Just remove.

Done.

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/360001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:1061: // Supported media
profiles should be synced with Initialize function.
On 2015/03/23 08:28:08, wuchengli wrote:
> Like I said in android_video_decode_accelerator.cc, "be synced with" is not
> clear. I think you can declare an array and use it in both places. This
comment
> won't be needed.
> 
> VideoCodecProfile[] kSupportedProfiles = {H264PROFILE_BASELINE,
> H264PROFILE_MAIN, H264PROFILE_EXTENDED, ...};

Done.

[wuchengli, 2015-03-23 14:42:06]

lgtm with some small comments

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:106: const
media::VideoCodecProfile kSupportedProfiles[] = {
add static so we are not exposing it.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:416: #endif //
defined(OS_CHROMEOS)
Remove "// defined(OS_CHROMEOS)" comment

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:36: const
media::VideoCodecProfile kSupportedProfiles[] = {
static

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:326: // Only H.264 with
4:2:0 chroma sampling is supported.
Move the comment to where kSupportedProfiles is declared.

[piman, 2015-03-23 20:29:30]

LGTM + nits.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:546: for (size_t i =
0; i < arraysize(kSupportedProfiles); ++i) {
nit: you can do for (auto supported_profile : kSupportedProfiles) {
  if (profile == supported_profile) {
    profile_supported = true;
    break;
  }
}

You can even use std::find (may or may not be more readable).

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:930: for (size_t i =
0; i < arraysize(kSupportedProfiles); ++i) {
nit: same here, for (auto supported_profile : kSupportedProfiles)

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:398:
profiles.insert(profiles.end(), vda_profiles.begin(), vda_profiles.end());
nit: you can do profiles = DXVAVideoDecodeAccelerator::GetSupportedProfiles()
directly, skipping the vda_profiles intermediate and the insert.

Same in multiple places below.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:328: for (size_t i = 0;
i < arraysize(kSupportedProfiles); ++i) {
nit: same as other file, you can use range for loop

[wuchengli, 2015-03-24 07:03:36]

xhwang: please OWNER review for media/. Thanks.

[henryhsu, 2015-03-24 10:02:44]

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/dxva_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:106: const
media::VideoCodecProfile kSupportedProfiles[] = {
On 2015/03/23 14:42:05, wuchengli wrote:
> add static so we are not exposing it.

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:546: for (size_t i =
0; i < arraysize(kSupportedProfiles); ++i) {
On 2015/03/23 20:29:30, piman (Very slow to review) wrote:
> nit: you can do for (auto supported_profile : kSupportedProfiles) {
>   if (profile == supported_profile) {
>     profile_supported = true;
>     break;
>   }
> }
> 
> You can even use std::find (may or may not be more readable).

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/dxva_video_decode_accelerator.cc:930: for (size_t i =
0; i < arraysize(kSupportedProfiles); ++i) {
On 2015/03/23 20:29:30, piman (Very slow to review) wrote:
> nit: same here, for (auto supported_profile : kSupportedProfiles)

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:398:
profiles.insert(profiles.end(), vda_profiles.begin(), vda_profiles.end());
On 2015/03/23 20:29:30, piman (Very slow to review) wrote:
> nit: you can do profiles = DXVAVideoDecodeAccelerator::GetSupportedProfiles()
> directly, skipping the vda_profiles intermediate and the insert.
> 
> Same in multiple places below.

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:416: #endif //
defined(OS_CHROMEOS)
On 2015/03/23 14:42:06, wuchengli wrote:
> Remove "// defined(OS_CHROMEOS)" comment

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:36: const
media::VideoCodecProfile kSupportedProfiles[] = {
On 2015/03/23 14:42:06, wuchengli wrote:
> static

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:326: // Only H.264 with
4:2:0 chroma sampling is supported.
On 2015/03/23 14:42:06, wuchengli wrote:
> Move the comment to where kSupportedProfiles is declared.

Done.

https://codereview.chromium.org/795633005/diff/400001/content/common/gpu/medi...
content/common/gpu/media/vt_video_decode_accelerator.cc:328: for (size_t i = 0;
i < arraysize(kSupportedProfiles); ++i) {
On 2015/03/23 20:29:30, piman (Very slow to review) wrote:
> nit: same as other file, you can use range for loop

Done.

[xhwang, 2015-03-26 06:15:32]

Sorry for the delay. I just have some minor comments.

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

https://codereview.chromium.org/795633005/diff/460001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:103: if (coded_size.width() <=
max_resolution.width() &&
nit: s/if/return

https://codereview.chromium.org/795633005/diff/460001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:580: supported_profiles =
factories_->GetVideoDecodeAcceleratorSupportedProfiles();
nit: you can merge these two lines.

https://codereview.chromium.org/795633005/diff/460001/media/filters/gpu_video...
File media/filters/gpu_video_decoder.h (right):

https://codereview.chromium.org/795633005/diff/460001/media/filters/gpu_video...
media/filters/gpu_video_decoder.h:132: const gfx::Size& coded_size);
See comment below. You may not need this function if the factory can expose
something similar.

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:70:
GetVideoDecodeAcceleratorSupportedProfiles() = 0;
It seems this is only needed by IsProfileSupported() in GpuVideoDecoder. In that
case, why do we need to expose the full list of supported profiles to GVD? Can
we just have a CanDecode(profile, size) function here?

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:74:
GetVideoEncodeAcceleratorSupportedProfiles() = 0;
ditto

https://codereview.chromium.org/795633005/diff/460001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/460001/media/video/video_decod...
media/video/video_decode_accelerator.h:28: VideoCodecProfile profile;
Initialize |profile| in the constructor.

https://codereview.chromium.org/795633005/diff/460001/media/video/video_decod...
media/video/video_decode_accelerator.h:31: };
How about:

using SupportedProfiles = std::vector<SupportedProfile>;

Then you can use SupportedProfiles in many places in this CL.

[wuchengli, 2015-03-26 07:19:45]

wuchengli@chromium.org changed reviewers:
+ ananta@chromium.org

[wuchengli, 2015-03-26 07:27:24]

ananta@, sandersd@, and qinmin@. It is hard for us to test VDA on all four
platforms at once. Could you review the respective VDA and kindly test on your
platform for us? If you don't have the time to help with testing, we'll need the
instructions to build and set up the environment. Thanks.

ananta@ DxvaVDA
sandersd@ VtVDA
qinmin@ AndroidVDA

[Pawel Osciak, 2015-03-26 08:36:40]

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:38: // Insert decoder
media profiles without duplicate.
Convert |new_profiles| to their media equivalents and insert them into
|media_profiles|, ensuring no duplicates are inserted.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:45: // Insert encoder
media profiles without duplicate.
Similarly here.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:254: // same as the
order of querying supported profiles of VDAs.
Why do we have to query in the same order?

Wouldn't it be enough to have a set of supported profiles queried in any order,
and keep Create()ing until one matches? We know one will match eventually, and
we keep the preferred order by just doing Create() in preferred order?

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:72: // not have
duplicate codec profile.
s/would not have duplicate codec profile/will not contain duplicates/.

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

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2504:
profile.profile = media::H264PROFILE_MAIN;
We need more, at least:
H264PROFILE_BASELINE
H264PROFILE_MAIN
H264PROFILE_HIGH

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

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:471: profile.profile =
media::H264PROFILE_MAIN;
More H264 profiles needed here as well.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.cc:204: if
(cmd_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
Can we do this in the caller? Otherwise this flag only disables VAVDA, not all
VDAs like it should?

https://codereview.chromium.org/795633005/diff/460001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/795633005/diff/460001/content/content_common....
content/content_common.gypi:332:
'common/gpu/media/gpu_video_accelerator_util.cc',
.cc should come before .h

[henryhsu_tw, 2015-03-26 09:38:32]

henryhsu@google.com changed reviewers:
+ henryhsu@google.com

[henryhsu_tw, 2015-03-26 09:38:35]

PTAL

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_accelerator_util.h (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:38: // Insert decoder
media profiles without duplicate.
On 2015/03/26 08:36:39, Pawel Osciak wrote:
> Convert |new_profiles| to their media equivalents and insert them into
> |media_profiles|, ensuring no duplicates are inserted.

This function doesn't do convert. Only insert to another vector.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_accelerator_util.h:45: // Insert encoder
media profiles without duplicate.
On 2015/03/26 08:36:39, Pawel Osciak wrote:
> Similarly here.

Done.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:254: // same as the
order of querying supported profiles of VDAs.
On 2015/03/26 08:36:39, Pawel Osciak wrote:
> Why do we have to query in the same order?
> 
> Wouldn't it be enough to have a set of supported profiles queried in any
order,
> and keep Create()ing until one matches? We know one will match eventually, and
> we keep the preferred order by just doing Create() in preferred order?

For example, V4L2 supports H264 with 1080p and vaapi supports H264 with 4k.
If GetSupportedProfiles adds vaapi before V4L2, the profile of v4l2 will be
removed.
Then client wants to decode a H264 video with 4k. According to Create order, VDA
will choose V4L2VDA and initialize successfully. When decoding,
gpu_video_decoder will return error and change to software decoder. Therefore,
we keep the same order to ensure the returned profile can use corresponding
hardware decoder.

We can add resolution as parameter in Initialize function in the future.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.h:72: // not have
duplicate codec profile.
On 2015/03/26 08:36:39, Pawel Osciak wrote:
> s/would not have duplicate codec profile/will not contain duplicates/.

Done.

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

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:2504:
profile.profile = media::H264PROFILE_MAIN;
On 2015/03/26 08:36:40, Pawel Osciak wrote:
> We need more, at least:
> H264PROFILE_BASELINE
> H264PROFILE_MAIN
> H264PROFILE_HIGH

Done.

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

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:471: profile.profile =
media::H264PROFILE_MAIN;
On 2015/03/26 08:36:40, Pawel Osciak wrote:
> More H264 profiles needed here as well.

Done.

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/795633005/diff/460001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.cc:204: if
(cmd_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
On 2015/03/26 08:36:40, Pawel Osciak wrote:
> Can we do this in the caller? Otherwise this flag only disables VAVDA, not all
> VDAs like it should?

Done.

https://codereview.chromium.org/795633005/diff/460001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/795633005/diff/460001/content/content_common....
content/content_common.gypi:332:
'common/gpu/media/gpu_video_accelerator_util.cc',
On 2015/03/26 08:36:40, Pawel Osciak wrote:
> .cc should come before .h

Done.

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

https://codereview.chromium.org/795633005/diff/460001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:103: if (coded_size.width() <=
max_resolution.width() &&
On 2015/03/26 06:15:31, xhwang wrote:
> nit: s/if/return

Done.

https://codereview.chromium.org/795633005/diff/460001/media/filters/gpu_video...
media/filters/gpu_video_decoder.cc:580: supported_profiles =
factories_->GetVideoDecodeAcceleratorSupportedProfiles();
On 2015/03/26 06:15:32, xhwang wrote:
> nit: you can merge these two lines.

Done.

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:70:
GetVideoDecodeAcceleratorSupportedProfiles() = 0;
On 2015/03/26 06:15:32, xhwang wrote:
> It seems this is only needed by IsProfileSupported() in GpuVideoDecoder. In
that
> case, why do we need to expose the full list of supported profiles to GVD? Can
> we just have a CanDecode(profile, size) function here?

Because we need GetVideoEncodeAcceleratorSupportedProfiles in
rtc_video_encoder_factory.cc.
I'd prefer to keep original function prototype of decoder for consistency.

https://codereview.chromium.org/795633005/diff/460001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/460001/media/video/video_decod...
media/video/video_decode_accelerator.h:28: VideoCodecProfile profile;
On 2015/03/26 06:15:32, xhwang wrote:
> Initialize |profile| in the constructor.

Done.

https://codereview.chromium.org/795633005/diff/460001/media/video/video_decod...
media/video/video_decode_accelerator.h:31: };
On 2015/03/26 06:15:32, xhwang wrote:
> How about:
> 
> using SupportedProfiles = std::vector<SupportedProfile>;
> 
> Then you can use SupportedProfiles in many places in this CL.

Done.

[xhwang, 2015-03-26 16:21:49]

media/ lgtm % nits

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
File media/renderers/gpu_video_accelerator_factories.h (right):

https://codereview.chromium.org/795633005/diff/460001/media/renderers/gpu_vid...
media/renderers/gpu_video_accelerator_factories.h:70:
GetVideoDecodeAcceleratorSupportedProfiles() = 0;
On 2015/03/26 09:38:34, henryhsu_tw wrote:
> On 2015/03/26 06:15:32, xhwang wrote:
> > It seems this is only needed by IsProfileSupported() in GpuVideoDecoder. In
> that
> > case, why do we need to expose the full list of supported profiles to GVD?
Can
> > we just have a CanDecode(profile, size) function here?
> 
> Because we need GetVideoEncodeAcceleratorSupportedProfiles in
> rtc_video_encoder_factory.cc.
> I'd prefer to keep original function prototype of decoder for consistency.

Acknowledged.

https://codereview.chromium.org/795633005/diff/500001/media/video/video_decod...
File media/video/video_decode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/500001/media/video/video_decod...
media/video/video_decode_accelerator.cc:27: min_resolution(gfx::Size(0, 0)) {
nit: not needed; this is handled by the default ctor.

https://codereview.chromium.org/795633005/diff/500001/media/video/video_encod...
File media/video/video_encode_accelerator.cc (right):

https://codereview.chromium.org/795633005/diff/500001/media/video/video_encod...
media/video/video_encode_accelerator.cc:13: max_resolution(gfx::Size(0, 0)),
nit: not needed, this is handled by the default ctor.

[henryhsu_tw, 2015-03-30 08:29:58]

Tested and passed on Android.

[Justin Chuang, 2015-03-31 06:26:13]

On 2015/03/30 08:29:58, henryhsu_tw wrote:
> Tested and passed on Android.

Tested and passed on Mac. (w/ Henry's help)

[henryhsu, 2015-04-09 09:36:09]

The CQ bit was checked by henryhsu@chromium.org

[henryhsu, 2015-04-09 09:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
qinmin@chromium.org, piman@chromium.org, wuchengli@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/795633005/#ps580001
(title: "remove CONTENT_EXPORT for gpu_video_accelerator_util")

[commit-bot: I haz the power, 2015-04-09 09:36:26]

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

[commit-bot: I haz the power, 2015-04-09 10:03:06]

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

[commit-bot: I haz the power, 2015-04-09 10:03:09]

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

[wuchengli, 2015-04-10 02:49:35]

https://codereview.chromium.org/795633005/diff/580001/media/video/video_decod...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/795633005/diff/580001/media/video/video_decod...
media/video/video_decode_accelerator.h:27: struct SupportedProfile {
I think this needs MEDIA_EXPORT. Same for VEA::SupportedProfile.

[wuchengli, 2015-04-10 03:02:54]

Tested on Windows 7 and hardware decode acceleration worked.

[henryhsu, 2015-04-10 05:22:45]

The CQ bit was checked by henryhsu@chromium.org

[henryhsu, 2015-04-10 05:22:45]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
xhwang@chromium.org, piman@chromium.org, qinmin@chromium.org,
wuchengli@chromium.org
Link to the patchset: https://codereview.chromium.org/795633005/#ps620001
(title: "fix compile error")

[commit-bot: I haz the power, 2015-04-10 05:23:09]

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

[commit-bot: I haz the power, 2015-04-10 06:39:27]

Committed patchset #32 (id:620001)

[commit-bot: I haz the power, 2015-04-10 06:40:17]

Patchset 32 (id:??) landed as
https://crrev.com/d118544f749bae18a188141edba216fed9163eba
Cr-Commit-Position: refs/heads/master@{#324596}