Bug 169771 - REGRESSION(213764): Large images should not be decoded asynchronously when they are drawn on a canvas
Summary: REGRESSION(213764): Large images should not be decoded asynchronously when th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 170107 (view as bug list)
Depends on:
Blocks: 169547 170275
  Show dependency treegraph
 
Reported: 2017-03-16 11:48 PDT by Said Abou-Hallawa
Modified: 2017-03-30 02:06 PDT (History)
7 users (show)

See Also:


Attachments
Patch (96.91 KB, patch)
2017-03-19 21:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (97.12 KB, patch)
2017-03-20 00:59 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (911.87 KB, application/zip)
2017-03-20 02:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.73 MB, application/zip)
2017-03-20 02:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (867.56 KB, application/zip)
2017-03-20 04:32 PDT, Build Bot
no flags Details
Patch (97.70 KB, patch)
2017-03-20 12:53 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (109.87 KB, patch)
2017-03-20 18:26 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (117.45 KB, patch)
2017-03-22 21:26 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (118.16 KB, patch)
2017-03-23 11:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (118.16 KB, patch)
2017-03-23 12:32 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (129.44 KB, patch)
2017-03-24 23:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.72 MB, application/zip)
2017-03-25 02:40 PDT, Build Bot
no flags Details
Patch (125.20 KB, patch)
2017-03-26 17:22 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (125.03 KB, patch)
2017-03-27 15:52 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (125.77 KB, patch)
2017-03-27 17:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (129.89 KB, patch)
2017-03-27 18:35 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-03-16 11:48:29 PDT
It seems once the images finish decoding the repaint is dropped to floor. Hovering or resizing forces the needed repaint.
Comment 1 Said Abou-Hallawa 2017-03-16 11:49:15 PDT
<rdar://problem/31092794>
Comment 2 Said Abou-Hallawa 2017-03-19 21:56:04 PDT
Created attachment 304914 [details]
Patch
Comment 3 Said Abou-Hallawa 2017-03-20 00:59:04 PDT
Created attachment 304918 [details]
Patch
Comment 4 Build Bot 2017-03-20 02:12:23 PDT
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
Comment 5 Build Bot 2017-03-20 02:12:30 PDT
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
Comment 6 Build Bot 2017-03-20 02:21:37 PDT
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
Comment 7 Build Bot 2017-03-20 02:21:40 PDT
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
Comment 8 Build Bot 2017-03-20 04:32:18 PDT
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
Comment 9 Build Bot 2017-03-20 04:32:21 PDT
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
Comment 10 Said Abou-Hallawa 2017-03-20 12:53:29 PDT
Created attachment 304942 [details]
Patch
Comment 11 Simon Fraser (smfr) 2017-03-20 13:25:15 PDT
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?
Comment 12 Simon Fraser (smfr) 2017-03-20 13:25:47 PDT
Also, is this actually about images painted into <canvas>? If so, please retitle.
Comment 13 Said Abou-Hallawa 2017-03-20 18:26:44 PDT
Created attachment 304979 [details]
Patch
Comment 14 Said Abou-Hallawa 2017-03-22 21:26:00 PDT
Created attachment 305161 [details]
Patch
Comment 15 Said Abou-Hallawa 2017-03-23 11:34:24 PDT
Created attachment 305206 [details]
Patch
Comment 16 Simon Fraser (smfr) 2017-03-23 11:43:38 PDT
Please fix the title to explain that this is about <canvas>
Comment 17 Said Abou-Hallawa 2017-03-23 12:32:52 PDT
Created attachment 305214 [details]
Patch
Comment 18 Said Abou-Hallawa 2017-03-23 12:44:43 PDT
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.
Comment 19 Simon Fraser (smfr) 2017-03-23 16:21:45 PDT
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?).
Comment 20 Said Abou-Hallawa 2017-03-24 23:28:17 PDT
Created attachment 305367 [details]
Patch
Comment 21 Build Bot 2017-03-25 02:40:09 PDT
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
Comment 22 Build Bot 2017-03-25 02:40:11 PDT
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
Comment 23 Said Abou-Hallawa 2017-03-26 17:22:11 PDT
Created attachment 305438 [details]
Patch
Comment 24 Said Abou-Hallawa 2017-03-26 19:34:43 PDT
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.
Comment 25 Said Abou-Hallawa 2017-03-27 09:03:33 PDT
*** Bug 170107 has been marked as a duplicate of this bug. ***
Comment 26 Simon Fraser (smfr) 2017-03-27 12:28:20 PDT
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()?
Comment 27 Said Abou-Hallawa 2017-03-27 14:17:18 PDT
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().
Comment 28 Simon Fraser (smfr) 2017-03-27 14:24:55 PDT
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()
Comment 29 Said Abou-Hallawa 2017-03-27 15:52:55 PDT
Created attachment 305522 [details]
Patch
Comment 30 Simon Fraser (smfr) 2017-03-27 16:50:37 PDT
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.
Comment 31 Said Abou-Hallawa 2017-03-27 17:40:24 PDT
Created attachment 305529 [details]
Patch
Comment 32 WebKit Commit Bot 2017-03-27 18:22:16 PDT
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
Comment 33 Said Abou-Hallawa 2017-03-27 18:35:49 PDT
Created attachment 305537 [details]
Patch
Comment 34 WebKit Commit Bot 2017-03-27 19:23:13 PDT
Comment on attachment 305537 [details]
Patch

Clearing flags on attachment: 305537

Committed r214450: <http://trac.webkit.org/changeset/214450>
Comment 35 WebKit Commit Bot 2017-03-27 19:23:19 PDT
All reviewed patches have been landed.  Closing bug.