[GStreamer] built-in media player doesn't update

• Component: Media
• Status: RESOLVED FIXED

Go to: http://ftp.osuosl.org/pub/faif-oggcast/FaiF_0x5B_RMS.ogg
Current time in the media player remains at 00:00 seconds and doesn't update as the file is being played.

Tested with Gnome web (epiphany) on Fedora 23.

[Michael Catanzaro]

It seems the current time is updated only when hovering over it with the mouse.

Hopefully this is a recent regression....

[ChangSeok Oh]

O.K Let me check it.

[ChangSeok Oh]

I can't play the ogg on MiniBrowser. :/ Is it possible to play the format in MiniBrowser?

[Michael Catanzaro]

(In reply to comment #3)
> I can't play the ogg on MiniBrowser. :/ Is it possible to play the format in
> MiniBrowser?

It works for me in MiniBrowser; the trick is that this is an audio file, so there's not going to be any video, and you need headphones or speakers.

If it's not working for you, I'd look for missing GStreamer plugins.

[ChangSeok Oh]

Created attachment 269435 [details]
Patch

[Philippe Normand]

This new test doesn't seem to pass on Mac. Is the issue present on Mac as well or GTK only?

[Build Bot]

Comment on attachment 269435 [details]
Patch

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

New failing tests:
media/audio-controls-timeline-in-media-document.html

[Build Bot]

Created attachment 269437 [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 269435 [details]
Patch

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

New failing tests:
media/audio-controls-timeline-in-media-document.html

[Build Bot]

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

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

[ChangSeok Oh]

(In reply to comment #6)
> This new test doesn't seem to pass on Mac. Is the issue present on Mac as
> well or GTK only?

Not sure about the Mac. Let me check why it is unhappy with this change later. I have no mac. :/

[Build Bot]

Comment on attachment 269435 [details]
Patch

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

New failing tests:
media/audio-controls-timeline-in-media-document.html

[Build Bot]

Created attachment 269439 [details]
Archive of layout-test-results from ews113 for mac-yosemite

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

[Xabier Rodríguez Calvar]

Comment on attachment 269435 [details]
Patch

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

> LayoutTests/media/audio-controls-timeline-in-media-document.html:29
> +        if (video.currentTime >= 1) {
> +            consoleWrite("EVENT(timeupdate)");
> +            testAndEnd("timeLineValue() >= 1");
> +        }

Are you getting 0 as video.currentTime at any moment. I don't know if we are signalling already a timeupdate for 0 when playback begins, but if we are not, we might get rid of this.

[Xabier Rodríguez Calvar]

(In reply to comment #14)
> Comment on attachment 269435 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269435&action=review
> 
> > LayoutTests/media/audio-controls-timeline-in-media-document.html:29
> > +        if (video.currentTime >= 1) {
> > +            consoleWrite("EVENT(timeupdate)");
> > +            testAndEnd("timeLineValue() >= 1");
> > +        }
> 
> Are you getting 0 as video.currentTime at any moment. I don't know if we are
> signalling already a timeupdate for 0 when playback begins, but if we are
> not, we might get rid of this.

The first sentence was supposed to be a question and I meant getting rid of the if clause.

[Xabier Rodríguez Calvar]

Comment on attachment 269435 [details]
Patch

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

The existence of Base and GTK cames from the time we used Apple controls as base. They decided to fork their controls and leave us with Base, while they use Apple alone so that they could change them without breaking our implementation. This had some advantages and some drawbacks.

Nowadays the separation between Base and GTK is not needed at all. EFL still uses Base alone, which in the end is a very outdated version (and probably buggy) of the Apple controls. If we decide to merge GTK into base, we'd need to switch EFL to use GTK controls, but I think they would be winning.

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1033
> +        if (panel.classList.contains(this.ClassNames.noVideo))
> +            return false;
> +        if (panel.parentElement.querySelector(':hover') === panel)
> +            return false;

controlsAreHidden is obviously a method to know if the controls are hidden or not. I don't like having the :hover clause here as soon as mouse enters the video area, controls should get visible by having the show and/or the hidden values changed. If that does not happen, I think we have a problem. The :hover clause was introduced by Apple when we were managing the controls together but now it is gone in their case too. Let me know if something breaks. If something does, then let's put it again, but something tells me it is not needed.

About checking the noVideo class, I think it is also a hack but we need it for now. I leave up to you to encapsulate it (and the :hover clause if we can't remove it) in a method called controlsAlwaysVisible as Apple does, which I think, it is a good idea at this moment.

[ChangSeok Oh]

(In reply to comment #15)
> (In reply to comment #14)
> > Comment on attachment 269435 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=269435&action=review
> > 
> > > LayoutTests/media/audio-controls-timeline-in-media-document.html:29
> > > +        if (video.currentTime >= 1) {
> > > +            consoleWrite("EVENT(timeupdate)");
> > > +            testAndEnd("timeLineValue() >= 1");
> > > +        }
> > 
> > Are you getting 0 as video.currentTime at any moment. I don't know if we are
> > signalling already a timeupdate for 0 when playback begins, but if we are
> > not, we might get rid of this.
> 
> The first sentence was supposed to be a question and I meant getting rid of
> the if clause.
Nope, we don't get 0 as video.currentTime so we can remove the if clause. Thanks.

> Nowadays the separation between Base and GTK is not needed at all. EFL still uses Base alone, which in the end is a very outdated version (and probably buggy) of the Apple controls. If we decide to merge GTK into base, we'd need to switch EFL to use GTK controls, but I think they would be winning.
Hrm, I am not sure if I got your point here. We use both mediaControlsBase.js and mediaControlsGtk.js. The Base is a kind of parent class implementation of the Gtk. I am not merging Gtk only fix to Base. The fix of the Base would affect on Efl port as well.

> controlsAreHidden is obviously a method to know if the controls are hidden or not. I don't like having the :hover clause here as soon as mouse enters the video area, controls should get visible by having the show and/or the hidden values changed. If that does not happen, I think we have a problem. The :hover clause was introduced by Apple when we were managing the controls together but now it is gone in their case too.
No, that's not true. the :hover clause was added by my previous fix. I had a similar thought to yours so I checked panel's opacity rather than having :hover in the first patch but you preferred the way of checking :hover. =) Please see https://bugs.webkit.org/show_bug.cgi?id=149111#c5
In anycase we would see bug149111 without it.

[ChangSeok Oh]

Created attachment 270119 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to comment #17)
> > Nowadays the separation between Base and GTK is not needed at all. EFL still uses Base alone, which in the end is a very outdated version (and probably buggy) of the Apple controls. If we decide to merge GTK into base, we'd need to switch EFL to use GTK controls, but I think they would be winning.
> Hrm, I am not sure if I got your point here. We use both
> mediaControlsBase.js and mediaControlsGtk.js. The Base is a kind of parent
> class implementation of the Gtk. I am not merging Gtk only fix to Base. The
> fix of the Base would affect on Efl port as well.

I know about Base, GTK and EFL. I'll explain it again because I think I didn't manage to make my point.

At the beginning there was no Base, there was Apple and GTK+ inherited from Apple. EFL took Apple controls later.

Then Apple decided to "fork" their controls, so it copied Apple into Base and changed GTK to use Base+GTK+ and EFL to use Base, and Apple began to diverge.

Considering that EFL does not update their controls and they are using Base directly (meaning an very old version of the Apple ones), I think we could reduce our maintainance burden by merging Base and GTK into Base. That way EFL and GTK would have the same controls, which I don't think is a bad idea. Code complexity would be reduce a lot.

> > controlsAreHidden is obviously a method to know if the controls are hidden or not. I don't like having the :hover clause here as soon as mouse enters the video area, controls should get visible by having the show and/or the hidden values changed. If that does not happen, I think we have a problem. The :hover clause was introduced by Apple when we were managing the controls together but now it is gone in their case too.

> No, that's not true. the :hover clause was added by my previous fix. I had a
> similar thought to yours so I checked panel's opacity rather than having
> :hover in the first patch but you preferred the way of checking :hover. =)
> Please see https://bugs.webkit.org/show_bug.cgi?id=149111#c5
> In anycase we would see bug149111 without it.

Panel opacity corresponds to look and feel and :hover corresponds to user action. If you make me choose between opacity and :hover, of course I choose :hover :)

About who added that, I must have misanalyzed the code at the moment I wrote this cause you're right.

Anyway, what I said about this is still true. We should not have the :hover clause, we should check for the :hover event coming in and going out and update the "show/hidden" classes. I could agree that it could be out of the scope of this patch and fix this later, but I still don't like that.

[ChangSeok Oh]

(In reply to comment #19)
> Considering that EFL does not update their controls and they are using Base
> directly (meaning an very old version of the Apple ones), I think we could
> reduce our maintainance burden by merging Base and GTK into Base. That way
> EFL and GTK would have the same controls, which I don't think is a bad idea.
> Code complexity would be reduce a lot.
O.K I see. So you want to make gtk and efl ports share same code base with mediaControlsBase.js. It sounds make sense to me, too. But it is a little bit off topic from this bug itself. Let's handle it in a separate bug. I can do it. https://bugs.webkit.org/show_bug.cgi?id=153601

> Anyway, what I said about this is still true. We should not have the :hover
> clause, we should check for the :hover event coming in and going out and
> update the "show/hidden" classes. I could agree that it could be out of the
> scope of this patch and fix this later, but I still don't like that.
Let's handle it later in a different ticket. This also seems not directly related with this bug to me. https://bugs.webkit.org/show_bug.cgi?id=153603

[Xabier Rodríguez Calvar]

Comment on attachment 270119 [details]
Patch

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

I didn't review many things at the first glance, I became who I hated, sorry for that :)

I think one more iteration and we could be clear.

> Source/WebCore/ChangeLog:11
> +        i,e it is not actually hidden. We can fix this by simply returing false for no-video media

i,e -> i.e. returing -> returning

> Source/WebCore/ChangeLog:17
> +        (Controller.prototype.controlsAreHidden):

Either remove this or add controlsAlwaysVisible (obviously, the alternative function name that I'll comment below).

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1027
> +    controlsAlwaysVisible: function()

Let's put a verb in the function name: "controlsAreAlwaysVisible" or "controlsShouldBeAlwaysVisible".

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1030
> +        var panel = this.controls.panel;
> +        return panel.classList.contains(this.ClassNames.noVideo) || (panel.parentElement.querySelector(':hover') === panel);

I changed my mind (sorry) about having the :hover here. If the intention of the function name is to know if the controls are (or should be) always visible, it does not make sense to have the :hover here as :hover is a "non-permanent" situation. Let's move it back to the other place.

Then, since you are using panel only once, consider removing the variable (that should have been a const instead of var, btw).

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1035
> +        if (this.controlsAlwaysVisible())

Variable name discussed above.

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1039
> -        return (!panel.classList.contains(this.ClassNames.show) || panel.classList.contains(this.ClassNames.hidden))
> -            && (panel.parentElement.querySelector(':hover') !== panel);
> +        return !panel.classList.contains(this.ClassNames.show) || panel.classList.contains(this.ClassNames.hidden);

Read the :hover, as discussed above.

I'll r+ if you move the :hover, but I would be must happier if you add a watch for the :hover and mangle the show/hidden class name. Actually I don't understand why the mouseenter and mouseleave don't detect these cases.

> LayoutTests/media/audio-controls-timeline-in-media-document.html:5
> +<script src=media-file.js></script>
> +<script src=video-test.js></script>
> +<script src=media-controls.js></script>

Add " to the filenames.

> LayoutTests/media/audio-controls-timeline-in-media-document.html:9
> +    var timeline = mediaControlsElement(internals.shadowRoot(video).firstChild.firstChild, '-webkit-media-controls-timeline');

var -> const. Consistency within the code, switch ' to ".

> LayoutTests/media/audio-controls-timeline-in-media-document.html:18
> +    video = iframe.contentDocument.querySelector('video');

Consistency within the code, switch ' to ".

> LayoutTests/media/audio-controls-timeline-in-media-document.html:26
> +var video;
> +var iframe = document.createElement('iframe');

var -> const. Consistency within the code, switch ' to ".

[ChangSeok Oh]

Comment on attachment 270119 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        i,e it is not actually hidden. We can fix this by simply returing false for no-video media
> 
> i,e -> i.e. returing -> returning

Oops.

>> Source/WebCore/ChangeLog:17
>> +        (Controller.prototype.controlsAreHidden):
> 
> Either remove this or add controlsAlwaysVisible (obviously, the alternative function name that I'll comment below).

controlsAreAlwaysVisible is added.

>> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1027
>> +    controlsAlwaysVisible: function()
> 
> Let's put a verb in the function name: "controlsAreAlwaysVisible" or "controlsShouldBeAlwaysVisible".

controlsAreAlwaysVisible it is!

>> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1030
>> +        return panel.classList.contains(this.ClassNames.noVideo) || (panel.parentElement.querySelector(':hover') === panel);
> 
> I changed my mind (sorry) about having the :hover here. If the intention of the function name is to know if the controls are (or should be) always visible, it does not make sense to have the :hover here as :hover is a "non-permanent" situation. Let's move it back to the other place.
> 
> Then, since you are using panel only once, consider removing the variable (that should have been a const instead of var, btw).

O.K.

>> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1035
>> +        if (this.controlsAlwaysVisible())
> 
> Variable name discussed above.

Done.

>> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:1039
>> +        return !panel.classList.contains(this.ClassNames.show) || panel.classList.contains(this.ClassNames.hidden);
> 
> Read the :hover, as discussed above.
> 
> I'll r+ if you move the :hover, but I would be must happier if you add a watch for the :hover and mangle the show/hidden class name. Actually I don't understand why the mouseenter and mouseleave don't detect these cases.

I opened https://bugs.webkit.org/show_bug.cgi?id=153603. Let me investigate it there.

>> LayoutTests/media/audio-controls-timeline-in-media-document.html:5
>> +<script src=media-controls.js></script>
> 
> Add " to the filenames.

Done.

>> LayoutTests/media/audio-controls-timeline-in-media-document.html:9
>> +    var timeline = mediaControlsElement(internals.shadowRoot(video).firstChild.firstChild, '-webkit-media-controls-timeline');
> 
> var -> const. Consistency within the code, switch ' to ".

Yup.

>> LayoutTests/media/audio-controls-timeline-in-media-document.html:18
>> +    video = iframe.contentDocument.querySelector('video');
> 
> Consistency within the code, switch ' to ".

ditto.

>> LayoutTests/media/audio-controls-timeline-in-media-document.html:26
>> +var iframe = document.createElement('iframe');
> 
> var -> const. Consistency within the code, switch ' to ".

Done.

[ChangSeok Oh]

Created attachment 270190 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 270190 [details]
Patch

Awesome, thanks!

[ChangSeok Oh]

Comment on attachment 270190 [details]
Patch

My pleasure ;)

[WebKit Commit Bot]

Comment on attachment 270190 [details]
Patch

Clearing flags on attachment: 270190

Committed r195809: <http://trac.webkit.org/changeset/195809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

(In reply to comment #19)
> (In reply to comment #17)
> > > Nowadays the separation between Base and GTK is not needed at all. EFL still uses Base alone, which in the end is a very outdated version (and probably buggy) of the Apple controls. If we decide to merge GTK into base, we'd need to switch EFL to use GTK controls, but I think they would be winning.
> > Hrm, I am not sure if I got your point here. We use both
> > mediaControlsBase.js and mediaControlsGtk.js. The Base is a kind of parent
> > class implementation of the Gtk. I am not merging Gtk only fix to Base. The
> > fix of the Base would affect on Efl port as well.
> 
> I know about Base, GTK and EFL. I'll explain it again because I think I
> didn't manage to make my point.
> 
> At the beginning there was no Base, there was Apple and GTK+ inherited from
> Apple. EFL took Apple controls later.
> 
> Then Apple decided to "fork" their controls, so it copied Apple into Base
> and changed GTK to use Base+GTK+ and EFL to use Base, and Apple began to
> diverge.
> 
> Considering that EFL does not update their controls and they are using Base
> directly (meaning an very old version of the Apple ones), I think we could
> reduce our maintainance burden by merging Base and GTK into Base. That way
> EFL and GTK would have the same controls, which I don't think is a bad idea.
> Code complexity would be reduce a lot.

Basically I think the way is that we go. EFL had owned media control theme. But it was very poor quality controls. That's why we have used Apple's base media control themes. If EFL is continue to use base theme without GTK dependency when merging base + GTK+, I don't have any reason to object this plan.