Bug 65767 – toDataURL() uses stale data after putImageData()

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

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?

[Matthew Delaney]

Test comment. Please ignore.

[Radar WebKit Bug Importer]

<rdar://problem/10016113>

[Matthew Delaney]

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.

[Matthew Delaney]

Created attachment 122875 [details]
Patch

[Simon Fraser (smfr)]

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?

[Matthew Delaney]

(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.

[Matthew Delaney]

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.

[Matthew Delaney]

Created attachment 125277 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11421191

[Early Warning System Bot]

Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11416254

[Gyuyoung Kim]

Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11418250

[Gustavo Noronha (kov)]

Comment on attachment 125277 [details]
Patch

Attachment 125277 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11423179

[Matthew Delaney]

Created attachment 125371 [details]
Patch

[Matthew Delaney]

Created attachment 125380 [details]
Patch

[Matthew Delaney]

New patch to fix Skia build error.

[WebKit Review Bot]

Comment on attachment 125380 [details]
Patch

Attachment 125380 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11423359

[Matthew Delaney]

Created attachment 125395 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125395 [details]
Patch

Attachment 125395 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11416497

[Matthew Delaney]

Created attachment 125403 [details]
Patch

[Andy Estes]

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));

[Andy Estes]

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?

[Matthew Delaney]

(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...

[Matthew Delaney]

(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!

[Matthew Delaney]

Created attachment 125680 [details]
Patch

[Matthew Delaney]

Committed r106836: <http://trac.webkit.org/changeset/106836>

[Ryosuke Niwa]

Build fixes in:
http://trac.webkit.org/changeset/106843
http://trac.webkit.org/changeset/106850
http://trac.webkit.org/changeset/106859

[Matthew Delaney]

(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