Bug 140667 - [WinCairo] Accelerated compositing should be disabled when graphics card does not support it.
Summary: [WinCairo] Accelerated compositing should be disabled when graphics card does...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-20 00:40 PST by peavo
Modified: 2019-08-19 19:00 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.11 KB, patch)
2015-01-20 01:08 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2015-02-11 11:42 PST, peavo
no flags Details | Formatted Diff | Diff
Patch (5.27 KB, patch)
2015-02-11 13:58 PST, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2015-01-20 00:40:31 PST
When the graphics card is not able to do accelerated compositing, we should set the relevant preference, so WebCore knows that acceleration is not possible.
Comment 1 peavo 2015-01-20 01:08:24 PST
Created attachment 244974 [details]
Patch
Comment 2 Alex Christensen 2015-02-11 08:57:36 PST
What happens when we try to use accelerated compositing when it's not available?  Does it not draw at all, or crash, or draw slowly?
Comment 3 peavo 2015-02-11 09:53:36 PST
(In reply to comment #2)
> What happens when we try to use accelerated compositing when it's not
> available?  Does it not draw at all, or crash, or draw slowly?

It does not draw at all. When disabling accelerated compositing it will draw, but slowly.
Comment 4 Alex Christensen 2015-02-11 09:59:38 PST
Comment on attachment 244974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244974&action=review

I could see this making things a little harder to debug.  If we run into a problem in the future, we would also have to ask ourselves if it is a problem with the accelerated path or not.  Doesn't DirectX have a non-graphics-card implementation that ideally behaves the same, but slower?  Couldn't we use this through ANGLE instead of OpenGL?  If that's infeasible, then this is definitely the way to go.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:248
> +bool AcceleratedCompositingContext::acceleratedCompositingAvailable()

This and the declaration should probably also be inside #if USE(TEXTURE_MAPPER_GL)

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:263
> +        DestroyWindow(testWindow);

::DestroyWindow here and elsewhere
Comment 5 peavo 2015-02-11 11:42:46 PST
Created attachment 246402 [details]
Patch
Comment 6 peavo 2015-02-11 11:47:45 PST
(In reply to comment #4)
> Comment on attachment 244974 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244974&action=review
> 

Thanks for reviewing :)

> I could see this making things a little harder to debug.  If we run into a
> problem in the future, we would also have to ask ourselves if it is a
> problem with the accelerated path or not.  Doesn't DirectX have a
> non-graphics-card implementation that ideally behaves the same, but slower? 
> Couldn't we use this through ANGLE instead of OpenGL?  If that's infeasible,
> then this is definitely the way to go.
>

That might be possible, but I'm not sure :)
I think ANGLE examines the presence of some capabilities, and will not create a renderer if the requirements are not met.
 
> > Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:248
> > +bool AcceleratedCompositingContext::acceleratedCompositingAvailable()
> 
> This and the declaration should probably also be inside #if
> USE(TEXTURE_MAPPER_GL)
> 

I think the whole file is inside #if USE(TEXTURE_MAPPER_GL)
Comment 7 Alex Christensen 2015-02-11 12:48:23 PST
Comment on attachment 246402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=246402&action=review

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:254
> +    HWND testWindow = ::CreateWindowEx(WS_EX_NOACTIVATE, defWndProcWindowClassName(), L"AcceleratedCompositingTesterWindow", WS_POPUP | WS_VISIBLE | WS_DISABLED, -width, -height, width, height, 0, 0, 0, 0);

Is this visible?  Does it make a flash on the screen?  Does an invisible test work?

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:307
> +    COLORREF pixelColor = ::GetPixel(hdc, 0, 0);

We should probably call ::SetPixel to red, swapBuffers with colorComponent as green, and then check to see if pixelColor is green.  It is unclear what the pixel would be if the test fails.
Comment 8 peavo 2015-02-11 13:58:47 PST
Created attachment 246413 [details]
Patch
Comment 9 peavo 2015-02-11 14:01:17 PST
(In reply to comment #7)
> Comment on attachment 246402 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246402&action=review
> 

Thanks for the review :)

> > Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:254
> > +    HWND testWindow = ::CreateWindowEx(WS_EX_NOACTIVATE, defWndProcWindowClassName(), L"AcceleratedCompositingTesterWindow", WS_POPUP | WS_VISIBLE | WS_DISABLED, -width, -height, width, height, 0, 0, 0, 0);
> 
> Is this visible?  Does it make a flash on the screen?  Does an invisible
> test work?
> 

It's not visible. I'm not sure how it would be with multiple monitors, though. I tested with an invisible window, but that didn't work.
Comment 10 WebKit Commit Bot 2015-02-11 14:48:33 PST
Comment on attachment 246413 [details]
Patch

Clearing flags on attachment: 246413

Committed r179962: <http://trac.webkit.org/changeset/179962>
Comment 11 WebKit Commit Bot 2015-02-11 14:48:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Fujii Hironori 2019-08-19 19:00:39 PDT
Reverted r179962 in r248885 for Bug 200493 and Bug 200563. Reopened.