BitmapImage::drawPattern() may not draw a complete frame even after all the data is received

[Said Abou-Hallawa]

This may happen if the same image is not drawn synchronously through BitmapImage::draw(). The member BitmapImage::m_currentFrameDecodingStatus was added to control when a decoded frame can be destroyed. For asynchronous image decoding, we may want to keep the incomplete decoded frame till the more complete frame finishes decoding so we have something to paint. For synchronous image drawing, we know we have to destroy the incomplete decoded frame because we are going to generate a new one.

The BitmapImage::drawPattern() asks for a decoded frame if it is not cached and then draws it by calling GraphicsContext::drawPattern(). We need to destroy the incomplete decoded frames before trying to draw it as a pattern.

[Said Abou-Hallawa]

Created attachment 332626 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/34588529>

[Simon Fraser (smfr)]

Comment on attachment 332626 [details]
Patch

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

Needs a test case.

> Source/WebCore/platform/graphics/Image.cpp:-116
> -void Image::drawPattern(GraphicsContext& ctxt, const FloatRect& destRect, const FloatRect& tileRect, const AffineTransform& patternTransform,
> -    const FloatPoint& phase, const FloatSize& spacing, CompositeOperator op, BlendMode blendMode)
> -{
> -    if (!nativeImageForCurrentFrame())
> -        return;
> -
> -    ctxt.drawPattern(*this, destRect, tileRect, patternTransform, phase, spacing, op, blendMode);
> -
> -    if (imageObserver())
> -        imageObserver()->didDraw(*this);
> -}

Changelog doesn't say why this was removed.

[Said Abou-Hallawa]

Created attachment 332780 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 332626 [details]
Patch

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

A layout test was added to the new patch.

>> Source/WebCore/platform/graphics/Image.cpp:-116
>> -}
> 
> Changelog doesn't say why this was removed.

Image::drawPattern() was only called from BitmapImage(). When I moved this code to BitmapImage::drawPattern() nothing was calling this function.

To make the patch smaller and clearer, I have reverted this change back and I kept BitmapImage::drawPattern() call Image::drawPattern(). I can do this cleanup later in separate patch.

[Said Abou-Hallawa]

Created attachment 332781 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 332781 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.h:183
> +    bool m_deferUpdateImageDataEnabledForTesting { true };

It seems wrong for this to default to true.

> Source/WebCore/platform/graphics/BitmapImage.cpp:293
>      if (!ctxt.drawLuminanceMask()) {

Does the ctxt.drawLuminanceMask() code path also work correctly?

[Said Abou-Hallawa]

Created attachment 332783 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 332781 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedImage.h:183
>> +    bool m_deferUpdateImageDataEnabledForTesting { true };
> 
> It seems wrong for this to default to true.

I flipped the meaning of the member and its initial value. The new name is m_forceUpdateImageDataEnabledForTesting.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:293
>>      if (!ctxt.drawLuminanceMask()) {
> 
> Does the ctxt.drawLuminanceMask() code path also work correctly?

In theory yes because BitmapImage::draw() will be called to draw the image synchronous into the ImageBuffer context where the incomplete decoded frame will be thrown away correctly. However I could not verify that since max-mode: luminance; is not supported in WebKit.

[Said Abou-Hallawa]

Comment on attachment 332781 [details]
Patch

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

>>> Source/WebCore/platform/graphics/BitmapImage.cpp:293
>>>      if (!ctxt.drawLuminanceMask()) {
>> 
>> Does the ctxt.drawLuminanceMask() code path also work correctly?
> 
> In theory yes because BitmapImage::draw() will be called to draw the image synchronous into the ImageBuffer context where the incomplete decoded frame will be thrown away correctly. However I could not verify that since max-mode: luminance; is not supported in WebKit.

... mask-mode: luminance; is not supported in WebKit.

[WebKit Commit Bot]

Comment on attachment 332783 [details]
Patch

Clearing flags on attachment: 332783

Committed r227936: <https://trac.webkit.org/changeset/227936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.