Bug 123791 – [GStreamer] Playback rate is omitted when seeking

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Piotr Grad]

After setting playback rate to negative value: when seeking we should calculate start & end time. When playback rate is negative start time should be time passed to 'seek' method, and end time should be '0'.

[Piotr Grad]

Created attachment 216012 [details]
Patch and Layout test

[Piotr Grad]

Comment on attachment 216012 [details]
Patch and Layout test

inproper url to bugizlla entry

[Piotr Grad]

Created attachment 216013 [details]
Patch & layout test

[Philippe Normand]

Comment on attachment 216013 [details]
Patch & layout test

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

Thanks for the patch, it's a regression I think... this used to work at some point :) r- only because of the test issue and missing results

> LayoutTests/media/video-seek-with-negative-playback.html:1
> +<video src = 'content/test.ogv'></video>

Please use findMediaFile()

[Piotr Grad]

Created attachment 216019 [details]
Patch & layout test

[Philippe Normand]

Comment on attachment 216019 [details]
Patch & layout test

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

Looks good but I have some nits about the test. Can you please fix them?

> LayoutTests/media/video-seek-with-negative-playback.html:1
> +<video></video>

<video/>
It'd be good to make this a normal html file with the usual head and body too.

> LayoutTests/media/video-seek-with-negative-playback.html:8
> +        function ()

The coding style is not very consistent about curly braces. Can the test be refactored to avoid the nested waitForEvent()s?

> LayoutTests/media/video-seek-with-negative-playback.html:16
> +                    if ( lastTime == -1) {

empty space

[Piotr Grad]

Created attachment 216035 [details]
Patch & layout test

[Piotr Grad]

(In reply to comment #6)
> (From update of attachment 216019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216019&action=review
> 
> Looks good but I have some nits about the test. Can you please fix them?
> 
> > LayoutTests/media/video-seek-with-negative-playback.html:1
> > +<video></video>
> 
> <video/>
> It'd be good to make this a normal html file with the usual head and body too.

The problem is that when i use html, head, and body tags test would look much more complicated becouse of needed getElementById calls, etc.

> 
> > LayoutTests/media/video-seek-with-negative-playback.html:8
> > +        function ()
> 
> The coding style is not very consistent about curly braces. Can the test be refactored to avoid the nested waitForEvent()s?

The problem is that we have to wait for seeked event to listen for timeupdate event. Also we can't seek if player is not ready to play.

> 
> > LayoutTests/media/video-seek-with-negative-playback.html:16
> > +                    if ( lastTime == -1) {
> 
> empty space

[Philippe Normand]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 216019 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=216019&action=review
> > 
> > Looks good but I have some nits about the test. Can you please fix them?
> > 
> > > LayoutTests/media/video-seek-with-negative-playback.html:1
> > > +<video></video>
> > 
> > <video/>
> > It'd be good to make this a normal html file with the usual head and body too.
> 
> The problem is that when i use html, head, and body tags test would look much more complicated becouse of needed getElementById calls, etc.
> 

Come on, it's not that complicated. There's also a findMediaElement() function...

> > 
> > > LayoutTests/media/video-seek-with-negative-playback.html:8
> > > +        function ()
> > 
> > The coding style is not very consistent about curly braces. Can the test be refactored to avoid the nested waitForEvent()s?
> 
> The problem is that we have to wait for seeked event to listen for timeupdate event. Also we can't seek if player is not ready to play.
> 

Fine.

[Piotr Grad]

Created attachment 216145 [details]
Patch & layout test

[WebKit Commit Bot]

Comment on attachment 216145 [details]
Patch & layout test

Clearing flags on attachment: 216145

Committed r158734: <http://trac.webkit.org/changeset/158734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

This new test immediately failed on all Apple bots. Additionally, the test was committed with CRLF line-endings. Cleaned up the test and results in https://trac.webkit.org/r158752.

[Philippe Normand]

(In reply to comment #13)
> (In reply to comment #12)
> > All reviewed patches have been landed.  Closing bug.
> 
> This new test immediately failed on all Apple bots. Additionally, the test was committed with CRLF line-endings. Cleaned up the test and results in https://trac.webkit.org/r158752.

Oh, I thought the mac EWS was running layout tests nowadays?
Sorry about the trouble.