Bug 178039 – HTMLImageElement.decode() should optimize the decoding for the image explicit size

• Component: Images
• Status: NEW

[Said Abou-Hallawa]

If the HTMLImageElement has an intrinsic size, the decode() method should use it instead of decoding the image for the native size.

The intrinsic size of an HTMLImageElement can be set to the width and the height attributes of the <img> element. Example is:
    <img width="200" height="100>

Or it can be set from the javascript. Example is:
    var image = new Image(200, 100);

If the intrinsic size is used, decoding the image and drawing it will be much faster than using the native size. This is because a large image will not be scaled down when it is drawn into a smaller rectangle.

[Said Abou-Hallawa]

Created attachment 323074 [details]
Patch

[Build Bot]

Comment on attachment 323074 [details]
Patch

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

New failing tests:
fast/images/decode-animated-image.html
fast/images/decode-render-animated-image.html

[Build Bot]

Created attachment 323078 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Build Bot]

Comment on attachment 323074 [details]
Patch

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

New failing tests:
fast/images/decode-animated-image.html
fast/images/decode-render-animated-image.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 323074 [details]
Patch

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

New failing tests:
fast/images/decode-animated-image.html
fast/images/decode-render-animated-image.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 323074 [details]
Patch

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

New failing tests:
fast/images/decode-animated-image.html
fast/images/decode-render-animated-image.html

[Build Bot]

Created attachment 323096 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

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

[Said Abou-Hallawa]

Created attachment 323158 [details]
Patch

[Said Abou-Hallawa]

Created attachment 323192 [details]
Patch

[Said Abou-Hallawa]

Created attachment 324757 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 324757 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:440
> +    return std::nullopt;

I think this can be 'return { }'

> Source/WebCore/html/HTMLImageElement.cpp:447
> +    return std::nullopt;

Ditto.

> Source/WebCore/html/HTMLImageElement.h:53
> +    std::optional<unsigned> intrinsicWidth() const;
> +    std::optional<unsigned> intrinsicHeight() const;

I'm not super keen on the names of these functions because "intrinsic size" has a specific meaning in CSS, and this isn't the same; all it does is read the attribute if present. Maybe this is more like "explicitWidth" or "widthFromAttribute"?

> Source/WebCore/platform/graphics/BitmapImage.cpp:516
> +    // Animated images should be decoded with the native size always.

Comment should say why.

> Source/WebCore/platform/graphics/BitmapImage.cpp:543
> +    Vector<size_t> itemsToBeDeleted;

This is a vector of indexes, not of items (or requests).

> Source/WebCore/platform/graphics/BitmapImage.cpp:544
> +    for (auto it = m_decodingRequests->begin(); it != m_decodingRequests->end(); ++it) {

Use a modern C++ loop: for (auto request : *m_decodingRequests) ?

> Source/WebCore/platform/graphics/BitmapImage.cpp:549
> +        it->callback();

What if the callback modifies m_decodingRequests? I think you should copy m_decodingRequests before iterating it.

> Source/WebCore/platform/graphics/BitmapImage.cpp:562
> +        m_decodingRequests->remove(itemsToBeDeleted.takeLast());

Is this removing by index or by item? Confused.

[Said Abou-Hallawa]

Created attachment 324945 [details]
Patch

[Tim Horton]

Isn’t the wording here wrong? You’re now deciding at the target resolution, explicitly NOT the intrinsic size of the image.

[Said Abou-Hallawa]

Comment on attachment 324757 [details]
Patch

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

>> Source/WebCore/html/HTMLImageElement.cpp:440
>> +    return std::nullopt;
> 
> I think this can be 'return { }'

Done.

>> Source/WebCore/html/HTMLImageElement.cpp:447
>> +    return std::nullopt;
> 
> Ditto.

Done.

>> Source/WebCore/html/HTMLImageElement.h:53
>> +    std::optional<unsigned> intrinsicHeight() const;
> 
> I'm not super keen on the names of these functions because "intrinsic size" has a specific meaning in CSS, and this isn't the same; all it does is read the attribute if present. Maybe this is more like "explicitWidth" or "widthFromAttribute"?

Names were changed to explicitWidth() and explicitHeight().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:516
>> +    // Animated images should be decoded with the native size always.
> 
> Comment should say why.

The comment was changed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:543
>> +    Vector<size_t> itemsToBeDeleted;
> 
> This is a vector of indexes, not of items (or requests).

The name of vector was renamed to indicesOfItemsToBeDeleted.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:544
>> +    for (auto it = m_decodingRequests->begin(); it != m_decodingRequests->end(); ++it) {
> 
> Use a modern C++ loop: for (auto request : *m_decodingRequests) ?

Done. I used the classic for-loop because I wanted to calculate the index of the element =  it - m_decodingRequests->begin(). I now calculate the index of the element = &request - &m_decodingRequests->first().

>> Source/WebCore/platform/graphics/BitmapImage.cpp:549
>> +        it->callback();
> 
> What if the callback modifies m_decodingRequests? I think you should copy m_decodingRequests before iterating it.

There is only caller for the decode() method which is ImageLoader::decode(). The ImageLoader passes a callback which resolves a promise. Resolving the promise can't result in reentering BitmapImage::decode(). Drain asked this question before and I answered it in https://bugs.webkit.org/show_bug.cgi?id=176732#c5.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:562
>> +        m_decodingRequests->remove(itemsToBeDeleted.takeLast());
> 
> Is this removing by index or by item? Confused.

Name of the vector was changed to indicesOfItemsToBeDeleted.

[Tim Horton]

(In reply to Tim Horton from comment #15)
> Isn’t the wording here wrong? You’re now deciding at the target resolution,
> explicitly NOT the intrinsic size of the image.

Deciding=decoding, and I see smfr raised the same complaint. Bug title is still wrong, though.

[Said Abou-Hallawa]

Created attachment 324946 [details]
Patch

[Said Abou-Hallawa]

Created attachment 325028 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 324946 [details]
Patch

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

Can we test this change?

> Source/WebCore/platform/graphics/BitmapImage.cpp:554
> +    for (const auto& request : *m_decodingRequests) {
> +        if (!frameHasDecodedNativeImageCompatibleWithOptionsAtIndex(m_currentFrame, m_currentSubsamplingLevel, request.sizeForDrawing))
> +            continue;
> +        
> +        // Call the request callback and mark this request for deletion.
> +        request.callback();
> +        indicesOfItemsToBeDeleted.append(&request - &m_decodingRequests->first());

Since you're getting the index here, the loop would be much cleaner as just for (size_t i = 0; i < m_decodingRequests.size(); ++i)

You say that calling the callback while traversing m_decodingRequests is safe because promise resolution isn't synchronous. However, this doesn't protect us against our own coding mistakes in C++ where we later change our code to modify m_decodingRequests inside the callback. I think you should copy the vector.

> Source/WebCore/platform/graphics/BitmapImage.cpp:566
> +    while (!indicesOfItemsToBeDeleted.isEmpty())
> +        m_decodingRequests->remove(indicesOfItemsToBeDeleted.takeLast());

Why bother to mutate (indicesOfItemsToBeDeleted? Just run through the list.

> Source/WebCore/platform/graphics/BitmapImage.h:175
> +    bool hasStartedAnimating() const { return bool(m_desiredFrameStartTime); }

I would find m_desiredFrameStartTime != 0 more readable.

[Build Bot]

Comment on attachment 325028 [details]
Patch

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

New failing tests:
fast/xsl/xslt-enc.xml

[Build Bot]

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

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

[Said Abou-Hallawa]

Created attachment 325065 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 324946 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:554
>> +        indicesOfItemsToBeDeleted.append(&request - &m_decodingRequests->first());
> 
> Since you're getting the index here, the loop would be much cleaner as just for (size_t i = 0; i < m_decodingRequests.size(); ++i)
> 
> You say that calling the callback while traversing m_decodingRequests is safe because promise resolution isn't synchronous. However, this doesn't protect us against our own coding mistakes in C++ where we later change our code to modify m_decodingRequests inside the callback. I think you should copy the vector.

I changed the for loop as suggested.

But I could not copy the Vector m_decodingRequests because the WTF::Function is not copyable. I tried to think of all the cases that can wrong with m_decodingRequests and I realized even using a vector of indices is wrong. If callDecodingCallbacks() is reentrant from callDecodingCallbacks() this will mess up all the indices. I tried to come up with a solution which can stand up against all the bugs which may introduce because of mistakes in our C++ code. Then I felt it's too much complicated for bug that we know for sure it does not exist.

Instead I can add assertion that 

callDecodingCallbacks() is not reentrant from callDecodingCallbacks()
decode() is not reentrant from either decode() or callDecodingCallbacks()

>> Source/WebCore/platform/graphics/BitmapImage.cpp:566
>> +        m_decodingRequests->remove(indicesOfItemsToBeDeleted.takeLast());
> 
> Why bother to mutate (indicesOfItemsToBeDeleted? Just run through the list.

Done.

>> Source/WebCore/platform/graphics/BitmapImage.h:175
>> +    bool hasStartedAnimating() const { return bool(m_desiredFrameStartTime); }
> 
> I would find m_desiredFrameStartTime != 0 more readable.

Done.

[Said Abou-Hallawa]

Created attachment 325071 [details]
Patch

[Build Bot]

Attachment 325071 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/BitmapImage.h:175:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Said Abou-Hallawa]

Created attachment 325094 [details]
Patch

[Said Abou-Hallawa]

Created attachment 325171 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 324946 [details]
Patch

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

>>> Source/WebCore/platform/graphics/BitmapImage.h:175
>>> +    bool hasStartedAnimating() const { return bool(m_desiredFrameStartTime); }
>> 
>> I would find m_desiredFrameStartTime != 0 more readable.
> 
> Done.

It has to be m_desiredFrameStartTime != MonotonicTime() because the conversion constructor MonotonicTime(double rawValue) is private.

[Said Abou-Hallawa]

Created attachment 325640 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #30)
> Created attachment 325640 [details]
> Patch

Tim suggested a solution for protecting m_decodingRequests while calling the decoding callbacks. We can move m_decodingRequests to a local protectedDecodingRequests. See which ones are compatible with the decoded frame. Call their callbacks. Delete these compatible requests from the local protectedDecodingRequests. Move the rest back to m_decodingRequests once we finish. I implemented this solution in this patch.

[Maciej Stachowiak]

Comment on attachment 325640 [details]
Patch

Unfortunately, this old patch no longer applies.