Bug 131390 – Support "Live" streams in media controls.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

Support "Live" streams in media controls.

[Jer Noble]

Created attachment 228882 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Eric Carlson]

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?

[Xabier Rodríguez Calvar]

(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.

[Jer Noble]

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.

[Jer Noble]

(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).

[Xabier Rodríguez Calvar]

(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?

[Xabier Rodríguez Calvar]

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.

[Jer Noble]

Created attachment 229154 [details]
Patch

[Brent Fulgham]

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...

[Jer Noble]

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.

[Jer Noble]

Committed r167828: <http://trac.webkit.org/changeset/167828>

[Tim Horton]

Follow up to fix test in http://trac.webkit.org/changeset/167837

[Philippe Normand]

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?