Bug 125715 – Add resize event for HTMLMediaElement

• Component: Media
• Status: RESOLVED FIXED

[Brendan Long]

This was recently added to the spec.

> Whenever the intrinsic width or intrinsic height of the video changes (including, for example, because the selected video track was changed), if the element's readyState attribute is not HAVE_NOTHING, the user agent must queue a task to fire a simple event named resize at the media element. 

http://html5.org/tools/web-apps-tracker?from=8346&to=8347

[Eric Carlson]

Can be merged from blink: https://codereview.chromium.org/103913010/

[Eric Carlson]

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

[Brendan Long]

If you need, it's possible to create multi-resolution videos with GStreamer:

# VP8 / Matroska
gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,width=160,height=120,framerate=24/1 ! vp8enc ! matroskamux name=m ! filesink location=multisize.mkv videotestsrc num-buffers=120 ! video/x-raw,width=320,height=240,framerate=24/1 ! vp8enc ! m.

# h.264 / MP4
# Theoretically at least. This video causes vlc to crash for me :\
gst-launch-1.0 videotestsrc num-buffers=120 ! video/x-raw,width=160,height=120,framerate=24/1 ! x264enc ! mp4mux name=m ! filesink location=multisize.mp4 videotestsrc num-buffers=120 ! video/x-raw,width=320,height=240,framerate=24/1 ! x264enc ! m.

[Brendan Long]

I just found something that should make this easier to test: We're supposed to fire a resize event when the video initially loads.

[Radar WebKit Bug Importer]

<rdar://problem/24079720>

[Jeremy Jones]

Created attachment 269381 [details]
Patch

[Eric Carlson]

Comment on attachment 269381 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests because there is no effect on layout.

Behavior changes need tests too. We should have a test to verify that the new event is posted when an element's size changes.

> Source/WebCore/html/HTMLMediaElement.cpp:4497
> +    if (m_readyState > HAVE_NOTHING)
> +        scheduleEvent(eventNames().resizeEvent);

I don't think we should assume the media engine won't call mediaPlayerSizeChanged when the size hasn't actually changed, so you should keep track of the current size and only fire the event when size does change. 

The spec says the fire the event "if the element's readyState attribute is not HAVE_NOTHING". It isn't likely that this will be called when readyState is HAVE_NOTHING, but you should check.

The spec also says that this event must be fired once the metadata has been loaded, before the 'loadedmetadata' event (see https://html.spec.whatwg.org/#getting-media-metadata), so you need to do that and verify it in the test.

[Jeremy Jones]

(In reply to comment #7)
> Comment on attachment 269381 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269381&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests because there is no effect on layout.
> 
> Behavior changes need tests too. We should have a test to verify that the
> new event is posted when an element's size changes.

Ok. I have a test for normal files and for HLS.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4497
> > +    if (m_readyState > HAVE_NOTHING)
> > +        scheduleEvent(eventNames().resizeEvent);
> 
> I don't think we should assume the media engine won't call
> mediaPlayerSizeChanged when the size hasn't actually changed, so you should
> keep track of the current size and only fire the event when size does
> change. 

Ok. I now check to make sure the size has changed since the last event was posted.

> 
> The spec says the fire the event "if the element's readyState attribute is
> not HAVE_NOTHING". It isn't likely that this will be called when readyState
> is HAVE_NOTHING, but you should check.

Already checking this.

> 
> The spec also says that this event must be fired once the metadata has been
> loaded, before the 'loadedmetadata' event (see
> https://html.spec.whatwg.org/#getting-media-metadata), so you need to do
> that and verify it in the test.

Added a test for this. It raises a question. For HLS, size can still be 0x0 after loadedmetadata. Spec sounds like we should always post resize before loaded metadata even if size is 0x0. The test checks for this behavior. The is an exception to "only firing the event if the size has changed" rule.

Additionally, this change causes a failure in: fast/dom/event-handler-attributes.html
Because this change causes lots of elements to have event handlers for resize. I think I need to minimize the scope of this change to be only for HTMLVideoElement and not HTMLElement in general.

[Jeremy Jones]

Created attachment 269759 [details]
Patch

[Jeremy Jones]

(In reply to comment #8)
> (In reply to comment #7)
> Additionally, this change causes a failure in:
> fast/dom/event-handler-attributes.html
> Because this change causes lots of elements to have event handlers for
> resize. I think I need to minimize the scope of this change to be only for
> HTMLVideoElement and not HTMLElement in general.

per https://html.spec.whatwg.org/multipage/webappapis.html#globaleventhandlers
I moved "resize" to be a global event handler to match spec instead of trying
minimize impact of this change. This also matches how the other media element events are implemented.

[Eric Carlson]

Comment on attachment 269759 [details]
Patch

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

the media-specific part of this patch looks OK to me, but someone that knows more about moving the attribute from DOMWindow.idl to GlobalEventHandlers should take a look.

> Source/WebCore/html/HTMLVideoElement.h:44
> +    virtual void postResizeEventIfNecessary(bool force) override;

Nit: "virtual" isn't necessary, it is implied by "override"

[Build Bot]

Comment on attachment 269759 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
media/track/track-kind.html
media/video-resize.html
media/track/track-cue-mutable-fragment.html
media/track/track-active-cues.html

[Build Bot]

Created attachment 269767 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 269759 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html
media/track/track-kind.html
media/video-resize.html
media/track/track-cue-mutable-fragment.html
media/track/track-active-cues.html

[Build Bot]

Created attachment 269771 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 269759 [details]
Patch

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

New failing tests:
media/track/track-kind.html
media/video-resize.html
media/track/track-cue-mutable-fragment.html
media/track/track-active-cues.html

[Build Bot]

Created attachment 269773 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Darin Adler]

Comment on attachment 269759 [details]
Patch

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

4 media tests still failing

> Source/WebCore/html/HTMLMediaElement.h:150
> +    virtual void postResizeEventIfNecessary(bool) { };

No semicolon needed here. Not clear what the argument means without an argument name, so it should be here, in comment to avoid causing unused argument warning.

    virtual void postResizeEventIfNecessary(bool force);

Also, I am not too fond of the “force” idiom. I suggest we have two separate functions, one called scheduleResizeEvent, the other called scheduleResizeEventIfNecessary.

> Source/WebCore/html/HTMLVideoElement.cpp:58
> +    , lastReportedVideoWidth(0)
> +    , lastReportedVideoHeight(0)

Won’t need these if we initialize in the header or use IntSize.

> Source/WebCore/html/HTMLVideoElement.cpp:198
> +    scheduleEvent(eventNames().resizeEvent);

Ideally we would want to coalesce these so we would only deliver a single resize event if this function is called repeatedly before the resize event is delivered.

> Source/WebCore/html/HTMLVideoElement.h:116
> +    unsigned lastReportedVideoWidth;
> +    unsigned lastReportedVideoHeight;

Data members should have m_ prefix. In new code we should initialize data members in the class definition.

    unsigned m_lastReportedVideoWidth { 0 };
    unsigned m_lastReportedVideoHeight { 0 };

But can’t we use IntSize for this instead?

    IntSize m_lastReportedVideoSize;

[Jeremy Jones]

(In reply to comment #18)
> Comment on attachment 269759 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269759&action=review
> 
> 4 media tests still failing

Updateding test -expected.txt for the new resize event.

> 
> > Source/WebCore/html/HTMLMediaElement.h:150
> > +    virtual void postResizeEventIfNecessary(bool) { };
> 
> No semicolon needed here. Not clear what the argument means without an
> argument name, so it should be here, in comment to avoid causing unused
> argument warning.

Removed semicolon.

> 
>     virtual void postResizeEventIfNecessary(bool force);
> 
> Also, I am not too fond of the “force” idiom. I suggest we have two separate
> functions, one called scheduleResizeEvent, the other called
> scheduleResizeEventIfNecessary.

Removed force and renamed as suggested.

> 
> > Source/WebCore/html/HTMLVideoElement.cpp:58
> > +    , lastReportedVideoWidth(0)
> > +    , lastReportedVideoHeight(0)
> 
> Won’t need these if we initialize in the header or use IntSize.

Initialized in the header.

> 
> > Source/WebCore/html/HTMLVideoElement.cpp:198
> > +    scheduleEvent(eventNames().resizeEvent);
> 
> Ideally we would want to coalesce these so we would only deliver a single
> resize event if this function is called repeatedly before the resize event
> is delivered.

Will coalesce these.

> 
> > Source/WebCore/html/HTMLVideoElement.h:116
> > +    unsigned lastReportedVideoWidth;
> > +    unsigned lastReportedVideoHeight;
> 
> Data members should have m_ prefix. In new code we should initialize data
> members in the class definition.

Oops. Added m_.
> 
>     unsigned m_lastReportedVideoWidth { 0 };
>     unsigned m_lastReportedVideoHeight { 0 };
> 
> But can’t we use IntSize for this instead?
> 
>     IntSize m_lastReportedVideoSize;

No. Dimensions are unsigned.

[Jeremy Jones]

Created attachment 270238 [details]
Patch

[Darin Adler]

Comment on attachment 270238 [details]
Patch

Please upload a new version that applies so we can see EWS results; I’m waiting to review until then.

[Jeremy Jones]

Created attachment 270354 [details]
Patch

[Jeremy Jones]

(In reply to comment #21)
> Comment on attachment 270238 [details]
> Patch
> 
> Please upload a new version that applies so we can see EWS results; I’m
> waiting to review until then.

Uploaded new version that applies. I had to build tot and regenerate a test result to get the patch to apply.

[Build Bot]

Comment on attachment 270354 [details]
Patch

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

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

[Build Bot]

Created attachment 270356 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 270354 [details]
Patch

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

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

[Build Bot]

Created attachment 270357 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 270354 [details]
Patch

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

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

[Build Bot]

Created attachment 270358 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Darin Adler]

Comment on attachment 270354 [details]
Patch

Need to update results for js/dom/dom-static-property-for-in-iteration.html I think.

[Jeremy Jones]

Created attachment 270367 [details]
Patch

[Jeremy Jones]

(In reply to comment #30)
> Comment on attachment 270354 [details]
> Patch
> 
> Need to update results for js/dom/dom-static-property-for-in-iteration.html
> I think.

I re-baselined that one. And fixed a problem with media/video-resize.html.

[Darin Adler]

Comment on attachment 270367 [details]
Patch

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

Looks good to me. And this time I am reviewing before EWS runs so I will say review+ and hope it’s all green!

> Source/WebCore/html/HTMLMediaElement.h:151
> +    virtual void scheduleResizeEvent() { }
> +    virtual void scheduleResizeEventIfNecessary() { }

Since these are only called by code inside HTMLMediaElement itself, they can be private. I think it would be good to have them be private instead of public.

> Source/WebCore/html/HTMLVideoElement.cpp:196
> +void HTMLVideoElement::scheduleResizeEventIfNecessary()

Might consider calling this:

    scheduleResizeEventIfSizeChanged

Seems *slightly* better.

> Source/WebCore/html/HTMLVideoElement.h:45
> +    virtual void scheduleResizeEvent() override;
> +    virtual void scheduleResizeEventIfNecessary() override;

Since these override functions but never themselves need to be called, they can be private. I think it would be good to have them be private instead of public.

[Jeremy Jones]

(In reply to comment #33)
> Comment on attachment 270367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270367&action=review
> 
> Looks good to me. And this time I am reviewing before EWS runs so I will say
> review+ and hope it’s all green!

Green this time!

> 
> > Source/WebCore/html/HTMLMediaElement.h:151
> > +    virtual void scheduleResizeEvent() { }
> > +    virtual void scheduleResizeEventIfNecessary() { }
> 
> Since these are only called by code inside HTMLMediaElement itself, they can
> be private. I think it would be good to have them be private instead of
> public.

Done.

> 
> > Source/WebCore/html/HTMLVideoElement.cpp:196
> > +void HTMLVideoElement::scheduleResizeEventIfNecessary()
> 
> Might consider calling this:
> 
>     scheduleResizeEventIfSizeChanged
> 
> Seems *slightly* better.

I had considered this but was on the fence. Changed.

> 
> > Source/WebCore/html/HTMLVideoElement.h:45
> > +    virtual void scheduleResizeEvent() override;
> > +    virtual void scheduleResizeEventIfNecessary() override;
> 
> Since these override functions but never themselves need to be called, they
> can be private. I think it would be good to have them be private instead of
> public.

Done.

[Jeremy Jones]

Created attachment 270369 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 270369 [details]
Patch for landing.

Clearing flags on attachment: 270369

Committed r195953: <http://trac.webkit.org/changeset/195953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.