Bug 83072 - Pixel access canvas APIs do not work transparently with high-DPI backing store
Summary: Pixel access canvas APIs do not work transparently with high-DPI backing store
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords:
Depends on:
Blocks: 73645
  Show dependency treegraph
 
Reported: 2012-04-03 14:31 PDT by mitz
Modified: 2012-04-08 21:40 PDT (History)
6 users (show)

See Also:


Attachments
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary (42.07 KB, patch)
2012-04-03 16:59 PDT, mitz
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (386.46 KB, application/zip)
2012-04-03 17:36 PDT, WebKit Review Bot
no flags Details
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary (42.49 KB, patch)
2012-04-04 09:58 PDT, mitz
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-04-03 14:31:38 PDT
Pixel access canvas APIs do not work transparently with high-DPI backing store
Comment 1 mitz 2012-04-03 16:59:49 PDT
Created attachment 135459 [details]
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary
Comment 2 WebKit Review Bot 2012-04-03 17:04:52 PDT
Attachment 135459 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:61:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:61:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:62:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2012-04-03 17:36:38 PDT
Comment on attachment 135459 [details]
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary

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

New failing tests:
compositing/checkerboard.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/video-page-visibility.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
animations/opacity-transform-animation.html
compositing/scrollbar-painting.html
compositing/backface-visibility.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/generated-content.html
compositing/fixed-position-changed-in-composited-layer.html
compositing/self-painting-layers.html
animations/3d/change-transform-in-end-event.html
animations/missing-values-last-keyframe.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
compositing/flat-with-transformed-child.html
accessibility/aria-disabled.html
animations/missing-values-first-keyframe.html
compositing/backface-visibility-hierarchical-transform.html
compositing/culling/clear-fixed-iframe.html
compositing/compositing-visible-descendant.html
compositing/fixed-position-changed-within-composited-parent-layer.html
compositing/color-matching/image-color-matching.html
compositing/color-matching/pdf-image-match.html
Comment 4 WebKit Review Bot 2012-04-03 17:36:41 PDT
Created attachment 135465 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 mitz 2012-04-03 17:44:46 PDT
Dimitri, do you know which ImageBuffer implementation is used in the configuration that’s produced attachment 135465 [details]?
Comment 6 Adam Barth 2012-04-03 22:52:37 PDT
JamesR probably knows which ImageBuffer implementation is used in cr-linux for the compositing tests.
Comment 7 mitz 2012-04-04 09:51:25 PDT
I have an idea about what might be causing this, so I am going to submit a new patch soon.
Comment 8 mitz 2012-04-04 09:58:43 PDT
Created attachment 135614 [details]
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary
Comment 9 WebKit Review Bot 2012-04-04 10:01:20 PDT
Attachment 135614 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:61:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:61:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/cg/ImageBufferDataCG.h:62:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Tim Horton 2012-04-05 13:16:07 PDT
Comment on attachment 135614 [details]
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary

View in context: https://bugs.webkit.org/attachment.cgi?id=135614&action=review

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:60
> +ImageBuffer::ImageBuffer(const IntSize& size, float /* resolutionScale */, ColorSpace, RenderingMode, DeferralMode, bool& success)
>      : m_data(size)
>      , m_size(size)
> +    , m_logicalSize(size)

Why not store resolutionScale, even if it's not being used yet? (in all of the ImageBuffer*s)

It lives on ImageBuffer unconditionally anyway.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:457
> +        RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());

Can you use the shared deviceRGBColorSpaceRef for all of these? (from GraphicsContextCG)

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:42
> +using namespace std;
> +

Do we do this? (import the whole namespace, that is, I have no problem with you using stuff from it).

> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:191
> +                vImageAffineWarp_ARGB8888(&src, &dst, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);

Ooh!
Comment 11 mitz 2012-04-05 14:03:32 PDT
(In reply to comment #10)
> (From update of attachment 135614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135614&action=review
> 
> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:60
> > +ImageBuffer::ImageBuffer(const IntSize& size, float /* resolutionScale */, ColorSpace, RenderingMode, DeferralMode, bool& success)
> >      : m_data(size)
> >      , m_size(size)
> > +    , m_logicalSize(size)
> 
> Why not store resolutionScale, even if it's not being used yet? (in all of the ImageBuffer*s)

I wanted to make it explicit that things aren’t working yet in these ports.

> 
> It lives on ImageBuffer unconditionally anyway.
> 
> > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:457
> > +        RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
> 
> Can you use the shared deviceRGBColorSpaceRef for all of these? (from GraphicsContextCG)

Yes, I can!

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:42
> > +using namespace std;
> > +
> 
> Do we do this? (import the whole namespace, that is, I have no problem with you using stuff from it).

Yes, this is the current policy.

> 
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:191
> > +                vImageAffineWarp_ARGB8888(&src, &dst, 0, &scaleTransform, backgroundColor, kvImageEdgeExtend);
> 
> Ooh!

What what?
Comment 12 Simon Fraser (smfr) 2012-04-06 10:29:49 PDT
Comment on attachment 135614 [details]
Make getImageData, putImageData, and toDataURL operate in user space and downample/upsample as necessary

View in context: https://bugs.webkit.org/attachment.cgi?id=135614&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:118
> +    // FIXME: Should we automatically use lower resolution?

...use a lower... or ...use the lower... ?
Comment 13 mitz 2012-04-06 10:49:07 PDT
Fixed (for CG) in <http://trac.webkit.org/r113457>.
Comment 14 Beth Dakin 2012-04-06 14:50:41 PDT
(In reply to comment #13)
> Fixed (for CG) in <http://trac.webkit.org/r113457>.

It seems like this may have caused many layout test failures on SnowLeopard.
Comment 15 mitz 2012-04-06 15:22:18 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > Fixed (for CG) in <http://trac.webkit.org/r113457>.
> 
> It seems like this may have caused many layout test failures on SnowLeopard.

I am going to see if I can reproduce those on Lion by disabling acceleration.
Comment 16 mitz 2012-04-06 16:58:48 PDT
Even without using Accelerate and IOSurface-backed canvas, I cannot reproduce failures on Lion. I am going to try to reproduce on Snow Leopard.
Comment 17 mitz 2012-04-06 20:15:36 PDT
With a debug build of r113531 on Mac OS X 10.6.8, I ran all the tests in canvas and fast/canvas, and did not reproduce the failures seen on the build bot.
Comment 18 mitz 2012-04-06 20:16:25 PDT
I am going to try a release build next.
Comment 19 mitz 2012-04-06 21:34:58 PDT
Also unable to reproduce those failures with a release build.
Comment 20 mitz 2012-04-08 20:39:27 PDT
Looking again at build.webkit.org, it appears as if the Snow Leopard Release (Tests) build has been exiting early after 500 failures even before r113457, but I can no longer see the results from those runs.
Comment 21 mitz 2012-04-08 21:27:01 PDT
I can get fast/canvas/canvas-arc-360-winding.html to fail if it is run after compositing/tiled-layers-hidpi.html (the Snow Leopard bots sometimes run all the tests using a single DumpRenderTree instance, so this effect becomes visible):

run-webkit-tests --child-processes=1 compositing/tiled-layers-hidpi.html fast/canvas/canvas-arc-360-winding.html
Comment 22 mitz 2012-04-08 21:40:22 PDT
Filed bug 83453.