Bug 65767 - toDataURL() uses stale data after putImageData()
Summary: toDataURL() uses stale data after putImageData()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Matthew Delaney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-08-05 06:05 PDT by M8R-1o5lig
Modified: 2012-02-06 15:10 PST (History)
9 users (show)

See Also:


Attachments
Test case and workaround (978 bytes, text/html)
2011-08-05 06:05 PDT, M8R-1o5lig
no flags Details
Patch (2.52 KB, patch)
2012-01-17 22:44 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (27.94 KB, patch)
2012-02-03 00:41 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (28.12 KB, patch)
2012-02-03 11:24 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (28.07 KB, patch)
2012-02-03 12:11 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (28.68 KB, patch)
2012-02-03 13:11 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (29.54 KB, patch)
2012-02-03 13:47 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (44.03 KB, patch)
2012-02-06 11:54 PST, Matthew Delaney
cmarrin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description M8R-1o5lig 2011-08-05 06:05:12 PDT
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?
Comment 1 Matthew Delaney 2011-08-24 11:26:56 PDT
Test comment. Please ignore.
Comment 2 Radar WebKit Bug Importer 2011-08-24 11:27:57 PDT
<rdar://problem/10016113>
Comment 3 Matthew Delaney 2012-01-17 20:28:06 PST
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.
Comment 4 Matthew Delaney 2012-01-17 22:44:12 PST
Created attachment 122875 [details]
Patch
Comment 5 Simon Fraser (smfr) 2012-01-18 09:55:48 PST
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?
Comment 6 Matthew Delaney 2012-01-18 11:35:00 PST
(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.
Comment 7 Matthew Delaney 2012-02-02 23:54:37 PST
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.
Comment 8 Matthew Delaney 2012-02-03 00:41:34 PST
Created attachment 125277 [details]
Patch
Comment 9 WebKit Review Bot 2012-02-03 01:01:38 PST
Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11421191
Comment 10 Early Warning System Bot 2012-02-03 01:04:04 PST
Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11416254
Comment 11 Gyuyoung Kim 2012-02-03 01:22:18 PST
Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11418250
Comment 12 Gustavo Noronha (kov) 2012-02-03 01:34:01 PST
Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11423179
Comment 13 Matthew Delaney 2012-02-03 11:24:16 PST
Created attachment 125371 [details]
Patch
Comment 14 Matthew Delaney 2012-02-03 12:11:10 PST
Created attachment 125380 [details]
Patch
Comment 15 Matthew Delaney 2012-02-03 12:11:33 PST
New patch to fix Skia build error.
Comment 16 WebKit Review Bot 2012-02-03 12:49:54 PST
Comment on attachment 125380 [details]
Patch

Attachment 125380 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11423359
Comment 17 Matthew Delaney 2012-02-03 13:11:43 PST
Created attachment 125395 [details]
Patch
Comment 18 WebKit Review Bot 2012-02-03 13:28:08 PST
Comment on attachment 125395 [details]
Patch

Attachment 125395 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11416497
Comment 19 Matthew Delaney 2012-02-03 13:47:01 PST
Created attachment 125403 [details]
Patch
Comment 20 Andy Estes 2012-02-03 17:26:53 PST
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 21 Andy Estes 2012-02-03 17:30:37 PST
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?
Comment 22 Matthew Delaney 2012-02-03 17:35:20 PST
(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...
Comment 23 Matthew Delaney 2012-02-03 17:38:27 PST
(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!
Comment 24 Matthew Delaney 2012-02-06 11:54:43 PST
Created attachment 125680 [details]
Patch
Comment 25 Matthew Delaney 2012-02-06 12:54:20 PST
Committed r106836: <http://trac.webkit.org/changeset/106836>