Bug 75146

Summary: [chromium] Accelerated 2D canvas is blank on windows
Product: WebKit Reporter: Vangelis Kokkevis <vangelis>
Component: WebCore Misc.Assignee: Vangelis Kokkevis <vangelis>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, husky, jamesr, kbr, pfeldman, twiz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jamesr: review+, webkit.review.bot: commit-queue-

Description Vangelis Kokkevis 2011-12-22 15:51:44 PST
It stopped working after http://trac.webkit.org/changeset/103264 was checked in.  There are two problems:
1. Now that on Windows we use the glTexStorage2D extension to allocate managed textures, we cannot populate the m_frontTexture via glCopyTexImage2D as the texture is marked immutable.  glCopyTexSubImage2D will work fine.
2. On Windows, accelerated canvas uses a BGRA texture for backing store.  glCopyTex(Sub)Image2D doesn't support BGRA source textures (even when the BGRA extension is enabled) and therefore the call returns an INVALID_OPERATION.

The short term fix to get accelerated canvas working again on windows is to bypass the texture copy when running the compositor in the main thread (which is the default setting). The copy isn't necessary anyway. I filed another bug for fixing the threaded case:
https://bugs.webkit.org/show_bug.cgi?id=75142
Comment 1 Vangelis Kokkevis 2011-12-22 16:00:55 PST
Created attachment 120400 [details]
Patch
Comment 2 Stephen White 2011-12-22 16:15:14 PST
Comment on attachment 120400 [details]
Patch

Thanks for fixing this!  I'll leave the final review to James, but could we rename m_useShadowTexture to something else, to avoid confusion wrt actual canvas shadows?  I can't think of anything clever at the moment.  m_useFrontTexture?  m_copyBackingStore?
Comment 3 James Robinson 2011-12-22 18:25:32 PST
Comment on attachment 120400 [details]
Patch

I'd say m_useDoubleBuffering. Otherwise, R=me

What's missing in our test coverage such that this went in without breaking any layout or unit tests but broke the real product?
Comment 4 Vangelis Kokkevis 2011-12-27 00:25:30 PST
Committed r103703: <http://trac.webkit.org/changeset/103703>
Comment 5 Pavel Feldman 2011-12-27 02:23:01 PST
Rolled out as r103706 for breaking Canvas2DLayerChromiumTest.testFullLifecycle:

(see http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/18488/steps/webkit_unit_tests/logs/stdio)

[----------] 1 test from Canvas2DLayerChromiumTest
[ RUN      ] Canvas2DLayerChromiumTest.testFullLifecycle
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:143: Failure
Value of: static_cast<CCCanvasLayerImpl*>(layerImpl.get())->textureId()
  Actual: 1
Expected: frontTextureId
Which is: 2
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:122: Failure
Actual function call count doesn't match EXPECT_CALL(allocatorMock, deleteTexture(frontTextureId, size, GraphicsContext3D::RGBA))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:110: Failure
Actual function call count doesn't match EXPECT_CALL(allocatorMock, createTexture(size, GraphicsContext3D::RGBA))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:115: Failure
Actual function call count doesn't match EXPECT_CALL(implMock, copyTexImage2D(GraphicsContext3D::TEXTURE_2D, 0, GraphicsContext3D::RGBA, 0, 0, 300, 150, 0))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:112: Failure
Actual function call count doesn't match EXPECT_CALL(implMock, bindTexture(GraphicsContext3D::TEXTURE_2D, frontTextureId))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:114: Failure
Actual function call count doesn't match EXPECT_CALL(implMock, framebufferTexture2D(GraphicsContext3D::FRAMEBUFFER, GraphicsContext3D::COLOR_ATTACHMENT0, GraphicsContext3D::TEXTURE_2D, backTextureId, 0))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:113: Failure
Actual function call count doesn't match EXPECT_CALL(implMock, bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, fboId))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:116: Failure
Actual function call count doesn't match EXPECT_CALL(implMock, bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, 0))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:119: Failure
Actual function call count doesn't match EXPECT_CALL(mainMock, deleteFramebuffer(fboId))...
         Expected: to be called once
           Actual: never called - unsatisfied and active
third_party/WebKit/Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp:106: Failure
Actual function call count doesn't match EXPECT_CALL(mainMock, createFramebuffer())...
         Expected: to be called once
           Actual: never called - unsatisfied and active
[  FAILED  ] Canvas2DLayerChromiumTest.testFullLifecycle (1 ms)
[----------] 1 test from Canvas2DLayerChromiumTest (1 ms total)
Comment 6 Vangelis Kokkevis 2012-01-03 16:35:00 PST
Created attachment 121016 [details]
Patch
Comment 7 Vangelis Kokkevis 2012-01-03 16:36:25 PST
New patch that modifies the unit test to check both threaded and non-threaded canvas paths.
James, can you please take another look? 

(In reply to comment #6)
> Created an attachment (id=121016) [details]
> Patch
Comment 8 James Robinson 2012-01-03 17:41:09 PST
Comment on attachment 121016 [details]
Patch

R=me
Comment 9 WebKit Review Bot 2012-01-03 18:57:18 PST
Comment on attachment 121016 [details]
Patch

Rejecting attachment 121016 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Chromium.h
patching file Source/WebKit/chromium/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/chromium/tests/CCSchedulerTestCommon.h
patching file Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp
Hunk #4 FAILED at 68.
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/Canvas2DLayerChromiumTest.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/11083269
Comment 10 Vangelis Kokkevis 2012-01-04 09:18:17 PST
Committed r104044: <http://trac.webkit.org/changeset/104044>