Summary: | Add resize event for HTMLMediaElement | ||
---|---|---|---|
Product: | WebKit | Reporter: | Brendan Long <b.long> |
Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | buildbot, commit-queue, eric.carlson, hiremathprashants, jeremyj-wk, jer.noble, jonlee, rniwa, simon.fraser, syoichi, webkit-bug-importer |
Priority: | P2 | Keywords: | InRadar, WebExposed |
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=199617 | ||
Attachments: |
Description
Brendan Long
2013-12-13 15:40:18 PST
Can be merged from blink: https://codereview.chromium.org/103913010/ *** Bug 125773 has been marked as a duplicate of this bug. *** 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. I just found something that should make this easier to test: We're supposed to fire a resize event when the video initially loads. Created attachment 269381 [details]
Patch
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. (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. Created attachment 269759 [details]
Patch
(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. 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" 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 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
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 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
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 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
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; (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. Created attachment 270238 [details]
Patch
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.
Created attachment 270354 [details]
Patch
(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. 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 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
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 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
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 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
Comment on attachment 270354 [details]
Patch
Need to update results for js/dom/dom-static-property-for-in-iteration.html I think.
Created attachment 270367 [details]
Patch
(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. 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. (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. Created attachment 270369 [details]
Patch for landing.
Comment on attachment 270369 [details] Patch for landing. Clearing flags on attachment: 270369 Committed r195953: <http://trac.webkit.org/changeset/195953> All reviewed patches have been landed. Closing bug. |