Bug 114044 – [GStreamer] cannot seek after video finished

• Component: Media
• Status: RESOLVED FIXED

[Balazs Kelemen]

I'm seeing this in EFL's MiniBrowser. After the video finished further seeking is ignored, even after restarting it. I will check whether it is EFL only (I guess it is not). Furthermore, when the video is finishing the last frame is not showed as it should be but either background or just a part of the last frame is showed. Probably we go into a bad state when the video finishes. (Btw, I'm totally new in GStreamer and web media so I'm just guessing.)

[Balazs Kelemen]

The same is happening with Qt.

[Philippe Normand]

Part of last frame? Mind attaching a screenshot?
When EOS is reached the pipeline goes to GST_STATE_NULL to release all resources and video/audio devices.

I guess this is a bug anyway, seeking should probably still be allowed.

[Balazs Kelemen]

I could not reproduce it now, it just shows controls and background. I'm new to the topic but it seems to me we should only release resources when the page with the video is shutting down. Doing this would also avoid reinitializing if the user wants to replay.

[Yael]

I have seen this problem when playing certain ogg files, but not others. 
Do you see this happening in e.g. mp4 files?

[Balazs Kelemen]

(In reply to comment #4)
> I have seen this problem when playing certain ogg files, but not others. 
> Do you see this happening in e.g. mp4 files?

Yep, it's the same with different files and with mp4 files as well.

[Balazs Kelemen]

Created attachment 198632 [details]
Patch

[Philippe Normand]

Comment on attachment 198632 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Instead of shutting down the pipeline just pause it when we are
> +        at the end of the stream.

Please, no. The pipeline needs to be shut down at EOS to release resources. Can't we preroll the pipeline again if a seek is requested after EOS occured?

[Balazs Kelemen]

(In reply to comment #7)
> (From update of attachment 198632 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198632&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        Instead of shutting down the pipeline just pause it when we are
> > +        at the end of the stream.
> 
> Please, no. The pipeline needs to be shut down at EOS to release resources. Can't we preroll the pipeline again if a seek is requested after EOS occured?

Makes sense, but I'm not sure we should optimize for the most trivial use case when the user just play through the media once. Can GStreamer reuse the decoded on-disk buffered data again, if we do this? If not, it is expensive to replay the video, no? Especially on fullscreen mode (mobile), we should find a way to not release the resources until the user exit from fullscreen (which is way beyond this patch, I'm just wondering what would be the good direction). Opinions?

[Philippe Normand]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 198632 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=198632&action=review
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        Instead of shutting down the pipeline just pause it when we are
> > > +        at the end of the stream.
> > 
> > Please, no. The pipeline needs to be shut down at EOS to release resources. Can't we preroll the pipeline again if a seek is requested after EOS occured?
> 
> Makes sense, but I'm not sure we should optimize for the most trivial use case when the user just play through the media once. 

What do you mean?

> Can GStreamer reuse the decoded on-disk buffered data again, if we do this?

Not by default. The on-disk buffer is temporary, stored in /tmp, and that's only for video over HTTP with preload on.

> If not, it is expensive to replay the video, no?

Shouldn't be. Switch the pipeline to PAUSED. Probably even in READY it would work, not sure.

> Especially on fullscreen mode (mobile), we should find a way to not release the resources until the user exit from fullscreen (which is way beyond this patch, I'm just wondering what would be the good direction). Opinions?

What does fullscreen mode has to do with this bug?

[Laszlo Gombos]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 198632 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=198632&action=review
> > > 
> > > > Source/WebCore/ChangeLog:11
> > > > +        Instead of shutting down the pipeline just pause it when we are
> > > > +        at the end of the stream.
> > > 

> > Especially on fullscreen mode (mobile), we should find a way to not release the resources until the user exit from fullscreen (which is way beyond this patch, I'm just wondering what would be the good direction). Opinions?
> 
> What does fullscreen mode has to do with this bug?

Essentially the engine should try to predict the next action and act based on that prediction. Releasing the pipeline is likely the best possible action if the prediction is that the pipeline will not be needed any more.  However if it is a reasonable to predict that the pipeline will be needed (e.g. full screen video with a big replay button) than it might make sense to delay the destruction of the pipeline until it is reasonable again to predict that the pipeline is not needed (e.g. exiting full screen video playback).

I realize that this might not be in the scope for this patch, but I can see some merit in the idea.

[Balazs Kelemen]

Created attachment 200128 [details]
Patch

[WebKit Commit Bot]

Attachment 200128 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-seek-after-end-expected.txt', u'LayoutTests/media/video-seek-after-end.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:523:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1247:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 200128 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        Seeks are not the only operations executed asynchronously, changing the pipeling state is

But ASYNC_DONE is not about seeks at all, AFAIK. It's only about state changes.

> Source/WebCore/ChangeLog:24
> +        seek has been compleeted.

typo: compleeted

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:144
> +    // Extract the integer part of the time (seconds) and the fractional part (microseconds). Attempt to
> +    // round the microseconds so no floating point precision is lost and we can perform an accurate seek.

FYI I'm reworking this in bug 90734

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:788
> +        if (messageSourceIsPlaybin)
> +            durationChanged();

Why this change? AFAIK duration messages can be sent by source elements too, for example.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1073
> +            // XXX comment

Left over?

> LayoutTests/media/video-seek-after-end.html:19
> +                        testExpected('video.currentTime.toFixed(2)', (video.duration / 2).toFixed(2), '==');

This might need relaxing. The new position depends on which media file was selected, which may not be the same across all ports.

[Balazs Kelemen]

(In reply to comment #13)
> (From update of attachment 200128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200128&action=review
> 
> > Source/WebCore/ChangeLog:18
> > +        Seeks are not the only operations executed asynchronously, changing the pipeling state is
> 
> But ASYNC_DONE is not about seeks at all, AFAIK. It's only about state changes.

Seeks are triggering state changes and notification about them is done via GST_MESSAGE_STATE_CHANGE, and in the case of flushing seeks (we only do this kind) also via GST_MESSAGE_ASYNC_DONE. The fact that they make a GST_MESSAGE_STATE_CHANGE emitted is the reason why the current code is working more or less.
I think it's better to think about a state as a pair of the current and the pending states. Keeping that in mind even a seek in paused state triggers an exact state transition, i.e. this:

(paused, none) -- seek --> (paused, paused) -- seek done --> (paused, none)

In order to correctly keep track of when does a seek finish I think it's necessary to handle ASYNC_DONE (well, I guess this is the reason why it exists).

Note the documentation of gst_element_seek:
"Returns : TRUE if the event was handled. Flushing seeks will trigger a preroll, which will emit GST_MESSAGE_ASYNC_DONE"

> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:788
> > +        if (messageSourceIsPlaybin)
> > +            durationChanged();
> 
> Why this change? AFAIK duration messages can be sent by source elements too, for example.

Yes, but I am assuming it is forwarded to the playbin2 as well as GST_MESSAGE_STATE_CHANGE, so it seems unnecessary to handle if the source is not the playbin.

> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1073
> > +            // XXX comment
> 
> Left over?

Sure it is, but that part really deserves a comment :)

> 
> > LayoutTests/media/video-seek-after-end.html:19
> > +                        testExpected('video.currentTime.toFixed(2)', (video.duration / 2).toFixed(2), '==');
> 
> This might need relaxing. The new position depends on which media file was selected, which may not be the same across all ports.

That's right, it could make the test output platform dependent. I will just use a hard coded seek target than.

[Philippe Normand]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 200128 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=200128&action=review
> > 
> > > Source/WebCore/ChangeLog:18
> > > +        Seeks are not the only operations executed asynchronously, changing the pipeling state is
> > 
> > But ASYNC_DONE is not about seeks at all, AFAIK. It's only about state changes.
> 
> Seeks are triggering state changes and notification about them is done via GST_MESSAGE_STATE_CHANGE, and in the case of flushing seeks (we only do this kind) also via GST_MESSAGE_ASYNC_DONE. The fact that they make a GST_MESSAGE_STATE_CHANGE emitted is the reason why the current code is working more or less.
> I think it's better to think about a state as a pair of the current and the pending states. Keeping that in mind even a seek in paused state triggers an exact state transition, i.e. this:
> 
> (paused, none) -- seek --> (paused, paused) -- seek done --> (paused, none)
> 
> In order to correctly keep track of when does a seek finish I think it's necessary to handle ASYNC_DONE (well, I guess this is the reason why it exists).
> 
> Note the documentation of gst_element_seek:
> "Returns : TRUE if the event was handled. Flushing seeks will trigger a preroll, which will emit GST_MESSAGE_ASYNC_DONE"
> 

Oh I didn't remember about that. The documentation of the message type itself only mentions state changes.

> > 
> > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:788
> > > +        if (messageSourceIsPlaybin)
> > > +            durationChanged();
> > 
> > Why this change? AFAIK duration messages can be sent by source elements too, for example.
> 
> Yes, but I am assuming it is forwarded to the playbin2 as well as GST_MESSAGE_STATE_CHANGE, so it seems unnecessary to handle if the source is not the playbin.
> 

Ok that makes sense.

[Balazs Kelemen]

Created attachment 200649 [details]
Patch

[Balazs Kelemen]

(In reply to comment #16)
> Created an attachment (id=200649) [details]
> Patch

Fixed the left-behind comment, typo in changelog, and the test.

[WebKit Commit Bot]

Attachment 200649 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-seek-after-end-expected.txt', u'LayoutTests/media/video-seek-after-end.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:523:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

The style issues are intentional, I filed https://bugs.webkit.org/show_bug.cgi?id=115629

[Balazs Kelemen]

Created attachment 200886 [details]
Patch

just typos in comment and changelog

[WebKit Commit Bot]

Attachment 200886 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-seek-after-end-expected.txt', u'LayoutTests/media/video-seek-after-end.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:523:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

Created attachment 201346 [details]
style fixed

[Philippe Normand]

Comment on attachment 201346 [details]
style fixed

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

Looks good, only a little nit to fix before landing. Thanks!

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:513
> +    if (getStateResult == GST_STATE_CHANGE_ASYNC || (state != GST_STATE_PAUSED && state != GST_STATE_PLAYING) || m_isEndReached) {

the state test can be simplified to state < GST_STATE_PAUSED I think

[Balazs Kelemen]

(In reply to comment #23)
> (From update of attachment 201346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201346&action=review
> 
> Looks good, only a little nit to fix before landing. Thanks!
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:513
> > +    if (getStateResult == GST_STATE_CHANGE_ASYNC || (state != GST_STATE_PAUSED && state != GST_STATE_PLAYING) || m_isEndReached) {
> 
> the state test can be simplified to state < GST_STATE_PAUSED I think

Fixed, thanks!

Landed in http://trac.webkit.org/changeset/150022

[Chris Dumez]

Reverted r150022 for reason:

Causes assertions in media tests

Committed r150031: <http://trac.webkit.org/changeset/150031>

[Chris Dumez]

(In reply to comment #25)
> Reverted r150022 for reason:
> 
> Causes assertions in media tests
> 
> Committed r150031: <http://trac.webkit.org/changeset/150031>

See:
http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r150022%20(12199)/media/video-played-collapse-crash-log.txt

[Philippe Normand]

Relanded in https://trac.webkit.org/r150066 ...