Bug 114044

Summary: [GStreamer] cannot seek after video finished
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: MediaAssignee: Balazs Kelemen <kbalazs>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, eric.carlson, glenn, gustavo, hausmann, jer.noble, laszlo.gombos, menard, mrobinson, pnormand, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
style fixed none

Description Balazs Kelemen 2013-04-05 10:51:46 PDT
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.)
Comment 1 Balazs Kelemen 2013-04-05 11:23:53 PDT
The same is happening with Qt.
Comment 2 Philippe Normand 2013-04-05 11:52:10 PDT
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.
Comment 3 Balazs Kelemen 2013-04-05 12:05:19 PDT
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.
Comment 4 Yael 2013-04-05 12:14:54 PDT
I have seen this problem when playing certain ogg files, but not others. 
Do you see this happening in e.g. mp4 files?
Comment 5 Balazs Kelemen 2013-04-06 09:04:15 PDT
(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.
Comment 6 Balazs Kelemen 2013-04-17 17:00:26 PDT
Created attachment 198632 [details]
Patch
Comment 7 Philippe Normand 2013-04-17 23:28:52 PDT
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?
Comment 8 Balazs Kelemen 2013-04-18 02:48:24 PDT
(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?
Comment 9 Philippe Normand 2013-04-18 02:58:44 PDT
(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?
Comment 10 Laszlo Gombos 2013-04-22 19:56:46 PDT
(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.
Comment 11 Balazs Kelemen 2013-04-30 10:27:11 PDT
Created attachment 200128 [details]
Patch
Comment 12 WebKit Commit Bot 2013-04-30 10:30:19 PDT
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.
Comment 13 Philippe Normand 2013-05-03 10:31:14 PDT
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.
Comment 14 Balazs Kelemen 2013-05-03 14:45:54 PDT
(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.
Comment 15 Philippe Normand 2013-05-03 15:16:32 PDT
(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.
Comment 16 Balazs Kelemen 2013-05-06 02:36:52 PDT
Created attachment 200649 [details]
Patch
Comment 17 Balazs Kelemen 2013-05-06 02:38:23 PDT
(In reply to comment #16)
> Created an attachment (id=200649) [details]
> Patch

Fixed the left-behind comment, typo in changelog, and the test.
Comment 18 WebKit Commit Bot 2013-05-06 02:39:28 PDT
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.
Comment 19 Balazs Kelemen 2013-05-06 02:50:48 PDT
> 
> 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
Comment 20 Balazs Kelemen 2013-05-07 03:29:35 PDT
Created attachment 200886 [details]
Patch

just typos in comment and changelog
Comment 21 WebKit Commit Bot 2013-05-07 03:32:31 PDT
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.
Comment 22 Balazs Kelemen 2013-05-10 08:18:01 PDT
Created attachment 201346 [details]
style fixed
Comment 23 Philippe Normand 2013-05-13 00:58:01 PDT
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
Comment 24 Balazs Kelemen 2013-05-13 11:37:43 PDT
(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
Comment 25 Chris Dumez 2013-05-13 12:35:51 PDT
Reverted r150022 for reason:

Causes assertions in media tests

Committed r150031: <http://trac.webkit.org/changeset/150031>
Comment 26 Chris Dumez 2013-05-13 12:36:59 PDT
(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
Comment 27 Philippe Normand 2013-05-14 06:24:56 PDT
Relanded in https://trac.webkit.org/r150066 ...