Bug 171016 – Implement the imageready event to reliably test the async image decoding

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This event will fire when the image frame finishes decoding. The layout test can then call testRunner.notifyDone() and be sure the image will be drawn.

NOTE: I am still not sure about the name of the event and I am not sure also if it will exposed or be internally limited. I will fix things from the review feedback.

[Said Abou-Hallawa]

Created attachment 307515 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/30452288>

[Simon Fraser (smfr)]

Comment on attachment 307515 [details]
Patch

Does this need to be behind a setting?

[Build Bot]

Comment on attachment 307515 [details]
Patch

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

New failing tests:
fast/images/sprite-sheet-image-draw.html
js/dom/dom-static-property-for-in-iteration.html
fast/images/async-image-background-image-repeated.html
fast/images/async-image-background-image.html

[Build Bot]

Created attachment 307525 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 307515 [details]
Patch

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

New failing tests:
js/dom/dom-static-property-for-in-iteration.html

[Build Bot]

Created attachment 307528 [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

[Build Bot]

Comment on attachment 307515 [details]
Patch

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

New failing tests:
fast/images/sprite-sheet-image-draw.html
js/dom/dom-static-property-for-in-iteration.html
fast/images/async-image-background-image.html
fast/images/async-image-background-image-repeated.html

[Build Bot]

Created attachment 307532 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Said Abou-Hallawa]

Created attachment 308655 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308658 [details]
Patch

[Build Bot]

Comment on attachment 308658 [details]
Patch

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

New failing tests:
transitions/crossfade-transition.html
js/dom/dom-static-property-for-in-iteration.html
svg/as-list-image/svg-list-image-intrinsic-size-1.html

[Build Bot]

Created attachment 308682 [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 308658 [details]
Patch

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

New failing tests:
transitions/crossfade-transition.html
js/dom/dom-static-property-for-in-iteration.html
svg/as-list-image/svg-list-image-intrinsic-size-1.html

[Build Bot]

Created attachment 308683 [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

[Build Bot]

Comment on attachment 308658 [details]
Patch

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

New failing tests:
transitions/crossfade-transition.html
js/dom/dom-static-property-for-in-iteration.html
svg/as-list-image/svg-list-image-intrinsic-size-1.html

[Build Bot]

Created attachment 308685 [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

[Build Bot]

Comment on attachment 308658 [details]
Patch

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

New failing tests:
transitions/crossfade-transition.html
svg/as-list-image/svg-list-image-intrinsic-size-1.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 308658 [details]
Patch

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

New failing tests:
transitions/crossfade-transition.html
svg/as-list-image/svg-list-image-intrinsic-size-1.html

[Build Bot]

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

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

[Said Abou-Hallawa]

Created attachment 308709 [details]
Patch

[Sam Weinig]

Comment on attachment 308709 [details]
Patch

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

> Source/WebCore/dom/GlobalEventHandlers.idl:105
> +    [NotEnumerable] attribute EventHandler onimageframeready;

This probably needs to be conditionally available if you are trying to make this runtime enabled.

[Said Abou-Hallawa]

Created attachment 308773 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308788 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 308788 [details]
Patch

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

> Source/WTF/wtf/FeatureDefines.h:769
> +#if !defined(ENABLE_IMAGE_FRAME_READY)
> +#define ENABLE_IMAGE_FRAME_READY 1
> +#endif

This turns it on unconditionally for all platforms. Is that the intent?

Also I think ENABLE_IMAGE_FRAME_READY should be ENABLE_IMAGE_FRAME_READY_EVENT

> Source/WebCore/html/HTMLAttributeNames.in:282
> +onimageframeready

Are we still adding "on" attributes for new event types? Please check with cdumez.

> Source/WebCore/rendering/RenderElement.cpp:1512
> +    if (element() && image.image()->isBitmapImage())
> +        element()->dispatchImageFrameReadyEvent();

This will fire on every frame of a GIF. Is that right?

> Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:150
> +#define WebKitImageFrameReadyEnabledPreferenceKey @"WebKitImageFrameReadyEnabled"

I don't think you need any of the WK1/WK2 prefs.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:133
> +    void setImageFrameReadyEnabled(bool);

This should be enabled through InternalSettings (see setWebGL2Enabled), not testRunner.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:144
> +    void display(bool noTrackRepaints);

It's sad that this has a negative name, and causes tests to fall into the boolean trap ("display(false)").

I think it would be better to use an enum (see Internals.idl for examples). Sadly the DRT bindings code will have to be written by hand.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #26)
> Comment on attachment 308788 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308788&action=review
> 
> > Source/WTF/wtf/FeatureDefines.h:769
> > +#if !defined(ENABLE_IMAGE_FRAME_READY)
> > +#define ENABLE_IMAGE_FRAME_READY 1
> > +#endif
> 
> This turns it on unconditionally for all platforms. Is that the intent?
> 

Is there any reason to add something to FeatureDefines.h? Why do we have this still?

[Said Abou-Hallawa]

Created attachment 308870 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 308788 [details]
Patch

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

>>> Source/WTF/wtf/FeatureDefines.h:769
>>> +#endif
>> 
>> This turns it on unconditionally for all platforms. Is that the intent?
>> 
>> Also I think ENABLE_IMAGE_FRAME_READY should be ENABLE_IMAGE_FRAME_READY_EVENT
> 
> Is there any reason to add something to FeatureDefines.h? Why do we have this still?

Yes the event has to be turned on for all platforms otherwise the tests will timeout on other platforms.

The event is not spec-ed yet and I think it is helpful to surround the event code with the enable macro for two reasons. (1) It can be turned off by just changing the macro value. (2) It makes it clear that this code is still experimental.

>> Source/WebCore/html/HTMLAttributeNames.in:282
>> +onimageframeready
> 
> Are we still adding "on" attributes for new event types? Please check with cdumez.

The attribute was deleted.

>> Source/WebCore/rendering/RenderElement.cpp:1512
>> +        element()->dispatchImageFrameReadyEvent();
> 
> This will fire on every frame of a GIF. Is that right?

I think this is the right behavior. imageFrameReady should fire for static and animated images with the following behavior:

1. Static images: the decoded frame is ready, the image clients are repainted and the decoded frame will be drawn during the next drawing phase.
2. Animated images: the decoded frame is ready but we have two cases for repainting the image clients:
    a) If the current frame duration was elapsed, the image clients are repainted and the decoded frame will be drawn during the next drawing phase. 
   b)  If the current frame duration wasn't elapsed, the images clients will be repainted once the current frame timer fired.

By doing that the event handler can be sure if it asks to draw the image, no decoded will be done.

>> Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h:150
>> +#define WebKitImageFrameReadyEnabledPreferenceKey @"WebKitImageFrameReadyEnabled"
> 
> I don't think you need any of the WK1/WK2 prefs.

They were deleted.

>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:133
>> +    void setImageFrameReadyEnabled(bool);
> 
> This should be enabled through InternalSettings (see setWebGL2Enabled), not testRunner.

Done.

>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:144
>> +    void display(bool noTrackRepaints);
> 
> It's sad that this has a negative name, and causes tests to fall into the boolean trap ("display(false)").
> 
> I think it would be better to use an enum (see Internals.idl for examples). Sadly the DRT bindings code will have to be written by hand.

Done but with a little bit of hacks

[Said Abou-Hallawa]

Created attachment 308875 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 308875 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add the imageframeready event to the DOM Element to reliably test the async image decoding

"to the DOM Element" sounds weird. Maybe just "implement the imageframeready event to reliably test..."

I'm looking at https://github.com/whatwg/html/issues/1920 and don't see a specific proposal for the name of this event. Did you just make up "imageframeready"? Should it just be "ready"?

> Source/WebCore/rendering/RenderElement.cpp:1512
> +    if (element() && image.image()->isBitmapImage())
> +        element()->dispatchImageFrameReadyEvent();

This will dispatch the event for CSS background images as well as <img> which is certainly not what the spec will say. Can we restrict it to RenderImage for now?

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:117
> +void TestRunner::display(DisplayOptions* options)

It's ugly taking a pointer to an enum. Can't the caller handle the default case, or use a std::optional?

[Simon Fraser (smfr)]

I think we have to decide if we're implementing something close to the proposed "ready" event, or something purely for testing (which also works for CSS images).

[Said Abou-Hallawa]

Comment on attachment 308875 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Add the imageframeready event to the DOM Element to reliably test the async image decoding
> 
> "to the DOM Element" sounds weird. Maybe just "implement the imageframeready event to reliably test..."
> 
> I'm looking at https://github.com/whatwg/html/issues/1920 and don't see a specific proposal for the name of this event. Did you just make up "imageframeready"? Should it just be "ready"?

The purpose of this patch is not to implement this proposal. The purpose is to have a reliable way to test async image decoding. The proposal is not exactly what I want. These are the differences which I disagree with or you can say they do not fit my requirements:

1. This event should fire for image elements and css image container elements. Otherwise I would not be able to test the background image. I really don't understand what is the difference between <img src="image.png"> and <div style="background-image: url(image.png);"></div>? The should look the same and I would expect they should behave the same. Even our code treats them equally when considering both of them clients of the same CachedImage.
2. The proposal suggests that the image could only be decoded asynchronously if the "async" attribute is included with the image element. The current implementation depends on a simple heuristic which decides if an image is worth async decoding or not.
3. The proposal suggests that the event fires only once per image. But the tests require the event to fire multiple times per image. Examples for this are the animated images and the incomplete frames. In these cases, the tests need to be aware of what decoding activities are being carried out.
4. The name "ready" is not very descriptive especially when it is used for css background image element containers. Can we name imageready?
5. I think the event should also provide more information about the image and the image frame; e.g. source url, image frame index and frame duration. I will need this information to make the animated images tests reliable. Currently some of the animated image tests are flaky.

I think the scope of this patch should be as stated in the title "....to reliably test the async image decoding". I am planning to add my comments and findings to the proposal once I use the event and test it and have more understanding about the event timing.

>> Source/WebCore/rendering/RenderElement.cpp:1512
>> +        element()->dispatchImageFrameReadyEvent();
> 
> This will dispatch the event for CSS background images as well as <img> which is certainly not what the spec will say. Can we restrict it to RenderImage for now?

If I do that I will not be able to reliably test the async image decoding for background images; like the test cases below. Can we keep it like that and restrict it later for the non testing case? This event can only be dispatched if it is enabled from Internals. There is no way for a web developer to use it right now.

>> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:117
>> +void TestRunner::display(DisplayOptions* options)
> 
> It's ugly taking a pointer to an enum. Can't the caller handle the default case, or use a std::optional?

This is the caller from WebKitTestRunner/InjectedBundle/Derived Sources/JSTestRunner.cpp

JSValueRef JSTestRunner::display(JSContextRef context, JSObjectRef, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
{
    TestRunner* impl = toTestRunner(context, thisObject);
    if (!impl)
        return JSValueMakeUndefined(context);

    DisplayOptions* options = argumentCount > 0 ? toDisplayOptions(context, arguments[0]) : 0;
    impl->display(options);

    return JSValueMakeUndefined(context);
}

So I have also to implement toDisplayOptions like this: inline DisplayOptions* toDisplayOptions(JSContextRef context, JSValueRef value).

I can fix the code generator or switch it to the code generator which is used for Internals.idl. But can we please do this is a separate patch just to limit this patch for what it is supposed to do?

[Said Abou-Hallawa]

Created attachment 308923 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 308923 [details]
Patch

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

> Source/WTF/wtf/FeatureDefines.h:769
> +#if !defined(ENABLE_IMAGE_READY_EVENT)
> +#define ENABLE_IMAGE_READY_EVENT 1
> +#endif

Remove this.

> Source/WebCore/dom/Element.cpp:2516
> +        dispatchEvent(Event::create(eventNames().imagereadyEvent, true, true));

I think this can just be dispatchEvent(Event::create("webkitImageFrameReady", true, true));

> Source/WebCore/dom/Element.h:494
> +    void dispatchImageReadyEvent();

Rename with "ForTesting"

> Source/WebCore/dom/EventNames.h:249
> +    macro(imageready) \

Remove.

> Source/WebCore/page/RuntimeEnabledFeatures.h:219
> +#if ENABLE(IMAGE_READY_EVENT)
> +    void setImageReadyEventEnabled(bool isEnabled) { m_isImageReadyEventEnabled = isEnabled; }
> +    bool imageReadyEventEnabled() const { return m_isImageReadyEventEnabled; }
> +#endif

This becomes a Setting: webkitImageReadyEventEnabled

> Source/WebCore/testing/InternalSettings.cpp:114
> +#if ENABLE(IMAGE_READY_EVENET)
> +    , m_imageReadyEventEnabled(RuntimeEnabledFeatures::sharedFeatures().imageReadyEventEnabled())
> +#endif
>  {

Remove.

> Source/WebCore/testing/InternalSettings.cpp:204
> +#if ENABLE(IMAGE_READY_EVENET)
> +    RuntimeEnabledFeatures::sharedFeatures().setImageReadyEventEnabled(m_imageReadyEventEnabled);
> +#endif

Remove.

> Source/WebCore/testing/InternalSettings.cpp:727
> +void InternalSettings::setImageReadyEventEnabled(bool enabled)
> +{
> +#if ENABLE(IMAGE_READY_EVENT)
> +    RuntimeEnabledFeatures::sharedFeatures().setImageReadyEventEnabled(enabled);
> +#else
> +    UNUSED_PARAM(enabled);
> +#endif
> +}

Remove.

> Source/WebCore/testing/InternalSettings.h:120
> +    static void setImageReadyEventEnabled(bool);

Remove.

> Source/WebCore/testing/InternalSettings.h:198
> +        bool m_imageReadyEventEnabled;

Remove

> Source/WebCore/testing/InternalSettings.idl:90
> +    void setImageReadyEventEnabled(boolean enabled);

Remove.

> Tools/DumpRenderTree/DumpRenderTree.h:57
> +enum class DisplayOptions {
> +    ContentsAndTrackRepaints,
> +    ContentsOnly
> +};

Let's move this to a separate patch.

[Said Abou-Hallawa]

Created attachment 308971 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308974 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308974 [details]
Patch

Clearing flags on attachment: 308974

Committed r216154: <http://trac.webkit.org/changeset/216154>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.