Summary: | [GStreamer] cannot seek after video finished | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kelemen <kbalazs> | ||||||||||||
Component: | Media | Assignee: | 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
Balazs Kelemen
2013-04-05 10:51:46 PDT
The same is happening with Qt. 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. 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. I have seen this problem when playing certain ogg files, but not others. Do you see this happening in e.g. mp4 files? (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. Created attachment 198632 [details]
Patch
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? (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? (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? (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. Created attachment 200128 [details]
Patch
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 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. (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. (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. Created attachment 200649 [details]
Patch
(In reply to comment #16) > Created an attachment (id=200649) [details] > Patch Fixed the left-behind comment, typo in changelog, and the test. 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.
> > 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 Created attachment 200886 [details]
Patch
just typos in comment and changelog
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.
Created attachment 201346 [details]
style fixed
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 (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 Reverted r150022 for reason: Causes assertions in media tests Committed r150031: <http://trac.webkit.org/changeset/150031> (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 Relanded in https://trac.webkit.org/r150066 ... |