Bug 169771 – REGRESSION(213764): Large images should not be decoded asynchronously when they are drawn on a canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

It seems once the images finish decoding the repaint is dropped to floor. Hovering or resizing forces the needed repaint.

[Said Abou-Hallawa]

<rdar://problem/31092794>

[Said Abou-Hallawa]

Created attachment 304914 [details]
Patch

[Said Abou-Hallawa]

Created attachment 304918 [details]
Patch

[Build Bot]

Comment on attachment 304918 [details]
Patch

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

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/async-image-canvas-draw-image.html

[Build Bot]

Created attachment 304921 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304918 [details]
Patch

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

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/async-image-canvas-draw-image.html

[Build Bot]

Created attachment 304922 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Build Bot]

Comment on attachment 304918 [details]
Patch

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

New failing tests:
fast/images/slower-decoding-than-animation-image.html
fast/images/async-image-canvas-draw-image.html

[Build Bot]

Created attachment 304923 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

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

[Said Abou-Hallawa]

Created attachment 304942 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 304942 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:166
> +    std::optional<IntSize> sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size();

Does sizeForDrawing need to be an optional<> here?

> Source/WebCore/platform/graphics/BitmapImage.cpp:187
> +        StartAnimationResult result = internalStartAnimation();

Don't name the variable "result". We use that for the function return value.

> Source/WebCore/platform/graphics/BitmapImage.h:89
> +    bool frameHasNativeSizeNativeImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel) { return m_source.frameHasNativeSizeNativeImageAtIndex(index, subsamplingLevel); }

NativeSizeNativeImage is a bit of a mouthful, and the "native" doesn't have the same meaning in each.

> Source/WebCore/platform/graphics/DecodingMode.h:130
> +    // Four states of the imade decoding:
> +    // - Synchronous: DecodingMode::Synchronous
> +    // - Asynchronous + any size: DecodingMode::Asynchronous
> +    // - Asynchronous + nativeSize: an empty std::optional<IntSize>>
> +    // - Asynchronous + sizeForDrawing: a none empty std::optional<IntSize>>
> +    using ModeESize = Variant<Mode, std::optional<IntSize>>;
> +    ModeESize m_modeSize;

DecodingMode seems like a rather heavyweight solution to this problem. Can we just use a set of bits instead?

[Simon Fraser (smfr)]

Also, is this actually about images painted into <canvas>? If so, please retitle.

[Said Abou-Hallawa]

Created attachment 304979 [details]
Patch

[Said Abou-Hallawa]

Created attachment 305161 [details]
Patch

[Said Abou-Hallawa]

Created attachment 305206 [details]
Patch

[Simon Fraser (smfr)]

Please fix the title to explain that this is about <canvas>

[Said Abou-Hallawa]

Created attachment 305214 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 304942 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:166
>> +    std::optional<IntSize> sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size();
> 
> Does sizeForDrawing need to be an optional<> here?

No it does not need to. But If I make it of type IntSize, then I have to change the calls below

if (!frameHasAsyncNativeImageAtIndex(m_currentFrame, m_currentSubsamplingLevel, sizeForDrawing)) {
    if (!frameIsBeingDecodedAtIndex(m_currentFrame, sizeForDrawing)) {

to be

if (!frameHasAsyncNativeImageAtIndex(m_currentFrame, m_currentSubsamplingLevel, DecodingMode(sizeForDrawing))) {
    if (!frameIsBeingDecodedAtIndex(m_currentFrame, DecodingMode(sizeForDrawing))) {

If this okay I can remove the optional<>.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:187
>> +        StartAnimationResult result = internalStartAnimation();
> 
> Don't name the variable "result". We use that for the function return value.

Done. I used status instead.

>> Source/WebCore/platform/graphics/BitmapImage.h:89
>> +    bool frameHasNativeSizeNativeImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel) { return m_source.frameHasNativeSizeNativeImageAtIndex(index, subsamplingLevel); }
> 
> NativeSizeNativeImage is a bit of a mouthful, and the "native" doesn't have the same meaning in each.

I used Intrinsic instead of NativeSize. This is the best I can come up with.

>> Source/WebCore/platform/graphics/DecodingMode.h:130
>> +    ModeESize m_modeSize;
> 
> DecodingMode seems like a rather heavyweight solution to this problem. Can we just use a set of bits instead?

I modified this class a little to make it more clear. And I wanted to find an expression to say async_any_size. I also wanted to avoid meaningless data structure cases like sync+sizeForDrawing or async_any_size+sizeForDrawing.

[Simon Fraser (smfr)]

Comment on attachment 305214 [details]
Patch

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

> Source/WebCore/platform/graphics/DecodingMode.h:33
> +class DecodingMode {

Let's call this DecodingOptions

> Source/WebCore/platform/graphics/DecodingMode.h:35
> +    enum Mode { Synchronous, Asynchronous };

The bare "Mode" make this hard to understand. Maybe clearer as:

AsyncOrNot { Synchronous, Asynchronous }

> Source/WebCore/platform/graphics/DecodingMode.h:47
> +    bool isSyncMode() const

maybe bool decodesSynchronously() or shouldDecodeSynchronously() or just isSynchronous()?

> Source/WebCore/platform/graphics/DecodingMode.h:52
> +    bool isAsyncMode() const

decodesAsynchronously or isAsynchronous()

> Source/WebCore/platform/graphics/DecodingMode.h:57
> +    bool operator>=(const DecodingMode& decodingMode) const

This operator doesn't make sense on this class. How is DecodingMode some continuous thing that allows for ordered comparisons?

Are you putting here logic that better belongs at the call site, regarding decisions about whether we need to decode again?

> Source/WebCore/platform/graphics/DecodingMode.h:77
> +    bool hasSize() const

Does any external client need to do this query?

> Source/WebCore/platform/graphics/DecodingMode.h:92
> +    Mode mode() const

Seems unnecessary.

> Source/WebCore/platform/graphics/DecodingMode.h:120
> +    using ModeSize = Variant<Mode, std::optional<IntSize>>;
> +    ModeSize m_modeSize;

I'm OK with this Variant complexity, but I think it needs to be better hidden inside the class.

> Source/WebCore/platform/graphics/ImageFrame.h:105
> +    bool isBeingDecoded(const DecodingMode& decodingMode) const { return m_nextDecodingMode >= decodingMode; }

Seems weird to pass a DecodingMode into isBeingDecoded(). It's more like isCurrentDecodingCompatibleWithOptions(const DecodingOptions&), right?

What does this do if there's no decode in flight?

> Source/WebCore/platform/graphics/ImageFrame.h:155
> +    DecodingMode m_nextDecodingMode;

What does "next" mean here? Is it the decode in progress? These names should make it clear when "next" becomes "last" (or current?).

[Said Abou-Hallawa]

Created attachment 305367 [details]
Patch

[Build Bot]

Comment on attachment 305367 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 305373 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Said Abou-Hallawa]

Created attachment 305438 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 305214 [details]
Patch

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

>> Source/WebCore/platform/graphics/DecodingMode.h:33
>> +class DecodingMode {
> 
> Let's call this DecodingOptions

Done. Class was renamed.

>> Source/WebCore/platform/graphics/DecodingMode.h:35
>> +    enum Mode { Synchronous, Asynchronous };
> 
> The bare "Mode" make this hard to understand. Maybe clearer as:
> 
> AsyncOrNot { Synchronous, Asynchronous }

I renamed the enum to be DecodingMode.

>> Source/WebCore/platform/graphics/DecodingMode.h:47
>> +    bool isSyncMode() const
> 
> maybe bool decodesSynchronously() or shouldDecodeSynchronously() or just isSynchronous()?

Function was renamed to isSynchronous().

>> Source/WebCore/platform/graphics/DecodingMode.h:52
>> +    bool isAsyncMode() const
> 
> decodesAsynchronously or isAsynchronous()

Function was renamed to isAsynchronous()?

>> Source/WebCore/platform/graphics/DecodingMode.h:57
>> +    bool operator>=(const DecodingMode& decodingMode) const
> 
> This operator doesn't make sense on this class. How is DecodingMode some continuous thing that allows for ordered comparisons?
> 
> Are you putting here logic that better belongs at the call site, regarding decisions about whether we need to decode again?

This function answers the question "is this DecodingOptions compatible with another DecodingOptions for asynchronous decoding mode?". I renamed it to isAsynchronousCompatibleWith().

>> Source/WebCore/platform/graphics/DecodingMode.h:77
>> +    bool hasSize() const
> 
> Does any external client need to do this query?

I made it private.

>> Source/WebCore/platform/graphics/DecodingMode.h:92
>> +    Mode mode() const
> 
> Seems unnecessary.

Function was removed.

>> Source/WebCore/platform/graphics/ImageFrame.h:105
>> +    bool isBeingDecoded(const DecodingMode& decodingMode) const { return m_nextDecodingMode >= decodingMode; }
> 
> Seems weird to pass a DecodingMode into isBeingDecoded(). It's more like isCurrentDecodingCompatibleWithOptions(const DecodingOptions&), right?
> 
> What does this do if there's no decode in flight?

This function was removed from this class. Instead ImageFrameCache now keeps a queue of the frame requests which are waiting to be committed. Answering the question "is a frame being decoded for a certain sizeForDrawing or larger?" now relies on this queue. If there is no queued request for a frame or there is but for a smaller sizeForDrawing, a new request will be queued.

All the flashing or not drawing the image elements in the current build are happening because of answering the questions "isBeingDecoded()" and "hasDecodedNativeImage()" wrong.

>> Source/WebCore/platform/graphics/ImageFrame.h:155
>> +    DecodingMode m_nextDecodingMode;
> 
> What does "next" mean here? Is it the decode in progress? These names should make it clear when "next" becomes "last" (or current?).

m_nextDecodingMode was removed and m_lastDecodingMode was renamed to m_decodingOptions.

[Said Abou-Hallawa]

*** Bug 170107 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Comment on attachment 305438 [details]
Patch

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

Looks OK but let's do another rev with the new names.

> Source/WebCore/platform/graphics/BitmapImage.cpp:168
> +    std::optional<IntSize> sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size();

This does not need to be an optional<> here.

> Source/WebCore/platform/graphics/BitmapImage.h:85
> +    bool frameIsBeingDecodedCompatibleWithOptionsAtIndex(size_t index, const DecodingOptions& decodingOptions) const { return m_source.frameIsBeingDecodedCompatibleWithOptionsAtIndex(index, decodingOptions); }

frameIsBeingDecodedCompatibleWithOptionsAtIndex -> frameBeingDecodedIsCompatibleWithOptions (I don' think the "at index" is necessary).

What does this return if no frame is being decoded?

> Source/WebCore/platform/graphics/BitmapImage.h:87
>      bool frameIsCompleteAtIndex(size_t index) const { return m_source.frameIsCompleteAtIndex(index); }
>      bool frameHasAlphaAtIndex(size_t index) const { return m_source.frameHasAlphaAtIndex(index); }

I think we could remove the "at index" from these too, later. "Frame" is implicitly something that has an index.

> Source/WebCore/platform/graphics/BitmapImage.h:89
> +    bool frameHasIntrinsicNativeImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel) { return m_source.frameHasIntrinsicNativeImageAtIndex(index, subsamplingLevel); }

hasDecodedFrameWithFullSizeNativeImage()

> Source/WebCore/platform/graphics/BitmapImage.h:90
> +    bool frameHasAsyncNativeImageAtIndex(size_t index, const std::optional<SubsamplingLevel>& subsamplingLevel, const DecodingOptions& decodingOptions) { return m_source.frameHasAsyncNativeImageAtIndex(index, subsamplingLevel, decodingOptions); }

hasDecodedFrameCompatibleWithOptions()

> Source/WebCore/platform/graphics/GradientImage.h:47
> +    void draw(GraphicsContext&, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator, BlendMode, const DecodingOptions&, ImageOrientationDescription) final;

Maybe just pass DecodingOptions::DecodingMode or DecodingMode

> Source/WebCore/platform/graphics/Image.h:188
> +    virtual void draw(GraphicsContext&, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator, BlendMode, const DecodingOptions&, ImageOrientationDescription) = 0;

Maybe just pass DecodingOptions::DecodingMode or DecodingMode

> Source/WebCore/platform/graphics/ImageFrame.h:130
> +    bool hasIntrinsicNativeImage(const std::optional<SubsamplingLevel>& = { }) const;

hasFullSizeNativeImage()

> Source/WebCore/platform/graphics/ImageFrame.h:131
> +    bool hasAsyncNativeImage(const std::optional<SubsamplingLevel>&, const DecodingOptions&) const;

hasImageCompatibleWithOptions()

> Source/WebCore/platform/graphics/ImageFrame.h:132
> +    bool hasCompleteAsyncNativeImage(const std::optional<SubsamplingLevel>&, const DecodingOptions&) const;

How is this different from the one above? Should this be hasImageCompatibleWithOptions() and the previous one be hasOrIsDecodingImageCompatibleWithOptions()?

> Source/WebCore/platform/graphics/ImageFrameCache.h:96
> +    bool frameHasIntrinsicNativeImageAtIndex(size_t, const std::optional<SubsamplingLevel>&);

frameHasFullSizeNativeImageAtIndex

> Source/WebCore/platform/graphics/ImageSource.h:96
> -    bool frameIsBeingDecodedAtIndex(size_t index, const std::optional<IntSize>& sizeForDrawing) { return m_frameCache->frameIsBeingDecodedAtIndex(index, sizeForDrawing); }
> +    bool frameIsBeingDecodedCompatibleWithOptionsAtIndex(size_t index, const DecodingOptions& decodingOptions) { return m_frameCache->frameIsBeingDecodedCompatibleWithOptionsAtIndex(index, decodingOptions); }
>      bool frameIsCompleteAtIndex(size_t index) { return m_frameCache->frameIsCompleteAtIndex(index); }
>      bool frameHasAlphaAtIndex(size_t index) { return m_frameCache->frameHasAlphaAtIndex(index); }
>      bool frameHasImageAtIndex(size_t index) { return m_frameCache->frameHasImageAtIndex(index); }
> -    bool frameHasValidNativeImageAtIndex(size_t index, const std::optional<SubsamplingLevel>& subsamplingLevel, const std::optional<IntSize>& sizeForDrawing) { return m_frameCache->frameHasValidNativeImageAtIndex(index, subsamplingLevel, sizeForDrawing); }
> -    bool frameHasDecodedNativeImage(size_t index) { return m_frameCache->frameHasDecodedNativeImage(index); }
> +    bool frameHasIntrinsicNativeImageAtIndex(size_t index, const std::optional<SubsamplingLevel>& subsamplingLevel) { return m_frameCache->frameHasIntrinsicNativeImageAtIndex(index, subsamplingLevel); }
> +    bool frameHasAsyncNativeImageAtIndex(size_t index, const std::optional<SubsamplingLevel>& subsamplingLevel, const DecodingOptions& decodingOptions) { return m_frameCache->frameHasAsyncNativeImageAtIndex(index, subsamplingLevel, decodingOptions); }
>      SubsamplingLevel frameSubsamplingLevelAtIndex(size_t index) { return m_frameCache->frameSubsamplingLevelAtIndex(index); }

Match the BitmapImage names where possible.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:375
> +    if (!decodingOptions.isSynchronous()) {

is that the same as decodingOptions.isAsynchronous()?

[Said Abou-Hallawa]

Comment on attachment 305438 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:168
>> +    std::optional<IntSize> sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size();
> 
> This does not need to be an optional<> here.

Okay.

>> Source/WebCore/platform/graphics/BitmapImage.h:85
>> +    bool frameIsBeingDecodedCompatibleWithOptionsAtIndex(size_t index, const DecodingOptions& decodingOptions) const { return m_source.frameIsBeingDecodedCompatibleWithOptionsAtIndex(index, decodingOptions); }
> 
> frameIsBeingDecodedCompatibleWithOptionsAtIndex -> frameBeingDecodedIsCompatibleWithOptions (I don' think the "at index" is necessary).
> 
> What does this return if no frame is being decoded?

Function was renamed to frameBeingDecodedIsCompatibleWithOptionsAtIndex(). I will remove "atIndex" from BitmapImage, ImageSource and ImageFrameCache functions in a separate patch. But let's keep everything consistent for now.

It will return false. This is the function that does the real work. If no frame is being decoded, m_frameCommitQueue will be empty which means it will return false.

bool ImageFrameCache::frameBeingDecodedIsCompatibleWithOptionsAtIndex(size_t index, const DecodingOptions& decodingOptions)
{
    auto it = std::find_if(m_frameCommitQueue.begin(), m_frameCommitQueue.end(), [index, &decodingOptions](const ImageFrameRequest& frameRequest) {
        return frameRequest.index == index && frameRequest.decodingOptions.isAsynchronousCompatibleWith(decodingOptions);
    });
    return it != m_frameCommitQueue.end();
}

>> Source/WebCore/platform/graphics/BitmapImage.h:87
>>      bool frameHasAlphaAtIndex(size_t index) const { return m_source.frameHasAlphaAtIndex(index); }
> 
> I think we could remove the "at index" from these too, later. "Frame" is implicitly something that has an index.

Will do.

>> Source/WebCore/platform/graphics/BitmapImage.h:89
>> +    bool frameHasIntrinsicNativeImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel) { return m_source.frameHasIntrinsicNativeImageAtIndex(index, subsamplingLevel); }
> 
> hasDecodedFrameWithFullSizeNativeImage()

Function was renamed to frameHasFullSizeNativeImageAtIndex(). I omitted the "decoded" part because what we care about here is having a full size image even if it is not fully decoded. This is used by BitmapImage::frameImageAtIndexCacheIfNeeded() which is called by itmapImage::nativeImage(), BitmapImage::nativeImageForCurrentFrame(), BitmapImage::nativeImageOfSize() and BitmapImage::framesNativeImages(). These functions need the image to be a full size image. If it was created with full size, the cached image will be used. Otherwise it is going to be cached synchronously.

>> Source/WebCore/platform/graphics/BitmapImage.h:90
>> +    bool frameHasAsyncNativeImageAtIndex(size_t index, const std::optional<SubsamplingLevel>& subsamplingLevel, const DecodingOptions& decodingOptions) { return m_source.frameHasAsyncNativeImageAtIndex(index, subsamplingLevel, decodingOptions); }
> 
> hasDecodedFrameCompatibleWithOptions()

Done. Function was renamed.

>> Source/WebCore/platform/graphics/GradientImage.h:47
>> +    void draw(GraphicsContext&, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator, BlendMode, const DecodingOptions&, ImageOrientationDescription) final;
> 
> Maybe just pass DecodingOptions::DecodingMode or DecodingMode

Done. An enum class DecodingMode is defined in DecodingOptions.h but it is not a member of the DecodingOptions class.

>> Source/WebCore/platform/graphics/Image.h:188
>> +    virtual void draw(GraphicsContext&, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator, BlendMode, const DecodingOptions&, ImageOrientationDescription) = 0;
> 
> Maybe just pass DecodingOptions::DecodingMode or DecodingMode

Ditto.

>> Source/WebCore/platform/graphics/ImageFrame.h:130
>> +    bool hasIntrinsicNativeImage(const std::optional<SubsamplingLevel>& = { }) const;
> 
> hasFullSizeNativeImage()

Done. Function was renamed.

>> Source/WebCore/platform/graphics/ImageFrame.h:131
>> +    bool hasAsyncNativeImage(const std::optional<SubsamplingLevel>&, const DecodingOptions&) const;
> 
> hasImageCompatibleWithOptions()

Done. Function was renamed to hasNativeImageCompatibleWithOptions(). I just added "Native" to be specific about what image we deal with and to be compatible with other functions.

>> Source/WebCore/platform/graphics/ImageFrame.h:132
>> +    bool hasCompleteAsyncNativeImage(const std::optional<SubsamplingLevel>&, const DecodingOptions&) const;
> 
> How is this different from the one above? Should this be hasImageCompatibleWithOptions() and the previous one be hasOrIsDecodingImageCompatibleWithOptions()?

This function is not needed. I added it to this patch but I did not use it. I removed it.

>> Source/WebCore/platform/graphics/ImageFrameCache.h:96
>> +    bool frameHasIntrinsicNativeImageAtIndex(size_t, const std::optional<SubsamplingLevel>&);
> 
> frameHasFullSizeNativeImageAtIndex

Done. Function was renamed.

>> Source/WebCore/platform/graphics/ImageSource.h:96
>>      SubsamplingLevel frameSubsamplingLevelAtIndex(size_t index) { return m_frameCache->frameSubsamplingLevelAtIndex(index); }
> 
> Match the BitmapImage names where possible.

Done. Names were matched.

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:375
>> +    if (!decodingOptions.isSynchronous()) {
> 
> is that the same as decodingOptions.isAsynchronous()?

No they are not the same. !decodingOptions.isSynchronous() == decodingOptions.isAsynchronous() || decodingOptions.hasSize().

[Simon Fraser (smfr)]

Comment on attachment 305438 [details]
Patch

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

>>> Source/WebCore/platform/graphics/BitmapImage.h:85
>>> +    bool frameIsBeingDecodedCompatibleWithOptionsAtIndex(size_t index, const DecodingOptions& decodingOptions) const { return m_source.frameIsBeingDecodedCompatibleWithOptionsAtIndex(index, decodingOptions); }
>> 
>> frameIsBeingDecodedCompatibleWithOptionsAtIndex -> frameBeingDecodedIsCompatibleWithOptions (I don' think the "at index" is necessary).
>> 
>> What does this return if no frame is being decoded?
> 
> Function was renamed to frameBeingDecodedIsCompatibleWithOptionsAtIndex(). I will remove "atIndex" from BitmapImage, ImageSource and ImageFrameCache functions in a separate patch. But let's keep everything consistent for now.
> 
> It will return false. This is the function that does the real work. If no frame is being decoded, m_frameCommitQueue will be empty which means it will return false.
> 
> bool ImageFrameCache::frameBeingDecodedIsCompatibleWithOptionsAtIndex(size_t index, const DecodingOptions& decodingOptions)
> {
>     auto it = std::find_if(m_frameCommitQueue.begin(), m_frameCommitQueue.end(), [index, &decodingOptions](const ImageFrameRequest& frameRequest) {
>         return frameRequest.index == index && frameRequest.decodingOptions.isAsynchronousCompatibleWith(decodingOptions);
>     });
>     return it != m_frameCommitQueue.end();
> }

So the right name is frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex()

[Said Abou-Hallawa]

Created attachment 305522 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 305522 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:173
> +    if (decodingMode == DecodingMode::Asynchronous && shouldUseAsyncDecodingForLargeImage()) {

shouldUseAsyncDecodingForLargeImage should be shouldUseAsyncDecodingForLargeImages

> Source/WebCore/platform/graphics/BitmapImage.cpp:176
> +        // Check if a NativeImage is available for this sizeForDrawing.

Now that the function name is better, you don't need the comment.

> Source/WebCore/platform/graphics/BitmapImage.cpp:178
> +        if (!frameHasDecodedNativeImageCompatibleWithOptionsAtIndex(m_currentFrame, m_currentSubsamplingLevel, DecodingOptions(sizeForDrawing))) {
> +            if (!frameIsBeingDecodedAndIsCompatibleWithOptionsAtIndex(m_currentFrame, DecodingOptions(sizeForDrawing))) {

These could be tested in one if () clause.

[Said Abou-Hallawa]

Created attachment 305529 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305529 [details]
Patch

Rejecting attachment 305529 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 305529, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests
:040000 040000 b5f57fe68993ce91cf47dfa7f7526915797f6755 7318652aa77b2cc5f169f3e17e46f3967f7d0b27 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3420671

[Said Abou-Hallawa]

Created attachment 305537 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305537 [details]
Patch

Clearing flags on attachment: 305537

Committed r214450: <http://trac.webkit.org/changeset/214450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.