Created attachment 103065 [details] Test case and workaround toDataURL() appears to be using stale data if the canvas is updated using putImageData(). If the canvas is subsequently updated using ordinary drawing commands, a call to toDataURL() will work as expected. To reproduce, open the attached sample code to generate an ordered list of: 1. The real canvas. 2. The toDataURL()-image for the uninitialized canvas. 3. The toDataURL()-image for the canvas after it is filled with red pixels using putImageData(). 4. The toDataURL()-image for the canvas after a no-op drawing command. Expected output (OK in Chromium 15.0.845.0 for Mac OS X 10.7.0): 1. A red square. 2. A blank square. 3. A red square. 4. A red square. Output on Safari 5.1 (7534.48.3) and WebKit Nightly 5.1 (7534.48.3, r92445) for Mac OS X 10.7.0: 1. A red square. 2. A blank square. 3. A blank square (UNEXPECTED). 4. A red square. By removing the moveTo(), lineTo(), stroke() lines, it can be verified that the canvas is indeed a red square already after putImageData(). Workaround: As in the sample code, attempt to do some no-ops on the canvas: context.moveTo( 0, 0 ); context.lineTo( 0, 0 ); context.stroke(); (Beware of anti-aliasing ghosting!) Observation: If the size of the canvas is reduced (10x10 works) the problem goes away. Perhaps the issue is related to hardware acceleration?
Test comment. Please ignore.
<rdar://problem/10016113>
The issue here is largely one of timing and false assumptions about CG behavior. Basically, we had loosely assumed before that modifying the backing store's bits behind the back of the graphics context would magically always make its way to the screen. However there's no guarantee of that - how is CG supposed to know that we modified the backing store memory in the case where we do no drawing through its API as your testcase here showcases, is the question. I've tried out a handful of possible solutions, but none of them get it right other than adding in a NoOp draw call type hack similar to the one suggested in the attached testcase. Essentially, CG wasn't' designed to have the likes of putImageData and getImageData meddling around with its backing store's data directly, so there's no easy and nice way to hint to CG that the context's bits have changed. I'm uploading a patch now that uses a noop draw call after any putImageData call to trick the context into thinking its been drawn into.
Created attachment 122875 [details] Patch
Comment on attachment 122875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122875&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:301 > + FloatRect rect(1, 1, 0, 0); > + context->fillRect(rect); Doesn't this need to set the fill color to transparent?
(In reply to comment #5) > (From update of attachment 122875 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122875&action=review > > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:301 > > + FloatRect rect(1, 1, 0, 0); > > + context->fillRect(rect); > > Doesn't this need to set the fill color to transparent? It's a fill rect of area=width=height=0, so it shouldn't matter. I'm still trying to rack my brain to think if there's some composite operation or something else that might affect the bits still in this case, but I currently can't think of anything.
Trying a new approach: wrap the bits in a CGImage and draw them to the ImageBuffer with the CGContext in a state where it'll copy the bits down just as canvas's putImageData requires.
Created attachment 125277 [details] Patch
Comment on attachment 125277 [details] Patch Attachment 125277 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11421191
Comment on attachment 125277 [details] Patch Attachment 125277 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11416254
Comment on attachment 125277 [details] Patch Attachment 125277 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11418250
Comment on attachment 125277 [details] Patch Attachment 125277 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11423179
Created attachment 125371 [details] Patch
Created attachment 125380 [details] Patch
New patch to fix Skia build error.
Comment on attachment 125380 [details] Patch Attachment 125380 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11423359
Created attachment 125395 [details] Patch
Comment on attachment 125395 [details] Patch Attachment 125395 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11416497
Created attachment 125403 [details] Patch
Comment on attachment 125403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125403&action=review I don't know enough about the new implementation of ImageBuffer::putByteArray() to review this, but I do have a few comments. > Source/WebCore/ChangeLog:16 > + TODO: Will add a pixel test for this (since this is only a painting issue) in the next patch I think it's worth writing the test as part of this patch instead of putting it off for later. > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:312 > if (m_context->isAcceleratedContext()) { There would be less indentation if you wrote this as: if (!m_context->isAcceleratedContext()) { m_data.putData(source, sourceSize, sourceRect, destPoint, m_size, m_context->isAcceleratedContext(), multiplied == Unmultiplied); return; } // accelerated context case goes here > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:321 > + if (multiplied == Unmultiplied) > + sourceCopy->m_data.putData(source, sourceSize, sourceRect, IntPoint(-sourceRect.x(), -sourceRect.y()), sourceCopy->size(), false, true); > + else > + sourceCopy->m_data.putData(source, sourceSize, sourceRect, IntPoint(-sourceRect.x(), -sourceRect.y()), sourceCopy->size(), false, false); You could write this more concisely as: sourceCopy->m_data.putData(source, sourceSize, sourceRect, IntPoint(-sourceRect.x(), -sourceRect.y()), sourceCopy->size(), false, (multiplied == Unmultiplied));
Comment on attachment 125403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125403&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:319 > + sourceCopy->m_data.putData(source, sourceSize, sourceRect, IntPoint(-sourceRect.x(), -sourceRect.y()), sourceCopy->size(), false, true); Shouldn't the second-to-last argument here be true? In the !isAcceleratedContext() case you set this argument to the value of m_context->isAcceleratedContext in your call to m_data.putData(). Why is it false here as well?
(In reply to comment #21) > (From update of attachment 125403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125403&action=review > > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:319 > > + sourceCopy->m_data.putData(source, sourceSize, sourceRect, IntPoint(-sourceRect.x(), -sourceRect.y()), sourceCopy->size(), false, true); > > Shouldn't the second-to-last argument here be true? In the !isAcceleratedContext() case you set this argument to the value of m_context->isAcceleratedContext in your call to m_data.putData(). Why is it false here as well? Yea, I see how this one looks weird, though it's actually correct. The false is from the fact that sourceCopy is not accelerated (though, we ourselves are accelerated) since ImageBuffers pass down that knowledge to their ImageBufferData objects for this call. I should replace the false with sourceCopy()->context()->isAccelerated() instead to make it clearer. Really, this whole could use a good cleanup...
(In reply to comment #20) > (From update of attachment 125403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125403&action=review > > I don't know enough about the new implementation of ImageBuffer::putByteArray() to review this, but I do have a few comments. > > > Source/WebCore/ChangeLog:16 > > + TODO: Will add a pixel test for this (since this is only a painting issue) in the next patch > > I think it's worth writing the test as part of this patch instead of putting it off for later. Oop, yea, meant to do that along with the Skia port fixes. Cooking that up now then. > > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:312 > > if (m_context->isAcceleratedContext()) { > > There would be less indentation if you wrote this as: > > if (!m_context->isAcceleratedContext()) { > m_data.putData(source, sourceSize, sourceRect, destPoint, m_size, m_context->isAcceleratedContext(), multiplied == Unmultiplied); > return; > } Good call. > > You could write this more concisely as: > > sourceCopy->m_data.putData(source, sourceSize, sourceRect, IntPoint(-sourceRect.x(), -sourceRect.y()), sourceCopy->size(), false, (multiplied == Unmultiplied)); Good calls!
Created attachment 125680 [details] Patch
Committed r106836: <http://trac.webkit.org/changeset/106836>
Build fixes in: http://trac.webkit.org/changeset/106843 http://trac.webkit.org/changeset/106850 http://trac.webkit.org/changeset/106859
(In reply to comment #26) > Build fixes in: > http://trac.webkit.org/changeset/106843 > http://trac.webkit.org/changeset/106850 > http://trac.webkit.org/changeset/106859 And this one for windows: http://trac.webkit.org/changeset/106860