Summary: | toDataURL() uses stale data after putImageData() | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | M8R-1o5lig | ||||||||||||||||||
Component: | Canvas | Assignee: | Matthew Delaney <mdelaney7> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | cmarrin, dglazkov, gustavo, mdelaney7, mitz, rniwa, simon.fraser, webkit.review.bot, xan.lopez | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||
OS: | OS X 10.7 | ||||||||||||||||||||
Attachments: |
|
Description
M8R-1o5lig
2011-08-05 06:05:12 PDT
Test comment. Please ignore. 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 |