Image::currentFrameHasAlpha should not always return false for SVGImage

[Philip Rogers]

Image::currentFrameHasAlpha is not overridden in SVGImage and always returns false. SVG Images can have alpha too so this needs to return the correct alpha value, or at least return true.

http://trac.webkit.org/changeset/135629 was recently added which takes advantage of the alpha value and probably breaks transparent SVG images.

[Justin Novosad]

I'm not sure why the default base class implementation returns false.  I would think that true is always the conservative response here.

[Philip Rogers]

Created attachment 184009 [details]
Trivial patch

[Stephen Chenney]

Comment on attachment 184009 [details]
Trivial patch

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

> Source/WebCore/platform/graphics/Image.h:102
> +    virtual bool currentFrameHasAlpha() { return true; }

Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.

[Darin Adler]

Comment on attachment 184009 [details]
Trivial patch

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

>> Source/WebCore/platform/graphics/Image.h:102
>> +    virtual bool currentFrameHasAlpha() { return true; }
> 
> Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.

Besides that issue (and I personally would be open to pure virtual for this), I think the function name is wrong. It should be named currentFrameMayHaveAlpha or some other name like that that makes it clear that it does not give a definitive answer for all types of images.

[Justin Novosad]

(In reply to comment #3)

> Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.

I also think we need to understand the impact of this change. Many of the sites that call this method need to know whether it is safe to assume that the image is opaque in order to apply shortcuts (occlusion culling, skip blending, etc.)  In that context "true" is an appropriate default value since it is safe to have false positives.  Before submitting this, we should make sure that this change is in fact safe for all the call sites.

Also, does this fix any bugs?  If we used to be falsely assuming that SVGimages are always opaque and they're not, then there is a good chance that this patch fixes a bug.  It would be nice to have a LayoutTest that proves it.

If this was not causing a bug, do we understand why?

[Philip Rogers]

Comment on attachment 184009 [details]
Trivial patch

Thanks for the reviews! I'm clearing the r? flag while I prepare the updated patch.

[Philip Rogers]

Created attachment 185001 [details]
First pass at addressing reviewer comments

[Philip Rogers]

(In reply to comment #3)
> (From update of attachment 184009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184009&action=review
> 
> > Source/WebCore/platform/graphics/Image.h:102
> > +    virtual bool currentFrameHasAlpha() { return true; }
> 
> Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.

I agree, and fixing this exposed a small bug: GeneratedImage also needs to implement this. I've updated the latest patch to address this.

(In reply to comment #4)
> (From update of attachment 184009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184009&action=review
> 
> >> Source/WebCore/platform/graphics/Image.h:102
> >> +    virtual bool currentFrameHasAlpha() { return true; }
> > 
> > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.
> 
> Besides that issue (and I personally would be open to pure virtual for this), I think the function name is wrong. It should be named currentFrameMayHaveAlpha or some other name like that that makes it clear that it does not give a definitive answer for all types of images.

I tried a couple names such as currentFrameMayHaveAlpha and currentFrameMayhaveTransparency but I wasn't happy with how it looked from the caller's perspective. As an example, RenderImage::backgroundIsObscured() calls this function but is really looking to know whether the image is fully opaque or not. The latest patch uses "currentFrameIsKnownToBeOpaque".

(In reply to comment #5)
> (In reply to comment #3)
> 
> > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.
> 
> I also think we need to understand the impact of this change. Many of the sites that call this method need to know whether it is safe to assume that the image is opaque in order to apply shortcuts (occlusion culling, skip blending, etc.)  In that context "true" is an appropriate default value since it is safe to have false positives.  Before submitting this, we should make sure that this change is in fact safe for all the call sites.

I hope the new function name "knownToBeOpaque" clarifies how we handle the unknown case.

> 
> Also, does this fix any bugs?  If we used to be falsely assuming that SVGimages are always opaque and they're not, then there is a good chance that this patch fixes a bug.  It would be nice to have a LayoutTest that proves it.
> 
> If this was not causing a bug, do we understand why?

This was a tough one--this does not actually fix any bugs! When accessing SVG images we currently go through the SVGImageCache which returns a bitmap image for imageForRenderer. Therefore, in all of the callsites we actually weren't dealing with an SVG image but instead a rasterized bitmap image of the SVG image. In WK106159 I hope to change this. The latest patch modifies a test that will catch regressions in the future.

[WebKit Review Bot]

Comment on attachment 185001 [details]
First pass at addressing reviewer comments

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

[Stephen White]

Thanks very much for taking this on!  Justin and I were discussing how misleading the current situation is, and this is a great improvement.

(In reply to comment #8)
> I tried a couple names such as currentFrameMayHaveAlpha and currentFrameMayhaveTransparency but I wasn't happy with how it looked from the caller's perspective. As an example, RenderImage::backgroundIsObscured() calls this function but is really looking to know whether the image is fully opaque or not. The latest patch uses "currentFrameIsKnownToBeOpaque".

I hate to bikeshed, but could it be simply currentFrameIsOpaque() (and isOpaque())?  They're shorter, and I think it's clear that the conservative thing to do is return false if you're not certain.

> (In reply to comment #5)
> > (In reply to comment #3)
> > 
> > > Do the derived classes that actually do not have alpha all explicitly set it? Personally I would tend to make this pure virtual and have everyone override it, but I have no idea how that would sit with the general community.
> > 
> > I also think we need to understand the impact of this change. Many of the sites that call this method need to know whether it is safe to assume that the image is opaque in order to apply shortcuts (occlusion culling, skip blending, etc.)  In that context "true" is an appropriate default value since it is safe to have false positives.  Before submitting this, we should make sure that this change is in fact safe for all the call sites.
> 
> I hope the new function name "knownToBeOpaque" clarifies how we handle the unknown case.
> 
> > 
> > Also, does this fix any bugs?  If we used to be falsely assuming that SVGimages are always opaque and they're not, then there is a good chance that this patch fixes a bug.  It would be nice to have a LayoutTest that proves it.
> > 
> > If this was not causing a bug, do we understand why?
> 
> This was a tough one--this does not actually fix any bugs! When accessing SVG images we currently go through the SVGImageCache which returns a bitmap image for imageForRenderer. Therefore, in all of the callsites we actually weren't dealing with an SVG image but instead a rasterized bitmap image of the SVG image. In WK106159 I hope to change this. The latest patch modifies a test that will catch regressions in the future.

[Peter Beverloo (cr-android ews)]

Comment on attachment 185001 [details]
First pass at addressing reviewer comments

Attachment 185001 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16160637

[Philip Rogers]

Created attachment 185029 [details]
Update for bot happiness

[Philip Rogers]

(In reply to comment #10)
> I hate to bikeshed, but could it be simply currentFrameIsOpaque() (and isOpaque())?  They're shorter, and I think it's clear that the conservative thing to do is return false if you're not certain.

This is obvious at the moment but a year from now (when I've forgotten the details) I would be very surprised to get a non-opaque image that returns true for isOpaque. For that reason I'd like to lean towards keeping it in the function name.

[Build Bot]

Comment on attachment 185029 [details]
Update for bot happiness

Attachment 185029 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16154713

[Stephen White]

(In reply to comment #13)
> (In reply to comment #10)
> > I hate to bikeshed, but could it be simply currentFrameIsOpaque() (and isOpaque())?  They're shorter, and I think it's clear that the conservative thing to do is return false if you're not certain.
> 
> This is obvious at the moment but a year from now (when I've forgotten the details) I would be very surprised to get a non-opaque image that returns true for isOpaque. For that reason I'd like to lean towards keeping it in the function name.

Wait.. if that were true, then your current patch would have isKnownToBeOpaque() returning true for a non-opaque image in the above case.  That doesn't seem right either.

I'm guessing the surprising case is isOpaque() returning false for an opaque image.  isKnownToBeOpaque() returning false for opaque seems less surprising, so I retract my objection to the verbosity.  :)

[Philip Rogers]

Created attachment 185309 [details]
Update for bot happines (2)

One last build file update. Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in needed an update for win bot's happiness.

[Stephen White]

Comment on attachment 185309 [details]
Update for bot happines (2)

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

Looks good, but Justin wants to take a look.

> Source/WebCore/ChangeLog:13
> +        This patch renames hasAlpha and currentFrameHasAlpha to isKnownOpaque and

Nit:  code says isKnownToBeOpaque, but ChangeLog says isKnownOpaque.

[Justin Novosad]

Comment on attachment 185309 [details]
Update for bot happines (2)

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

Me likes, but I'm no reviewer.

> Source/WebCore/platform/graphics/skia/BitmapImageSingleFrameSkia.cpp:14
> +    return m_nativeImage.bitmap().isOpaque();

Seeing this makes me think we should probably follow the example of this patch in skia and rename isOpaque to knownToBeOpaque, because it has a similar conservative best guess behavior.

> Source/WebCore/svg/graphics/SVGImage.h:85
> +    // FIXME: Implement this be less conservative.

Unclear. Punctuation would help.

[Stephen White]

Comment on attachment 185309 [details]
Update for bot happines (2)

r=me.  Nits are at your discretion.

[Philip Rogers]

Created attachment 185559 [details]
Update per reviewer comments

Two small updates: Fix the Hamletian ChangeLog typo and fix the grammar mistake in the FIXMEs.

@Darin
I think this patch is ready to commit but I'd like to double-check that your concerns were addressed. Do you mind taking one more look?

[Philip Rogers]

(In reply to comment #20)
> Created an attachment (id=185559) [details]
> Update per reviewer comments
> 
> Two small updates: Fix the Hamletian ChangeLog typo and fix the grammar mistake in the FIXMEs.
> 
> @Darin
> I think this patch is ready to commit but I'd like to double-check that your concerns were addressed. Do you mind taking one more look?

It has been a couple days so I'm going to send this to the queue before it rots. I am happy to re-open, roll-out, or revisit this at any time.

[WebKit Review Bot]

Comment on attachment 185559 [details]
Update per reviewer comments

Rejecting attachment 185559 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 185559, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
/PlatformContextSkiaTest.cpp
patching file Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/box-shadow/image-box-shadow-expected.html
patching file LayoutTests/fast/box-shadow/image-box-shadow.html
patching file LayoutTests/fast/box-shadow/resources/green.svg

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

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

[Philip Rogers]

Created attachment 186098 [details]
Rebased patch for landing

Looks like the previous patch went stale after all. Attaching an updated, rebased patch that's ready for the queue.

[WebKit Review Bot]

Comment on attachment 186098 [details]
Rebased patch for landing

Clearing flags on attachment: 186098

Committed r141637: <http://trac.webkit.org/changeset/141637>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.