Bug 53468 – Handle gpu-backed images in getUnmultipliedImageData()

• Component: Canvas
• Status: RESOLVED FIXED

[Mike Reed]

In Skia platform, ImageBuffer::getUnmultipliedImageData() assumes that the image is always backed by memory pixels. However, if the image is actually backed by a gpu (e.g. FBO), then the current code will crash as bitmap.getPixels() will return NULL. The fix is to relies on SkDevice::readPixels(), which abstracts that from the caller. It will return the pixels in question correctly, regardless of a raster or gpu backend.

[Mike Reed]

Created attachment 80708 [details]
patch to fix bug

Patch addresses the case when the skia platform uses a gpu behind its Image when the Image is backing a <canvas> element. It relies on SkDevice::readPixels() to abstract how the bitmap is actually backed.

[James Robinson]

Comment on attachment 80708 [details]
patch to fix bug

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

Mostly looks good - R- for the ChangeLog format and whitespace churn.  Can you also check which tests cover this and put that in the changelog?

> WebCore/ChangeLog:5
> +        Reviewed by NOBODY (OOPS!).
> +
> +        In Skia platform, ImageBuffer::getUnmultipliedImageData() assumes that

you're missing a one-line patch description and link to the bugzilla entry here.  They should go after the Reviewed by line and before the long description, with a newline separating them from those lines.

> WebCore/ChangeLog:128
> -        Reapplying this patch again. 
> +        Reapplying this patch again.

It looks like your editor is stripping trailing newlines.  We don't do that in WebKit for lines that you aren't otherwise changing as it messes up annotation data.

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:5
> - * 
> + *

these changes are unnecessary

[Mike Reed]

Created attachment 80773 [details]
fix for 53468

updated changelog

[Mike Reed]

Created attachment 80775 [details]
fix for 53468

removed accidental WS edits from ChangeLog

[WebKit Review Bot]

Attachment 80708 [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/skia/ImageBufferSkia.cpp:220:  More than one command on the same line  [whitespace/newline] [4]
Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:304:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mike Reed]

Created attachment 80795 [details]
fix for 53468

address style breaks (multiple commands on same line, using {} with single line).

[James Robinson]

Please mark the patch review? when you want it reviewed - that will also trigger the various EWS bots to take a look at the patch.  webkit-patch upload should do that by default.

[James Robinson]

Comment on attachment 80795 [details]
fix for 53468

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

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:228
> +            unsigned char* destPixel = SkTCast<unsigned char*>(&destBitmapRow[x]);

I don't know what SkTCast<>  is or what it does - is there any reason that just = &destBitmapRow[x] doesn't work?

[Mike Reed]

(In reply to comment #8)
> (From update of attachment 80795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80795&action=review
> 
> > WebCore/platform/graphics/skia/ImageBufferSkia.cpp:228
> > +            unsigned char* destPixel = SkTCast<unsigned char*>(&destBitmapRow[x]);
> 
> I don't know what SkTCast<>  is or what it does - is there any reason that just = &destBitmapRow[x] doesn't work?

SkTCast performs a safe cast that does not break strict-aliasing rules (warnings).

[James Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 80795 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80795&action=review
> > 
> > > WebCore/platform/graphics/skia/ImageBufferSkia.cpp:228
> > > +            unsigned char* destPixel = SkTCast<unsigned char*>(&destBitmapRow[x]);
> > 
> > I don't know what SkTCast<>  is or what it does - is there any reason that just = &destBitmapRow[x] doesn't work?
> 
> SkTCast performs a safe cast that does not break strict-aliasing rules (warnings).

Sounds like something we should do consistently - currently there's a good amount of code in WebKit that casts between pointer types using either static_cast<> or reinterpret_cast<>.  I'm not aware of any port that compiles with -fstrict-aliasing (chromium compiles with -fno-strict-aliasing by default) but it's a good goal to keep in mind for the future.

Long term I think we should either introduce a better cast into WTF or decide we don't care about aliasing, but probably not use a skia-specific cast.  I don't think it's blocking for this patch, however.

[James Robinson]

Comment on attachment 80795 [details]
fix for 53468

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

R=me, but the indentation seems off in most of the file.  Please resolve that before landing.

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:229
> +            SkPMColor color = destBitmapRow[x];

indentation is off here (should be 4-space indent, not 5)

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:234
> +                unsigned a = SkGetPackedA32(color);
> +                destPixel[0] = a ? SkGetPackedR32(color) * 255 / a : 0;
> +                destPixel[1] = a ? SkGetPackedG32(color) * 255 / a : 0;
> +                destPixel[2] = a ? SkGetPackedB32(color) * 255 / a : 0;

same here - 4 space indent, not 5

would uint32_t be a better type for 'a' to be explicit about the size?

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:310
> +        SkPMColor* destRow = bitmap.getAddr32(0, y);

another 5-space indent

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:316
> +            if (multiplied == Unmultiplied)
> +                destRow[x] = SkPreMultiplyARGB(srcPixel[3], srcPixel[0], srcPixel[1], srcPixel[2]);
> +            else
> +                destRow[x] = SkPackARGB32(srcPixel[3], srcPixel[0], srcPixel[1], srcPixel[2]);

also too far over

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:329
> +    putImageData<Unmultiplied>(source, sourceSize, sourceRect, destPoint, context()->platformContext()->canvas()->getDevice(), m_size);

5 spaces

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:335
> +    context()->platformContext()->prepareForSoftwareDraw();
> +    putImageData<Premultiplied>(source, sourceSize, sourceRect, destPoint, context()->platformContext()->canvas()->getDevice(), m_size);

5 spaces

[Brian Salomon]

Created attachment 80915 [details]
Updated fix

James, I took your suggestion and made destBitmapRow a uint32_t. It's now cast to a SkPMColor to be read and a uchar* to be written. I count 4 spaces for the indents, though (?). I also used reinterpret_cast rather than SkTCast.

[James Robinson]

Comment on attachment 80915 [details]
Updated fix

Looks good!

[WebKit Commit Bot]

Comment on attachment 80915 [details]
Updated fix

Clearing flags on attachment: 80915

Committed r77567: <http://trac.webkit.org/changeset/77567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

I *think* this may have broken the linux software path for a bunch of canvas tests and svg tests. See http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/12525 .

[Dirk Pranke]

Reverted r77567 for reason:

broke chromium linux svg, canvas tests, possibly win also?

Committed r77587: <http://trac.webkit.org/changeset/77587>

[Mike Reed]

Created attachment 81791 [details]
Use device->readPixels() when the device is backed by the GPU

fixes earlier layouttest breaks

[Kenneth Russell]

Comment on attachment 81791 [details]
Use device->readPixels() when the device is backed by the GPU

r=me. Submitting the revised version.

[WebKit Commit Bot]

Comment on attachment 81791 [details]
Use device->readPixels() when the device is backed by the GPU

Rejecting attachment 81791 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 2

Last 500 characters of output:
6cf70c8554d3635e55485aff427e6559479c65af
r78103 = 9723ec972829cb7aa274ccf1f9f170b5071670bf
r78104 = f8fcad88ae3d1b3d197f62ace6ab686292b17b17
r78105 = aaf0c60848d93d3b6939c0c1492c6920afe945f9
r78106 = d65164321164bbaa58934671141f174be385e605
r78107 = f7e32a0a952879dcc729428289f75ab815600c01
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://queues.webkit.org/results/7886005

[Kenneth Russell]

Mike, Brian, would that patch apply cleanly against top of tree? If not, is it easy for you to regenerate it?

I can land it by hand but it would be better to send it through the commit queue.

[Mike Reed]

Created attachment 81853 [details]
rebase (ChangeLog) to fix patch failure on previous version

[Mike Reed]

Created attachment 81864 [details]
fix for 53468 -- rebased against rev. 78126

[Kenneth Russell]

Comment on attachment 81864 [details]
fix for 53468 -- rebased against rev. 78126

There are a couple of spurious whitespace changes but it's fine. Let's try again.

[WebKit Commit Bot]

Comment on attachment 81864 [details]
fix for 53468 -- rebased against rev. 78126

Clearing flags on attachment: 81864

Committed r78148: <http://trac.webkit.org/changeset/78148>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.