Add 4k HW decode override flag and support larger bitstream buffers in V4L2VDA.

Add 4k HW decode override flag and support larger bitstream buffers in V4L2VDA.

Add a flag to override 4k hardware video decode capability detection in GVD
if needed. This is for platforms that currently do not give us a proper way
to detect it.

Also bump input bistream size in V4L2VDA to accommodate potentially
larger streams in higher resolutions. We cannot estimate input buffer size
based on the stream, as we do not currently pass video config info down to
the VDAs, and the bitstream buffers are needed in order to parse the
stream to get that information.

BUG=373717
TEST=4k video playback

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

[Pawel Osciak, 2014-05-15 11:27:27]

piman@: please flags: chrome/browser/chromeos/login/chrome_restart_request.cc
content/browser/renderer_host/render_process_host_impl.cc

fischman@: please gvd/rest

Thank you.

[Ami GONE FROM CHROMIUM, 2014-05-15 16:00:41]

https://codereview.chromium.org/285343003/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/285343003/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:108:
kInputBufferMaxSize = 4 * 1024 * 1024,
CL description made me expect this to be conditional on the switch (would
require not being a compile-time constant anymore, obvs).  I don't feel strongly
about this, but I am sad at wasting 16MB shm per <video> :(

https://codereview.chromium.org/285343003/diff/1/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/285343003/diff/1/media/base/media_switches.cc...
media/base/media_switches.cc:20: const char kForce4kAcceleratedVideoDecode[] =
kAssume4kHWVideoDecodeIsSupported
is closer to what this is.
But really there's nothing in the code that is 4k-specific.

kIgnoreSizeLimitsForAcceleratedVideoDecode is closer to reality.

[piman, 2014-05-15 17:02:27]

https://codereview.chromium.org/285343003/diff/1/media/filters/gpu_video_deco...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/285343003/diff/1/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.cc:129: ((cpu.vendor_name() == "GenuineIntel")
&& cpu.model() >= 55);
Is this in the renderer?

What would happen if the renderer submits large videos anyway? Think malicious
renderer.

[Pawel Osciak, 2014-05-16 03:53:00]

https://codereview.chromium.org/285343003/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/285343003/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:108:
kInputBufferMaxSize = 4 * 1024 * 1024,
On 2014/05/15 16:00:42, Ami Fischman wrote:
> CL description made me expect this to be conditional on the switch (would
> require not being a compile-time constant anymore, obvs).  I don't feel
strongly
> about this, but I am sad at wasting 16MB shm per <video> :(

I'm uncomfortable making this remote relationship between
GVD::IsCodedSizeSupported and V4L2VDA buffer size.
I agree it would be good to save that memory though. Ideally we should be using
the same code as GVD::ICSS() in V4L2VDA
to decide which buffer size to use instead of hardcoding this. I'm not sure if
it's a good idea to cross-call GVD from V4L2VDA like this though, and moving
ICSS to VDA would have to go to the base class...

https://codereview.chromium.org/285343003/diff/1/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/285343003/diff/1/media/base/media_switches.cc...
media/base/media_switches.cc:20: const char kForce4kAcceleratedVideoDecode[] =
On 2014/05/15 16:00:42, Ami Fischman wrote:
> kAssume4kHWVideoDecodeIsSupported
> is closer to what this is.
> But really there's nothing in the code that is 4k-specific.
> 
> kIgnoreSizeLimitsForAcceleratedVideoDecode is closer to reality.

I'd prefer s/Size/Resolution/. This will probably become the longest flag name
in Chrome.

https://codereview.chromium.org/285343003/diff/1/media/filters/gpu_video_deco...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/285343003/diff/1/media/filters/gpu_video_deco...
media/filters/gpu_video_decoder.cc:129: ((cpu.vendor_name() == "GenuineIntel")
&& cpu.model() >= 55);
On 2014/05/15 17:02:27, piman wrote:
> Is this in the renderer?
> 
> What would happen if the renderer submits large videos anyway? Think malicious
> renderer.

This gates whether a HW or SW decoder is used. It's called after parsing the
container with the values from the container to decide if we can instantiate a
HW decoder to decode given stream. If not, we use a SW one.

If you mean a malicious stream that has invalid header information in the
container, then if it doesn't fit in the buffers, we check for that and fail if
we can't fit in the buffers. Such a case doesn't really differ much from a
stream that has any other invalid metadata in it.

[piman, 2014-05-16 05:15:37]

LGTM for my parts.

[Ami GONE FROM CHROMIUM, 2014-05-16 16:49:59]

https://codereview.chromium.org/285343003/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/285343003/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:108:
kInputBufferMaxSize = 4 * 1024 * 1024,
On 2014/05/16 03:53:01, Pawel Osciak wrote:
> On 2014/05/15 16:00:42, Ami Fischman wrote:
> > CL description made me expect this to be conditional on the switch (would
> > require not being a compile-time constant anymore, obvs).  I don't feel
> strongly
> > about this, but I am sad at wasting 16MB shm per <video> :(
> 
> I'm uncomfortable making this remote relationship between
> GVD::IsCodedSizeSupported and V4L2VDA buffer size.

...as opposed to the current "remote relationship" between the magic number of
2MB in V4L2VDA and GVD::ICSS enforcing that nothing that might break that shows
up here?

I don't think that what I'm proposing makes the coupling any stronger than it
already is, and it will save a bunch of memory, so I think it's worthwhile.

https://codereview.chromium.org/285343003/diff/1/media/base/media_switches.cc
File media/base/media_switches.cc (right):

https://codereview.chromium.org/285343003/diff/1/media/base/media_switches.cc...
media/base/media_switches.cc:20: const char kForce4kAcceleratedVideoDecode[] =
On 2014/05/16 03:53:01, Pawel Osciak wrote:
> On 2014/05/15 16:00:42, Ami Fischman wrote:
> > kAssume4kHWVideoDecodeIsSupported
> > is closer to what this is.
> > But really there's nothing in the code that is 4k-specific.
> > 
> > kIgnoreSizeLimitsForAcceleratedVideoDecode is closer to reality.
> 
> I'd prefer s/Size/Resolution/. This will probably become the longest flag name
> in Chrome.

\grumpycat{good}
(it's not like this is a flag that should be set by anything _other_ than
/sbin/session_manager_setup.sh or whatever that file is called, right?)

[Pawel Osciak, 2014-05-16 23:31:27]

PTAL.

Changed flag name, now passing to GPU process as well to select input buffer
size.

[Ami GONE FROM CHROMIUM, 2014-05-19 18:42:42]

LGTM

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

https://codereview.chromium.org/285343003/diff/80001/media/filters/gpu_video_...
media/filters/gpu_video_decoder.cc:126: // selection in
V4L2VideoDecodeAccelerator.
s/in/in platform-specific implementations, such as V4L2VideoDecodeAccelerator/

[Pawel Osciak, 2014-05-20 00:03:31]

Antoine: would appreciate a quick second look, now that I'm also passing it to
the GPU process.
Thanks.

[piman, 2014-05-20 00:25:36]

still lgtm

[Pawel Osciak, 2014-05-20 01:35:37]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-05-20 01:36:05]

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

[commit-bot: I haz the power, 2014-05-20 04:57:31]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 06:18:48]

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

[commit-bot: I haz the power, 2014-05-20 06:18:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Pawel Osciak, 2014-05-20 07:42:49]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-05-20 07:42:55]

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

[commit-bot: I haz the power, 2014-05-20 15:19:26]

Change committed as 271653