Bug 157750 – BitmapImage::checkForSolidColor() cleanup

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This function is repeated in two files: BitmapImageCG.cpp and BitmapImageCairo.cpp. We need to move this function to BitmapImage.cpp and we need to create a new platform dependent function NativeImage::solidColor(). This later function returns an Optional<Color> given a NativeImagePtr. It is going to be called from the merged BitmapImage::checkForSolidColor().

Also we need to simplify the state of the solidColor in BitmapImage. Currently its state is controlled by three members: m_checkedForSolidColor, m_isSolidColor and m_solidColor. We can simplify that by getting rid of the two flags and changing the type of m_solidColor to be Optional<Color>.

[Said Abou-Hallawa]

Created attachment 279038 [details]
Patch

[Said Abou-Hallawa]

Created attachment 279042 [details]
Patch

[Build Bot]

Comment on attachment 279042 [details]
Patch

Attachment 279042 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1332622

New failing tests:
fast/images/hidpi-image-position-on-device-pixels.html
imported/mozilla/svg/sizing/inline--display-inline--01.xhtml
imported/mozilla/svg/sizing/inline--position-relative--01.xhtml

[Build Bot]

Created attachment 279046 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279042 [details]
Patch

Attachment 279042 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1332619

New failing tests:
fast/images/hidpi-image-position-on-device-pixels.html
imported/mozilla/svg/sizing/inline--display-inline--01.xhtml
imported/mozilla/svg/sizing/inline--position-relative--01.xhtml

[Build Bot]

Created attachment 279048 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 279042 [details]
Patch

Attachment 279042 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1332836

New failing tests:
fast/filter-image/filter-image-animation.html
fast/images/hidpi-image-position-on-device-pixels.html
imported/mozilla/svg/sizing/inline--display-inline--01.xhtml
imported/mozilla/svg/sizing/inline--position-relative--01.xhtml

[Build Bot]

Created attachment 279053 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 279062 [details]
Patch

[Darin Adler]

Comment on attachment 279062 [details]
Patch

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

I’m OK with this, but since the goal is to clean up, I think we should make this even cleaner.

The name “solid color” is not quite right for the concept “can be drawn with the solid color optimization”. Note that there are many images which are solid colors, but for which we choose not to do the work to detect whether the optimization can be applied. So a slightly longer name might be more clear.

> Source/WebCore/platform/graphics/BitmapImage.cpp:674
> +void BitmapImage::checkForSolidColor()

This has only one call site. I am not sure it should be a separate function.

All the logic in this function is tricky and unclear. May need a comment or two explaining the policy.

> Source/WebCore/platform/graphics/BitmapImage.cpp:679
> +    if (frameCount() != 1)
> +        return;

The caller does this check, so there is no need to repeat it here.

> Source/WebCore/platform/graphics/BitmapImage.cpp:686
> +        if (!ensureFrameIsCached(0))
> +            return;

While not new, this policy seems peculiar. If we hit this case we will never later detect that there is a solid color. Maybe there is a good reason for this. Needs a comment.

> Source/WebCore/platform/graphics/BitmapImage.cpp:695
>  bool BitmapImage::mayFillWithSolidColor()

The interface to BitmapImage itself is not really good. The solidColor() function will return an unpredictable value if called without first calling mayFillWithSolidColor. So the two functions can’t really be used separately. They could be combined into a single function that returns a color. Presumably an invalid color can then indicate that there the image does not consist of a single solid color.

> Source/WebCore/platform/graphics/BitmapImage.cpp:698
> +    if (m_currentFrame || frameCount() != 1)
> +        return false;

While I acknowledge it matches the old logic, I don’t understand the rationale where m_currentFrame being non-null means we should return false. Needs a comment.

Also, this changes behavior. The old code would check for the solid color when m_currentFrame was non-null and cache the result. The new code skips the check in that case, meaning it could possibly re-do the check later. The rationale either way is not entirely clear to me.

In particular, there’s a mess with which checks are done here and which are done inside checkForSolidColor(). Clearer organization would help. Some checks mean “don’t even cache the result of a check for whether this is a solid color”, other checks mean “cache a negative result and don’t keep trying to answer the question again in the future as the image is loaded/modified further”. It’s also unclear whether a BitmapImage can hold an image that may be modified, and if it is used for that, how the solid color indicator gets invalidated as the image’s data is changed.

> Source/WebCore/platform/graphics/BitmapImage.cpp:709
>  Color BitmapImage::solidColor() const
>  {
> -    return m_solidColor;
> +    return m_solidColor ? m_solidColor.value() : Color();
>  }

It’s invalid to ever call this when m_solidColor is invalid, so this should just return m_solidColor.value() and it could assert both that it’s not a missing value and that it’s valid. Better still would be changing the interface so there are not two separate functions.

> Source/WebCore/platform/graphics/BitmapImage.h:66
> +    Optional<Color> solidColor(const NativeImagePtr&);

Confusing.

This function never returns a missing value, it always returns a Color, an invalid Color if the image is not known to be entirely a single color. Any caller that checks if this optional color is missing has written dead code. No comment that indicates this. Given the way this function is currently designed, the return value should be Color, not Optional<Color>.

Also, this function returns a valid color if it can analyze the image and determine that it’s a solid color. But it does not try very hard, and so its name is unclear. This is a quick check to see if this is a particular type of single-color image. I could imagine a more sophisticated function that can handle images other than single pixel ones. The name here should somehow remain simple but make it clear that this is just a "is eligible for the solid color optimization" function, not a thorough check to see if an image is a single solid color that correctly answers the question for other images.

> Source/WebCore/platform/graphics/BitmapImage.h:318
> +    Optional<Color> m_solidColor; // If we're a 1x1 solid color, this is the color to use to fill.

Confusing.

The value of this data member is a missing value if we haven’t analyzed to check for a solid color or not, but an invalid color if we have analyzed and decided it’s not a solid color, and a valid color if we have analyzed and decide that the solid color optimization applies. No comment to indicate these subtle distinctions.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:72
> +    RetainPtr<CGContextRef> bitmapContext = adoptCF(CGBitmapContextCreate(pixel, 1, 1, 8, sizeof(pixel), sRGBColorSpaceRef(),

Would be nice to use auto here instead of writing out the type.

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:78
> +    GraphicsContext(bitmapContext.get()).setCompositeOperation(CompositeCopy);

Peculiar to use that GraphicsContext function here instead of just calling:

    CGContextSetBlendMode(bitmapContext.get(), kCGBlendModeCopy);

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:80
> +    CGRect destinationRect = CGRectMake(0, 0, 1, 1);
> +    CGContextDrawImage(bitmapContext.get(), destinationRect, image.get());

No need for the local variable here.

[Said Abou-Hallawa]

Created attachment 279862 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 279062 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:674
>> +void BitmapImage::checkForSolidColor()
> 
> This has only one call site. I am not sure it should be a separate function.
> 
> All the logic in this function is tricky and unclear. May need a comment or two explaining the policy.

Done. checkForSolidColor(), mayFillWithSolidColor() and solidColor() are now merged in one function named singlePixelSolidColor(). Also comments were added to this new function to explain the policy of calculating the solid color optimization.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:679
>> +        return;
> 
> The caller does this check, so there is no need to repeat it here.

This check was removed but still we need to check it in the new function singlePixelSolidColor().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:686
>> +            return;
> 
> While not new, this policy seems peculiar. If we hit this case we will never later detect that there is a solid color. Maybe there is a good reason for this. Needs a comment.

Fixed. In the new function singlePixelSolidColor() we do not set m_solidColor unless we either have a valid frame and we can get the solidColor from its native image. Or we are sure the image will not have this optimization and in this case m_solidColor is set to Color().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:695
>>  bool BitmapImage::mayFillWithSolidColor()
> 
> The interface to BitmapImage itself is not really good. The solidColor() function will return an unpredictable value if called without first calling mayFillWithSolidColor. So the two functions can’t really be used separately. They could be combined into a single function that returns a color. Presumably an invalid color can then indicate that there the image does not consist of a single solid color.

Done. checkForSolidColor(), mayFillWithSolidColor() and solidColor() are now merged in one function named singlePixelSolidColor().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:698
>> +        return false;
> 
> While I acknowledge it matches the old logic, I don’t understand the rationale where m_currentFrame being non-null means we should return false. Needs a comment.
> 
> Also, this changes behavior. The old code would check for the solid color when m_currentFrame was non-null and cache the result. The new code skips the check in that case, meaning it could possibly re-do the check later. The rationale either way is not entirely clear to me.
> 
> In particular, there’s a mess with which checks are done here and which are done inside checkForSolidColor(). Clearer organization would help. Some checks mean “don’t even cache the result of a check for whether this is a solid color”, other checks mean “cache a negative result and don’t keep trying to answer the question again in the future as the image is loaded/modified further”. It’s also unclear whether a BitmapImage can hold an image that may be modified, and if it is used for that, how the solid color indicator gets invalidated as the image’s data is changed.

Checking m_currentFrame is actually not needed because it will not be non-zero unless frameCount() > 1. So checking frameCount() != 1 should be enough.

I agree the logic was separated between mayFillWithSolidColor() and checkForSolidColor() and the calculation policy was not clear and undocumented. I fixed that by combining the code in one function, cleaning up the logic and adding comments.

When the image is modified, destroyMetadataAndNotify() gets called which invalidates the solidColor optimization by setting m_solidColor to Nullopt. This will force recalculating its value from the new data once singlePixelSolidColor() is called again.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:709
>>  }
> 
> It’s invalid to ever call this when m_solidColor is invalid, so this should just return m_solidColor.value() and it could assert both that it’s not a missing value and that it’s valid. Better still would be changing the interface so there are not two separate functions.

Fixed. singlePixelSolidColor() returns a Color value based on the current state of the image data. It only sets the value of m_solidColor when it is sure whether the solidColor optimization should be applied or not.

>> Source/WebCore/platform/graphics/BitmapImage.h:66
>> +    Optional<Color> solidColor(const NativeImagePtr&);
> 
> Confusing.
> 
> This function never returns a missing value, it always returns a Color, an invalid Color if the image is not known to be entirely a single color. Any caller that checks if this optional color is missing has written dead code. No comment that indicates this. Given the way this function is currently designed, the return value should be Color, not Optional<Color>.
> 
> Also, this function returns a valid color if it can analyze the image and determine that it’s a solid color. But it does not try very hard, and so its name is unclear. This is a quick check to see if this is a particular type of single-color image. I could imagine a more sophisticated function that can handle images other than single pixel ones. The name here should somehow remain simple but make it clear that this is just a "is eligible for the solid color optimization" function, not a thorough check to see if an image is a single solid color that correctly answers the question for other images.

Fixed. NativeImage::singlePixelSolidColor() now returns Color not Optional<Color>.

Fixed. The function name was changed from solidColor to singlePixelSolidColor() since we only check the image 1x1 pixel to decide whether to apply this optimization or not.

>> Source/WebCore/platform/graphics/BitmapImage.h:318
>> +    Optional<Color> m_solidColor; // If we're a 1x1 solid color, this is the color to use to fill.
> 
> Confusing.
> 
> The value of this data member is a missing value if we haven’t analyzed to check for a solid color or not, but an invalid color if we have analyzed and decided it’s not a solid color, and a valid color if we have analyzed and decide that the solid color optimization applies. No comment to indicate these subtle distinctions.

Fixed. A comment was added to describe the meaning of the states of this class member. However singlePixelSolidColor() returns a Color value which can be valid or invalid. If the returned Color is valid, the solidColor optimization can be applied.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:72
>> +    RetainPtr<CGContextRef> bitmapContext = adoptCF(CGBitmapContextCreate(pixel, 1, 1, 8, sizeof(pixel), sRGBColorSpaceRef(),
> 
> Would be nice to use auto here instead of writing out the type.

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:78
>> +    GraphicsContext(bitmapContext.get()).setCompositeOperation(CompositeCopy);
> 
> Peculiar to use that GraphicsContext function here instead of just calling:
> 
>     CGContextSetBlendMode(bitmapContext.get(), kCGBlendModeCopy);

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:80
>> +    CGContextDrawImage(bitmapContext.get(), destinationRect, image.get());
> 
> No need for the local variable here.

Done.

[WebKit Commit Bot]

Comment on attachment 279862 [details]
Patch

Clearing flags on attachment: 279862

Committed r201424: <http://trac.webkit.org/changeset/201424>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.