Use GPU blacklist to control accelerated video decode.

Use GPU blacklist to control accelerated video decode.

BUG=137247, 142402, 141500, 139683
TEST=about:gpu page
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152167

[Zhenyao Mo, 2012-08-16 23:06:52]

Ami: please have a look.

Antoine: I don't think ChromeOS is affected by this change, but want to keep you
informed anyway in case in the future ChromeOS wants to control this feature on
certain configs.

[piman, 2012-08-16 23:37:04]

LGTM
Note: this only affects HTML5, just making sure that's what you want. Flash may
also use accelerated video decode, but it has a separate flag (which I don't
think is on by default), and I don't think --disable-accelerated-video-decode
has any effect.

[Zhenyao Mo, 2012-08-16 23:42:34]

On 2012/08/16 23:37:04, piman wrote:
> LGTM
> Note: this only affects HTML5, just making sure that's what you want. Flash
may
> also use accelerated video decode, but it has a separate flag (which I don't
> think is on by default), and I don't think --disable-accelerated-video-decode
> has any effect.

Thanks for pointing this out.  Ami, when you talk to me about using blacklist to
control video decode, you refer to HTML5 only or HTML5 and Flash?

[Ami GONE FROM CHROMIUM, 2012-08-17 00:19:16]

I was referring to HTML5 only.

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
File chrome/browser/resources/software_rendering_list.json (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
chrome/browser/resources/software_rendering_list.json:671: "description":
"Accelerated video decode is unstable except for ChromeOS",
This isn't accurate.  The 142402 disablement was a speculative step, and appears
*not* to have been to blame, after all.  We're on track for shipping hw decode
on m22 to windows.
Assuming we ship there then this string just wants to be:
Accelerated video decode is unavailable on Mac and Linux

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
chrome/browser/resources/software_rendering_list.json:676: "type": "chromeos"
I guess add "win" here?

https://chromiumcodereview.appspot.com/10832356/diff/1/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl.cc:189:
command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
What is this doing?

https://chromiumcodereview.appspot.com/10832356/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:2400: use_accelerated_video_decode &=
!CommandLine::ForCurrentProcess()->HasSwitch(
I think this change is wrong:
- The &= is unnecessary given the default-true assignment above.
- This turns on by default the feature everywhere, which is not correct.

[Zhenyao Mo, 2012-08-17 00:25:09]

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
File chrome/browser/resources/software_rendering_list.json (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
chrome/browser/resources/software_rendering_list.json:671: "description":
"Accelerated video decode is unstable except for ChromeOS",
On 2012/08/17 00:19:17, Ami Fischman wrote:
> This isn't accurate.  The 142402 disablement was a speculative step, and
appears
> *not* to have been to blame, after all.  We're on track for shipping hw decode
> on m22 to windows.
> Assuming we ship there then this string just wants to be:
> Accelerated video decode is unavailable on Mac and Linux

Will change.

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
chrome/browser/resources/software_rendering_list.json:676: "type": "chromeos"
On 2012/08/17 00:19:17, Ami Fischman wrote:
> I guess add "win" here?

The windows is not on at the moment, are you sure about turning it on?  If yes,
I can add win here.

https://chromiumcodereview.appspot.com/10832356/diff/1/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl.cc:189:
command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
On 2012/08/17 00:19:17, Ami Fischman wrote:
> What is this doing?

This passes the switch to renderer process to disable accelerated_video_decode
if the feature is blacklisted.

https://chromiumcodereview.appspot.com/10832356/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:2400: use_accelerated_video_decode &=
!CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/08/17 00:19:17, Ami Fischman wrote:
> I think this change is wrong:
> - The &= is unnecessary given the default-true assignment above.
> - This turns on by default the feature everywhere, which is not correct.

My change says accelerated_video_decode is on unless the switch to disable it is
passed - either by directly passing the switch at command-line, or inserted to
renderer process by blacklist.

[Zhenyao Mo, 2012-08-17 00:28:16]

Revised, please have another look.

[Ami GONE FROM CHROMIUM, 2012-08-17 04:14:00]

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
File chrome/browser/resources/software_rendering_list.json (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/chrome/browser/resourc...
chrome/browser/resources/software_rendering_list.json:676: "type": "chromeos"
On 2012/08/17 00:25:09, Zhenyao Mo wrote:
> On 2012/08/17 00:19:17, Ami Fischman wrote:
> > I guess add "win" here?
> 
> The windows is not on at the moment, are you sure about turning it on?  If
yes,
> I can add win here.

I'm currently negotiating that, and it looks like I'll reenable on win tomorrow
morning, hopefully before this CL lands (to make the revert clearest).

https://chromiumcodereview.appspot.com/10832356/diff/7001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/7001/content/renderer/re...
content/renderer/render_view_impl.cc:2401:
switches::kDisableAcceleratedVideoDecode);
My point is that now that everything goes via flag (w/out #ifdef's), l.2399-2405
can be rewritten as:
WebGraphicsContext3DCommandBufferImpl* context3d = NULL;
if (!cmd_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
  context3d = RenderThreadImpl::current()->GetGpuVDAContext3D();

[Ami GONE FROM CHROMIUM, 2012-08-17 04:15:52]

https://chromiumcodereview.appspot.com/10832356/diff/1/content/browser/gpu/gp...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/1/content/browser/gpu/gp...
content/browser/gpu/gpu_data_manager_impl.cc:189:
command_line->AppendSwitch(switches::kDisableAcceleratedVideoDecode);
On 2012/08/17 00:25:09, Zhenyao Mo wrote:
> On 2012/08/17 00:19:17, Ami Fischman wrote:
> > What is this doing?
> 
> This passes the switch to renderer process to disable accelerated_video_decode
> if the feature is blacklisted.

FWIW I was confused by the fact that GpuFeatureType lists "non-features" - the
flags it specifies are the blacklisted ones.

[Zhenyao Mo, 2012-08-17 17:16:16]

Revised, please review again.

https://chromiumcodereview.appspot.com/10832356/diff/7001/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/7001/content/renderer/re...
content/renderer/render_view_impl.cc:2401:
switches::kDisableAcceleratedVideoDecode);
On 2012/08/17 04:14:00, Ami Fischman wrote:
> My point is that now that everything goes via flag (w/out #ifdef's),
l.2399-2405
> can be rewritten as:
> WebGraphicsContext3DCommandBufferImpl* context3d = NULL;
> if (!cmd_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
>   context3d = RenderThreadImpl::current()->GetGpuVDAContext3D();

Done.

[Ami GONE FROM CHROMIUM, 2012-08-17 17:51:03]

https://chromiumcodereview.appspot.com/10832356/diff/10002/chrome/browser/res...
File chrome/browser/resources/software_rendering_list.json (right):

https://chromiumcodereview.appspot.com/10832356/diff/10002/chrome/browser/res...
chrome/browser/resources/software_rendering_list.json:672: "cr_bugs": [137247,
142402],
s/142402/133828/
(the former is now resolved, the latter is the mac tracking bug)

https://chromiumcodereview.appspot.com/10832356/diff/10002/chrome/browser/res...
chrome/browser/resources/software_rendering_list.json:681: "type": "win"
FYI this is compatible w/ ToT now that I reverted the disablement in 152086.

https://chromiumcodereview.appspot.com/10832356/diff/10002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/10002/content/renderer/r...
content/renderer/render_view_impl.cc:2401:
WebGraphicsContext3DCommandBufferImpl* context3d =
You went too far :)
context3d may still be NULL after this line runs, b/c of failure to create
contexts or spawn GPU process or whatnot.
(hence my previously suggested form)

[Zhenyao Mo, 2012-08-17 20:08:17]

PTAL

https://chromiumcodereview.appspot.com/10832356/diff/10002/chrome/browser/res...
File chrome/browser/resources/software_rendering_list.json (right):

https://chromiumcodereview.appspot.com/10832356/diff/10002/chrome/browser/res...
chrome/browser/resources/software_rendering_list.json:672: "cr_bugs": [137247,
142402],
On 2012/08/17 17:51:03, Ami Fischman wrote:
> s/142402/133828/
> (the former is now resolved, the latter is the mac tracking bug)

Done.

https://chromiumcodereview.appspot.com/10832356/diff/10002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10832356/diff/10002/content/renderer/r...
content/renderer/render_view_impl.cc:2401:
WebGraphicsContext3DCommandBufferImpl* context3d =
On 2012/08/17 17:51:03, Ami Fischman wrote:
> You went too far :)
> context3d may still be NULL after this line runs, b/c of failure to create
> contexts or spawn GPU process or whatnot.
> (hence my previously suggested form)

Ah thanks for catching the bug.  Fixed.

[Ami GONE FROM CHROMIUM, 2012-08-17 22:06:18]

LGTM assuming this works the way I think it does :)
(in particular, HW decode is still enabled on win & cros)