Bug 131390 - Support "Live" streams in media controls.
Summary: Support "Live" streams in media controls.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-08 13:52 PDT by Jer Noble
Modified: 2014-09-11 04:28 PDT (History)
13 users (show)

See Also:


Attachments
Patch (9.30 KB, patch)
2014-04-08 13:54 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (626.45 KB, application/zip)
2014-04-08 14:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (656.81 KB, application/zip)
2014-04-08 14:51 PDT, Build Bot
no flags Details
Patch (194.18 KB, patch)
2014-04-11 12:53 PDT, Jer Noble
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2014-04-08 13:52:52 PDT
Support "Live" streams in media controls.
Comment 1 Jer Noble 2014-04-08 13:54:56 PDT
Created attachment 228882 [details]
Patch
Comment 2 Build Bot 2014-04-08 14:28:55 PDT
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
Comment 3 Build Bot 2014-04-08 14:28:58 PDT
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 4 Build Bot 2014-04-08 14:51:39 PDT
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
Comment 5 Build Bot 2014-04-08 14:51:43 PDT
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 6 Eric Carlson 2014-04-08 14:54:43 PDT
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?
Comment 7 Xabier Rodríguez Calvar 2014-04-08 15:09:36 PDT
(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 8 Jer Noble 2014-04-08 16:17:55 PDT
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.
Comment 9 Jer Noble 2014-04-08 16:20:00 PDT
(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).
Comment 10 Xabier Rodríguez Calvar 2014-04-09 01:06:32 PDT
(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 11 Xabier Rodríguez Calvar 2014-04-10 07:10:22 PDT
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.
Comment 12 Jer Noble 2014-04-11 12:53:38 PDT
Created attachment 229154 [details]
Patch
Comment 13 Brent Fulgham 2014-04-16 12:29:38 PDT
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 14 Jer Noble 2014-04-16 17:09:30 PDT
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.
Comment 15 Jer Noble 2014-04-25 15:18:53 PDT
Committed r167828: <http://trac.webkit.org/changeset/167828>
Comment 16 Tim Horton 2014-04-25 17:32:51 PDT
Follow up to fix test in http://trac.webkit.org/changeset/167837
Comment 17 Philippe Normand 2014-09-11 04:28:06 PDT
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?