GNOME Bugzilla – Bug 746010
oggdemux: doesn't go into pull mode even when queue2 ring-buffer is enabled
Last modified: 2015-08-16 13:39:42 UTC
https://bugzilla.gnome.org/show_bug.cgi?id=704929 The above change is blocking oggdemux go into pull mode when ring-buffer is enabled. I'm not sure it is regression or not and the above patch is still needed or not. I checked that oggdemux works in pull mode with reverting the patch, and reverse-playback works properly in that case. * bug created as blocker guided by thiblahute
(In reply to Young Han Lee from comment #0) > I checked that oggdemux works in pull mode with reverting the patch, and > reverse-playback works properly in that case. This could be wrong. oggdemux reverse-playback seems to have a problem. reverse-playback starts again after reaching to position 0. This problem could be related to the following bug. I'll investigate it more. https://bugzilla.gnome.org/show_bug.cgi?id=445505
It is a blocker as before that patch reverse playback on HTTP streams works on pipeline like: gst-validate-1.0 --set-scenario reverse_playback playbin uri=http://127.0.0.1:8079/defaults/ogg/vorbis_theora.1.ogg audio-sink='fakesink sync=true name=fakeaudiosink' video-sink='fakesink sync=true name=fakevideosink' ring-buffer-max-size=10485760 That regression was not detected because of https://bugzilla.gnome.org/show_bug.cgi?id=745518
(In reply to Young Han Lee from comment #1) > (In reply to Young Han Lee from comment #0) > > > I checked that oggdemux works in pull mode with reverting the patch, and > > reverse-playback works properly in that case. > > This could be wrong. oggdemux reverse-playback seems to have a problem. > reverse-playback starts again after reaching to position 0. How does it restart?
(In reply to Thibault Saunier from comment #3) > > How does it restart? 0:00:10.551185004 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:00.006177622 0:00:10.553194798 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:00.004167656 0:00:10.555073647 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:00.002283863 0:00:10.556928716 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:00.000428234 0:00:10.566048749 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:08.433333328 0:00:10.587855374 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:08.233333330 0:00:10.615290166 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:07.999999999 0:00:10.639490290 15740 0x9cc000 DEBUG basesink gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: 0:00:07.766666663 Here are test logs. You can see the position value went to 0:00:08.433333328 once the reverse-playback was done. Note that, even in this case, reverse-playback test is passed with your latest patch(http://phabricator.freedesktop.org/differential/diff/52/). Because there are no 3 contiguous wrong positions. But, I think it would be also good to merge your patch first and write another make-up patch to test this if this is a problem.
(In reply to Young Han Lee from comment #4) > (In reply to Thibault Saunier from comment #3) > > > > How does it restart? > > > 0:00:10.551185004 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:00.006177622 > 0:00:10.553194798 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:00.004167656 > 0:00:10.555073647 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:00.002283863 > 0:00:10.556928716 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:00.000428234 > 0:00:10.566048749 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:08.433333328 > 0:00:10.587855374 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:08.233333330 > 0:00:10.615290166 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:07.999999999 > 0:00:10.639490290 15740 0x9cc000 DEBUG basesink > gstbasesink.c:4613:gst_base_sink_get_position:<fakesink1> res: 1, POSITION: > 0:00:07.766666663 > > Here are test logs. You can see the position value went to 0:00:08.433333328 > once the reverse-playback was done. > > Note that, even in this case, reverse-playback test is passed with your > latest patch(http://phabricator.freedesktop.org/differential/diff/52/). > Because there are no 3 contiguous wrong positions. But, I think it would be > also good to merge your patch first and write another make-up patch to test > this if this is a problem. Right, the impression I have about that is that the position reporting is wrong rather than new ((wrong) buffers are pushed, which is also a bug. I did not investigate it in details so I am not sure about that either.
What should happen about this? It's not really a blocker for 1.5/1.6, right? "Only" a regression from 0.10?
(In reply to Sebastian Dröge (slomo) from comment #7) > What should happen about this? It's not really a blocker for 1.5/1.6, right? > "Only" a regression from 0.10? The regression patch was merged b/w 1.1.3 and 1.1.4. I don't know what bug should be a blocker for 1.5/1.6 or not.
Created attachment 300062 [details] [review] fix oggdemux pull mode regression This patch reverts a change which caused oggdemux not to go pull mode even when queue2 ring-buffer is enabled.
Created attachment 300063 [details] [review] Fix oggdemux wrong timestamp bug for reverse playback 0:00:01.751764059 16607 0x7fa844060d40 DEBUG basesink gstbasesink.c:1907:gst_base_sink_get_sync_times:<xvimagesink0> got times start: 0:00:09.466666666, stop: 0:00:09.500000000, do_sync 1 0:00:01.789826604 16607 0x7fa844060d40 DEBUG basesink gstbasesink.c:1907:gst_base_sink_get_sync_times:<xvimagesink0> got times start: 0:00:09.000000000, stop: 0:00:09.033333333, do_sync 1 0:00:02.251512534 16607 0x7fa844060d40 DEBUG basesink gstbasesink.c:1907:gst_base_sink_get_sync_times:<xvimagesink0> got times start: 0:00:00.000000000, stop: 0:00:00.033333333, do_sync 1 Buffer's timestamp was calculated to zero just after seeking with negative rate as you can see the above logs. This causes sink element to wait for stream time to be zero. This was because ogg_packet's granule wasn't calculated properly when rate is negative. The value can be calculated by the same logic of forward playback. This patch fixes it.
Looking at the patch, it seems like a good idea. The only thing I'm not sure about is whether the timestamp should be the "normal" timestamp, or that plus the duration, since playback would reach a frame/sample from the "other side" in backward playback. Not sure what's gstreamer's expectation here.
(In reply to Vincent Penquerc'h from comment #11) > Looking at the patch, it seems like a good idea. The only thing I'm not sure > about is whether the timestamp should be the "normal" timestamp, or that > plus the duration, since playback would reach a frame/sample from the "other > side" in backward playback. Not sure what's gstreamer's expectation here. Thank you for your comment. The timestamp of each GstBuffer is actually PTS which means the time the buffer should be presented to the user. So I think a GstBuffer's PTS should be the same no matter which side it is reached from.
I agree that Sjoerd's patch that makes oggdemux not activate in pull mode if upstream provides the SEQUENTIAL scheduling flag should be reverted, and I plan to revert it soon unless someone convinces me otherwise. Avoiding pull mode activation is a feature regression, and demuxers should always use pull mode where that is possible (e.g. if queue2 allows that). If the goal is to minimise seeks, then that can still be done by making the demuxer behave differently in pull mode if the SEQUENTIAL flag is set.
Review of attachment 300063 [details] [review]: Tim, if no one objects to the revert of Sjoerd's patch, would you revert it and review this patch too?
This was reverted a while back: commit 0a3b584aa0cffa6310de162f0b70f318913d786f Author: Tim-Philipp Müller <tim@centricular.com> Date: Wed May 20 10:22:48 2015 +0100 Revert "oggdemux: Prevent seeks when _SCHEDULING_FLAG_SEQUENTIAL is set" This reverts commit 76647f2710d718e27f207b005956b7dba72c2d19. Avoiding pull mode activation is a feature regression, and demuxers should always use pull mode where that is possible, e.g. if there's an upstream queue2 with a ring buffer or a download buffer. This patch made reverse playback no longer possible over http. If the goal is to minimise seeks, then that can still be done by making the demuxer behave differently in pull mode if the SEQUENTIAL flag is set. If there are bugs, like the demuxer needlessly scanning the entire file on start-up in pull mode, then those should be fixed instead. https://bugzilla.gnome.org/show_bug.cgi?id=746010
What is left to be done here? Refactoring the oggdemux code to run in pull mode if upstream is sequential, but not do random access?
I confirmed that the reverse-playback problem I reported isn't reproduced anymore at master. Although I'm sure which patch fixes it, the following test passes without my suggested patch now. gst-validate-1.0 --set-scenario reverse_playback playbin uri=http://127.0.0.1:8079/defaults/ogg/vorbis_theora.1.ogg audio-sink='fakesink sync=true name=fakeaudiosink' video-sink='fakesink sync=true name=fakevideosink' ring-buffer-max-size=10485760 I think we can close this bug and do the ogg demuxer improvement job in other bug.
Yes, let's close this. I'm not sure ogg reverse playback works entirely correctly, but we should deal with that in a separate bug if there's still an issue.