Support "Live" streams in media controls.
Created attachment 228882 [details] Patch
Comment on attachment 228882 [details] Patch Attachment 228882 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5502483224854528 New failing tests: fast/layers/video-layer.html fast/hidpi/video-controls-in-hidpi.html media/media-controls-clone.html
Created attachment 228890 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 228882 [details] Patch Attachment 228882 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6161673060614144 New failing tests: fast/layers/video-layer.html fast/hidpi/video-controls-in-hidpi.html media/media-controls-clone.html
Created attachment 228892 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 228882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228882&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:430 > + > + Nit: these aren't needed. > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:482 > this.controls.panel.appendChild(this.controls.timelineBox); configureInlineControls only adds timelineBox for non live streams, why do it unconditionally here? > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:491 > + this.controls.panel.appendChild(this.controls.statusDisplay); And on the contrary, configureInlineControls always adds statusDisplay. Why only do it for live streams here? > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:512 > + if (this.video.error !== null) > + this.controls.statusDisplay.innerText = this.UIString('Error'); > + else if (this.isLive && this.video.readyState >= HTMLMediaElement.HAVE_CURRENT_DATA) > + this.controls.statusDisplay.innerText = this.UIString('Live Broadcast'); > + else if (this.video.networkState === HTMLMediaElement.NETWORK_LOADING) > + this.controls.statusDisplay.innerText = this.UIString('Loading'); > + else > + this.controls.statusDisplay.innerText = ''; What happened to "stalled", "aborted", etc? > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:514 > + this.setStatusHidden(!this.isLive && this.video.readyState > HTMLMediaElement.HAVE_NOTHING); Don't you also want to show it when there is an error?
(In reply to comment #6) If you allow me to complement your review, I think a test would be to see how the elements are shown or not shown with live and not live streams.
Comment on attachment 228882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228882&action=review >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:482 >> this.controls.panel.appendChild(this.controls.timelineBox); > > configureInlineControls only adds timelineBox for non live streams, why do it unconditionally here? You're right, i'll move it inside. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:491 >> + this.controls.panel.appendChild(this.controls.statusDisplay); > > And on the contrary, configureInlineControls always adds statusDisplay. Why only do it for live streams here? We would previously never show the statusDisplay in full screen mode. Now, we'll only show it for live streams. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:512 >> + this.controls.statusDisplay.innerText = ''; > > What happened to "stalled", "aborted", etc? They're not very useful. We hide the status display (in non-live mode) once we reach HAVE_METADATA, and I don't believe we ever displayed them in our previous (non-js) controls. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:514 >> + this.setStatusHidden(!this.isLive && this.video.readyState > HTMLMediaElement.HAVE_NOTHING); > > Don't you also want to show it when there is an error? We weren't previously, but you're right, we probably should. I'll look into the test errors and upload a new patch.
(In reply to comment #7) > (In reply to comment #6) > > If you allow me to complement your review, I think a test would be to see how the elements are shown or not shown with live and not live streams. I don't know how we'd trigger that test (forcing a stream to have a duration of +infinity).
(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #6) > > > > If you allow me to complement your review, I think a test would be to see how the elements are shown or not shown with live and not live streams. > > I don't know how we'd trigger that test (forcing a stream to have a duration of +infinity). A method in the Internals?
Comment on attachment 228882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228882&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:428 > + this.disconnectControls(); > + > + if (this.controlsType === Controller.InlineControls) > + this.configureInlineControls(); > + else if (this.controlsType == Controller.FullScreenControls) > + this.configureFullScreenControls(); > + > + if (this.shouldHaveControls()) > + this.addControls(); Another thing is that this part of the code is the same as the one in setControlsType. It seems that it could be in a method like this.reconnectControls() or something similar used in both places.
Created attachment 229154 [details] Patch
Comment on attachment 229154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229154&action=review r=me, but I have some concerns about not seeing "Stalled..." and so forth anymore! > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:486 > + this.controls.panel.appendChild(this.controls.statusDisplay); Why don't we want the statusDisplay for fullscreen non-live controls case? It looks like we do include it in the non-fullscreen case (see line 442) > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-488 > - this.controls.statusDisplay.innerText = this.UIString('Aborted'); We don't seem to get this text in 'updateStatusDisplay" > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-493 > - this.controls.statusDisplay.innerText = this.UIString('Suspended'); Ditto... > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-498 > - this.controls.statusDisplay.innerText = this.UIString('Stalled'); Ditto... > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-504 > - this.controls.statusDisplay.innerText = this.UIString('Waiting'); Ditto...
Comment on attachment 229154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229154&action=review >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:486 >> + this.controls.panel.appendChild(this.controls.statusDisplay); > > Why don't we want the statusDisplay for fullscreen non-live controls case? It looks like we do include it in the non-fullscreen case (see line 442) Only because we never have in the past. Currently, in fullscreen mode you can only either display the status or the timeline. They occupy the same space and are absolutely positioned. Displaying both would cause them to overlap in a most unsightly way. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-488 >> - this.controls.statusDisplay.innerText = this.UIString('Aborted'); > > We don't seem to get this text in 'updateStatusDisplay" We don't, true. But it's not a particularly useful status to display. Primarily, this is an event to handle, not a status to display, and it's useful for custom control authors, not end users. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-493 >> - this.controls.statusDisplay.innerText = this.UIString('Suspended'); > > Ditto... Ditto. >> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:-504 >> - this.controls.statusDisplay.innerText = this.UIString('Waiting'); > > Ditto... Ditto.
Committed r167828: <http://trac.webkit.org/changeset/167828>
Follow up to fix test in http://trac.webkit.org/changeset/167837
Comment on attachment 229154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229154&action=review > LayoutTests/ChangeLog:11 > + * http/tests/media/resources/hls/test-vod.m3u8: Added. This file is unused, was it added in the tree on purpose?