Bug 176016 – Implement HTMLImageElement.decode() method

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The specs is: https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode.

[Said Abou-Hallawa]

Created attachment 319170 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/34111882>

[Said Abou-Hallawa]

Created attachment 319185 [details]
Patch

[Sam Weinig]

Comment on attachment 319185 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:390
> +void ImageLoader::decode(Ref<DeferredPromise>&& promise)
> +{
> +    m_decodingPromise = WTFMove(promise);
> +
> +    if (m_image->errorOccurred()) {
> +        decodeError();
> +        return;
> +    }
> +
> +    if (m_image->image() && m_imageComplete)
> +        decode();
> +}

What happens if you call image.decode() twice in a row? Won't this override m_decodingPromise?

[Sam Weinig]

Comment on attachment 319185 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:544
> +void HTMLImageElement::decode(Ref<DeferredPromise>&& promise)
> +{
> +    return m_imageLoader.decode(WTFMove(promise));
> +}

This doesn't seem to implement https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode-2.  Specifically checking for no src or a srcset attributes / src with empty string.  Is the idea that the lower levels will handle this intrinsically? You should also add tests for more of the error conditions.

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 319232 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 319185 [details]
Patch

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

>> Source/WebCore/html/HTMLImageElement.cpp:544
>> +}
> 
> This doesn't seem to implement https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decode-2.  Specifically checking for no src or a srcset attributes / src with empty string.  Is the idea that the lower levels will handle this intrinsically? You should also add tests for more of the error conditions.

Error cases are handled in ImageLoader::decode(). The reason is the ImageLoader has all the information we need to either resolve or reject the promise.

>> Source/WebCore/loader/ImageLoader.cpp:390
>> +}
> 
> What happens if you call image.decode() twice in a row? Won't this override m_decodingPromise?

The promises are now collected in a Vector. When the decode() succeeds, all the promises are resolved. When the decode() fails all the promise are rejected.

[Simon Fraser (smfr)]

Comment on attachment 319232 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        -- If the image frame is already decoded, the promise will be resolved
> +        before return.

Is this correct, or should we resolve the promise on the next event loop?

> Source/WebCore/platform/graphics/BitmapImage.cpp:510
> +    if (m_decodeCallback)
> +        return;
> +
> +    m_decodeCallback = WTFMove(callback);

I'm not sure why you early return if there's already a callback?

> Source/WebCore/platform/graphics/BitmapImage.cpp:522
> +        }

Maybe return here, or do if (canAnimate()) { ... return; }

> Source/WebCore/platform/graphics/BitmapImage.cpp:571
> +    // Call the m_decodeCallback only if the image frame was decoded with the native size.

If not, will we do another decode to satisfy the promise?

[Said Abou-Hallawa]

Created attachment 320326 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 319232 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        before return.
> 
> Is this correct, or should we resolve the promise on the next event loop?

If we delay resolving  the promise till the next event loop, we can't guarantee the availability of the decoded frame at that time.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:510
>> +    m_decodeCallback = WTFMove(callback);
> 
> I'm not sure why you early return if there's already a callback?

I think you are right. I renamed m_decodeCallback to m_decodingCallbacks and I made it Vector<Function<void()>, 1>. BitmapImage::decode() will append all the callbacks to m_decodingCallbacks. When the frame finishes decoding, all the m_decodingCallbacks will be called one after another. I think this case can be hit if more than one HTMLImageElement reference the same Image object. In this case, their callbacks will be different and they have all have to be called back.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:522
>> +        }
> 
> Maybe return here, or do if (canAnimate()) { ... return; }

Done.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:571
>> +    // Call the m_decodeCallback only if the image frame was decoded with the native size.
> 
> If not, will we do another decode to satisfy the promise?

Yes. BitmapImage::decode() will make a new decoding request if the image frame was decoded with a size different from the native size. This decoding is the one we should make the callbacks for.

[Said Abou-Hallawa]

Created attachment 320328 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 320328 [details]
Patch

Clearing flags on attachment: 320328

Committed r221805: <http://trac.webkit.org/changeset/221805>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 320328 [details]
Patch

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

> Source/WebCore/loader/ImageLoader.cpp:389
> +    if (attr.isNull() || stripLeadingAndTrailingHTMLSpaces(attr).isEmpty()) {

There is no need for the attr.isNull() check here. The other check will also return true in that case. Should just remove it.

> Source/WebCore/loader/ImageLoader.cpp:402
> +    for (auto& promise : m_decodingPromises)
> +        promise->reject(Exception { EncodingError, WTFMove(message) });

If there is more than one promise, this will move the same String twice, and so it won’t work. I think it will crash. Need a test case that covers this.

Also, is there something that guarantees we will not reenter this while rejecting the promises? Can rejecting a promise run arbitrary JavaScript code? What prevents modification of m_decodingPromises while iterating it?

> Source/WebCore/loader/ImageLoader.h:98
> +    void decodeError(String&&);

Every caller of this function passes a const char*. Having it take a String&& instead means all those call sites have repeated code to allocate the String.

I think this function should either take a const char* argument or an ASCIILiteral argument rather than a String&& argument.

> Source/WebCore/platform/graphics/BitmapImage.h:216
> +    Vector<Function<void()>, 1> m_decodingCallbacks;

An empty vector is pretty huge; maybe we should consider optimizing so we aren’t always carrying an empty vector around. One way is to use std::unique_ptr to hold the vector. There are probably other good ways. Or maybe a BitmapImage is already a huge object.

[Darin Adler]

Comment on attachment 320328 [details]
Patch

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

>> Source/WebCore/loader/ImageLoader.cpp:402
>> +        promise->reject(Exception { EncodingError, WTFMove(message) });
> 
> If there is more than one promise, this will move the same String twice, and so it won’t work. I think it will crash. Need a test case that covers this.
> 
> Also, is there something that guarantees we will not reenter this while rejecting the promises? Can rejecting a promise run arbitrary JavaScript code? What prevents modification of m_decodingPromises while iterating it?

No, it won’t crash. We’ll just pass an exception with a null string. Not sure how that looks in JavaScript. Again, need a test case covering it.

[Said Abou-Hallawa]

The followup bug is https://bugs.webkit.org/show_bug.cgi?id=176732