GNOME Bugzilla – Bug 750783
baseparse: reverse playback in pull mode
Last modified: 2015-08-16 13:39:19 UTC
Reverse playback is disabled in pull mode. Adding logic to handle negative playback in pull mode. Tested the same by playing mp3 files in playback_test
Created attachment 305062 [details] [review] reverse playback in pull mode Please review
*** Bug 681631 has been marked as a duplicate of this bug. ***
Created attachment 305113 [details] [review] reverse playback in pull mode Updated the patch as per comments in IRC. now in case of reverse playback, start position is not being set to 0. This helps in proper reverse playback, when the seek range is limited.
Ping :)
Created attachment 305638 [details] [review] tests for pull mode This patch helps in testing the pull mode for baseparse. It checks in both forward play as well as reverse playback in pull mode. In case of pull mode, reverse playback is disabled, hence it will fail now. With the other patch attached in bug, reverse playback can be enabled and tested. Please review :)
Review of attachment 305638 [details] [review]: I only have small comment, otherwise looks good. ::: tests/check/libs/baseparse.c @@ +23,3 @@ #include "config.h" #endif + Oops, you added a white space here. @@ +370,3 @@ + case GST_EVENT_EOS: + if (loop) { + while (!g_main_loop_is_running (loop)); Some compilers will warn because of the empty statement. What I usually do is add bracket with a comment /* nothing */ This is worth a comment like /* Wait for main loop to start */
Review of attachment 305113 [details] [review]: This only need a final cleanup (spurious white spaces). Can you check that you haven't accidentally disabled gst-indent ? It should catch these at commit time. ::: libs/gst/base/gstbaseparse.c @@ +4337,3 @@ else startpos -= parse->priv->lead_in_ts; + White space. @@ +4358,3 @@ GST_FORMAT_BYTES, &seekstop)) goto convert_failed; + White space.
Created attachment 305730 [details] [review] reverse playback in pull mode
Created attachment 305731 [details] [review] tests for pull mode I do hv gst-indent installed, but it doesnt check for extra lines i guess.
Created attachment 305732 [details] [review] reverse playback in pull mode Sometimes i added extra line for more readability.. But it still complains for extra lines.. so removing them :)
Extra lines is fine, maybe it's Bugzilla review thingy which is buggy, From there, it seemed like if that lines had a white space in it. Anyway, it this wasn't the case, sorry for the noise.
:) it's ok.. anything else to be done for this?
I'm retesting now, and if happy, I'll merge !
I'm sorry, this slip out of my mind.
Created attachment 306884 [details] [review] reverse playback in pull mode rebasing with the latest code.. Please review :)
Thanks: commit 274f4b784a1cdea411eb65639e37c96238b1edac Author: Vineeth TM <vineeth.tm@samsung.com> Date: Mon Jul 6 09:26:58 2015 +0900 baseparse: reverse playback in pull mode right now reverse playback is disabled in pull mode. enabling the code for the same and changing a bit of logic to make reverse playback work. https://bugzilla.gnome.org/show_bug.cgi?id=750783 commit 9d57165c331cc6fd674f0d3b8e49349160e79974 Author: Vineeth T M <vineeth.tm@samsung.com> Date: Sat Jun 20 08:33:26 2015 +0900 baseparse: add reverse playback test in pull mode add test for reverse playback in pull mode and compare the buffers being received in sink chain to make sure the playback is allright https://bugzilla.gnome.org/show_bug.cgi?id=750783
Now seeking is sometime off (both when seeking over local file and remote http file).
Review of attachment 306884 [details] [review]: ::: libs/gst/base/gstbaseparse.c @@ +4335,3 @@ seekstop = gst_base_parse_find_offset (parse, seeksegment.stop, FALSE, NULL); + seeksegment.start = seeksegment.time = seeksegment.position = start_ts; This is what breaks seeking. The way stop is set to the duration seems strange in the first place, as if duration is not -1, stop shall be extrapolate to stop = start + duration. About the extrapolated value of start, I'm not sure why you do that, but so far it seems to confuse do_seek() helper.
Created attachment 307642 [details] [review] baseparse: Fix extrapolation of seeksegment.stop The stop shall be relative to start if extrapolated from the duration.
Created attachment 307643 [details] [review] baseparse: Don't override gst_segment_do_seek() This line has no purpose, clearly gst_segment_do_seek() is doing the right job, also, having the start time (a timestamp) be that same as time (the stream time) is quite odd.
Attachment 307642 [details] pushed as 8b6e870 - baseparse: Fix extrapolation of seeksegment.stop Attachment 307643 [details] pushed as 5b5cebf - baseparse: Don't override gst_segment_do_seek()
What exactly was the issue? and how to reproduce it? Does this mean we have to modify in qtdemux as well?.. Maybe similar issue exists in qtdemux.. This is the code in qtdemux from which i had referenced.. gst_qtdemux_perform_seek { ... segment->position = desired_offset; segment->time = desired_offset; if (segment->rate >= 0) { segment->start = desired_offset; /* we stop at the end */ if (segment->stop == -1) segment->stop = segment->duration; } else { segment->stop = desired_offset; } ... }
(In reply to Vineeth from comment #22) > What exactly was the issue? and how to reproduce it? For the record, before, open playback-test with an mp3, seek multiple time. Some seek, specially the one done at near the start endup at wrong position. For the duration, it's evident to me, but I didn't took that time to reproduce. You need a special pipeline where the segment start is not zero (rare). > > Does this mean we have to modify in qtdemux as well?.. Maybe similar issue > exists in qtdemux.. > > This is the code in qtdemux from which i had referenced.. > gst_qtdemux_perform_seek > { > ... > segment->position = desired_offset; > segment->time = desired_offset; > if (segment->rate >= 0) { > segment->start = desired_offset; > > /* we stop at the end */ > if (segment->stop == -1) > segment->stop = segment->duration; > } else { > segment->stop = desired_offset; > } > ... > } That would need to be looked at. Note that it's doing it slightly differently here.