|
|
Created:
8 years, 2 months ago by ccameron Modified:
8 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, Vangelis Kokkevis Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBlacklist video and flash acceleration on NVIDIA Linux drivers.
If you create a context when you are low memory, this context may get
into a bad state where geomerty is corrupted and the screen takes seconds
to paint. The context remains in this state until it is destroyed, even
if more memory comes available.
This disabled accelerated compositing to limit the exposure to this bug.
I plan to integrate this into M23. We shouldn't re-enable accelerated
compositing until we have a more robust workaround, or NVIDIA fixes the
context creation bug.
BUG=145600
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161150
Patch Set 1 #
Total comments: 2
Patch Set 2 : Disable just video decode and flash #Patch Set 3 : Increment revision #Patch Set 4 : Disable video acceleration as well #
Total comments: 1
Patch Set 5 : Incorporate review feedback #Patch Set 6 : Fix comma syntax #Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/11095006/diff/1/content/browser/gpu/software_... File content/browser/gpu/software_rendering_list.json (right): https://codereview.chromium.org/11095006/diff/1/content/browser/gpu/software_... content/browser/gpu/software_rendering_list.json:77: "version": "3.7", You need to update blacklist version for each change https://codereview.chromium.org/11095006/diff/1/content/browser/gpu/software_... content/browser/gpu/software_rendering_list.json:846: }, If you only mean for NVIDIA proprietary drivers, you should specify driver_vendor also, see entry 47.
Sorry, I can not approve this change. It is far too heavy a hammer. Chrome's Linux users will scream if this change is made. I think that instead we should just not turn on accelerated compositing for all web pages on Linux. Up to this point the reports of this issue have been relatively few and far between; it was only when accelerated compositing was forced on for all web pages did the issue become pervasive. Proper eviction of resources associated with background tabs would also mitigate this problem. I would approve such a change, but not this one. Of course, also, we need to apply pressure to NVIDIA via http://partners.nvidia.com/ to fix the performance pathologies in their driver. (I already filed Incident 1047408, but it hasn't received a status update since 9/12/2012.)
How about we disable accelerated compositing for just Flapper and video, and leave it on for 3D CSS, WebGL, and Canvas?
On 2012/10/08 23:09:10, ccameron1 wrote: > How about we disable accelerated compositing for just Flapper and video, and > leave it on for 3D CSS, WebGL, and Canvas? How do you plan to express that? I don't think the blacklist can do that, at least not currently. Is simply disabling force-compositing-mode on Linux not a sufficient workaround?
On 2012/10/08 23:13:00, kbr wrote: > On 2012/10/08 23:09:10, ccameron1 wrote: > > How about we disable accelerated compositing for just Flapper and video, and > > leave it on for 3D CSS, WebGL, and Canvas? > > How do you plan to express that? I don't think the blacklist can do that, at > least not currently. It would require some work. > Is simply disabling force-compositing-mode on Linux not a sufficient workaround? It would certainly help (I didn't realize it was the default), but I don't think it's enough -- flash and video trigger compositing on enough pages for me to worry about our exposure.
On 2012/10/08 23:19:09, ccameron1 wrote: > On 2012/10/08 23:13:00, kbr wrote: > > On 2012/10/08 23:09:10, ccameron1 wrote: > > > How about we disable accelerated compositing for just Flapper and video, and > > > leave it on for 3D CSS, WebGL, and Canvas? > > > > How do you plan to express that? I don't think the blacklist can do that, at > > least not currently. > > It would require some work. > It looks like with this: https://src.chromium.org/viewvc/chrome?view=rev&revision=159993 we can now blacklist video separately. We were already able to blacklist flapper even before that CL. > > Is simply disabling force-compositing-mode on Linux not a sufficient > workaround? > > It would certainly help (I didn't realize it was the default), but I don't think > it's enough -- flash and video trigger compositing on enough pages for me to > worry about our exposure. -force-compositing-mode isn't on by default on linux but we are running an experiment with it on DEV and CANARY.
On 2012/10/08 23:39:08, vangelis wrote: > On 2012/10/08 23:19:09, ccameron1 wrote: > > On 2012/10/08 23:13:00, kbr wrote: > > > On 2012/10/08 23:09:10, ccameron1 wrote: > > > > How about we disable accelerated compositing for just Flapper and video, > and > > > > leave it on for 3D CSS, WebGL, and Canvas? > > > > > > How do you plan to express that? I don't think the blacklist can do that, at > > > least not currently. > > > > It would require some work. > > > > It looks like with this: > https://src.chromium.org/viewvc/chrome?view=rev&revision=159993 > > we can now blacklist video separately. We were already able to blacklist > flapper even before that CL. > > > Is simply disabling force-compositing-mode on Linux not a sufficient > > workaround? > > > > It would certainly help (I didn't realize it was the default), but I don't > think > > it's enough -- flash and video trigger compositing on enough pages for me to > > worry about our exposure. > > -force-compositing-mode isn't on by default on linux but we are running an > experiment with it on DEV and CANARY. OK. Given this, I'd be comfortable blacklisting accelerated video on Linux to reduce the number of times we trigger the compositor.
I'll disable accelerated video decode in M23. I'd feel most comfortable with disabling compositing for both video and Flash, but it's not clear if that's even a thing (let alone a thing that we could get into M23). The two more solid workarounds are (A) using only 1 context per window and (B) putting a hard limit on the number of contexts. We may be able to get (B) in for M24, but we're getting short on time. Neither are possible for M23.
I've updated the patch blacklist accelerated video decode, flash_3d, and flash_stage3d, but I'm going to need to interrogate John about the full meaning of this before requesting that it be reviewed.
I've updated the patch to disable triggering compositing for all video and flash. This will negatively impact flash and video performance (but not functionality -- flash_stage3d is already disabled on Linux for other reasons). The consensus I'm trying to build is that we'd rather have Linux users be upset that "flash/video is slow" (which they can fix by ignoring the blacklist via flags) than "chrome is hanging my machine" (which they can only fix by disabling all compositing via flags). Use of 3D CSS or WebGL or Canvas is still enabled and will trigger compositing. The goal is for this to be removed by M24 (and replaced with a "only allow N contexts to exist" scheme). Does this sound good to all involved?
LGTM as long as this has been well tested and confirmed to only reduce performance but not functionality -- and does not reduce performance of basic composited configurations. Please wait for jbauman@ to approve this.
Also, please update the synopsis of this CL to accurately reflect what is being done, since this is archived permanently in the revision history for the file.
https://codereview.chromium.org/11095006/diff/3003/content/browser/gpu/softwa... File content/browser/gpu/software_rendering_list.json (right): https://codereview.chromium.org/11095006/diff/3003/content/browser/gpu/softwa... content/browser/gpu/software_rendering_list.json:856: "video" This should be "accelerated_video", I believe.
Fixed the label, syntax, and updated the description. I've updated the description, and I've tested this on a number of sites. Flash and video still work, but don't trigger compositing. 3D CSS and WebGL still work, and trigger compositing.
lgtm
Thanks!!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/11095006/12002
Change committed as 161150 |