GNOME Bugzilla – Bug 796559
qtdemux: Various segment fixes to properly take segment.offset into account
Last modified: 2018-11-03 15:31:09 UTC
See commit messages for details
Created attachment 372632 [details] [review] qtdemux: Update the segment for KEY_UNIT seeks via gst_segment_do_seek() Manually we were not adjusting the segment.offset accordingly, which caused jumps in running time if cur_type==GST_SEEK_TYPE_NONE in the seek. The demuxer segment is now always the actual seek segment instead of any manually adjusted segment, and it is taken as the base for generating the per-stream / per edit list segment segments.
Created attachment 372633 [details] [review] qtdemux: Calculate per-stream segment base directly from the segment There is no need to manually calculate it, we have the expected value directly available in the demuxer segment. And the previous calculations were not adjusting the segment.offset accordingly, thus shifting the running time. As a nice side-effect this makes it unnecessary to accumulate the base time of the different edit list segments.
Review of attachment 372633 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +4897,3 @@ stream->segment.applied_rate = qtdemux->segment.applied_rate; stream->segment.rate = rate; stream->segment.start = start + QTSTREAMTIME_TO_GSTTIME (stream, In theory, segment.time should be gst_segment_to_stream_time(offset) here and we would not have to calculate it.
Created attachment 372635 [details] [review] qtdemux: Correctly set segment.position for each stream segment if rate < 0
(In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 372633 [details] [review] [review]: > > ::: gst/isomp4/qtdemux.c > @@ +4897,3 @@ > stream->segment.applied_rate = qtdemux->segment.applied_rate; > stream->segment.rate = rate; > stream->segment.start = start + QTSTREAMTIME_TO_GSTTIME (stream, > > In theory, segment.time should be gst_segment_to_stream_time(offset) here > and we would not have to calculate it. Actually no, it must be stream time of offset forwards (that's exactly what it does now), or backwards the stream time of demux->segment.start (aka demux->segment.time), which it also does already (except for some weird special case that makes no sense to me).
Review of attachment 372632 [details] [review]: ::: gst/isomp4/qtdemux.c @@ +1566,3 @@ + /* TODO: If !update we can keep all the streams at their current + * position and don't have to go through all the edst/segment finding + * machinary. We only need to send an updated segment in that case. Isn't sending an updated segment required for all kinds of seeks? @@ -1566,3 @@ - /* may not have enough fragmented info to do this adjustment, - * and we can't scan (and probably should not) at this time with - * possibly flushing upstream */ What was this about "flushing upstream"? gst_qtdemux_perform_seek() is only called by gst_qtdemux_do_seek(), which is only called in pull mode. @@ +1731,3 @@ } else { /* now do the seek */ + ret = gst_qtdemux_perform_seek (qtdemux, &seeksegment, TRUE, seqnum, flags); Not something to be fixed by this patch, but maybe worth pointing out: this branch can never execute since `event` cannot be NULL. For event to be NULL, gst_qtdemux_do_seek() would have to be called with a NULL event. The only call is in gst_qtdemux_handle_src_event(), when the event type is GST_EVENT_SEEK... Since the type of the event has been checked at that point -- which required indirecting the pointer, event cannot be NULL.
(In reply to Alicia Boya García from comment #6) > Review of attachment 372632 [details] [review] [review]: > > ::: gst/isomp4/qtdemux.c > @@ +1566,3 @@ > + /* TODO: If !update we can keep all the streams at their current > + * position and don't have to go through all the edst/segment finding > + * machinary. We only need to send an updated segment in that case. > > Isn't sending an updated segment required for all kinds of seeks? Yes, "only" is the keyword here. Only a new segment is needed, not all the other stuff that is done to find the correct edst segment, and then finding the correct sample in there, etc. > @@ -1566,3 @@ > - /* may not have enough fragmented info to do this adjustment, > - * and we can't scan (and probably should not) at this time with > - * possibly flushing upstream */ > > What was this about "flushing upstream"? gst_qtdemux_perform_seek() is only > called by gst_qtdemux_do_seek(), which is only called in pull mode. I don't know, I just moved that comment with the code. It made no sense to me either. > @@ +1731,3 @@ > } else { > /* now do the seek */ > + ret = gst_qtdemux_perform_seek (qtdemux, &seeksegment, TRUE, seqnum, > flags); > > Not something to be fixed by this patch, but maybe worth pointing out: this > branch can never execute since `event` cannot be NULL. For event to be NULL, > gst_qtdemux_do_seek() would have to be called with a NULL event. The only > call is in gst_qtdemux_handle_src_event(), when the event type is > GST_EVENT_SEEK... Since the type of the event has been checked at that point > -- which required indirecting the pointer, event cannot be NULL. This is probably leftover from an older version of the code, where it maybe did a NULL seek in pull mode at the very beginning. Not sure.
Alicia, do you see any problems with the current patch?
(In reply to Sebastian Dröge (slomo) from comment #8) > Alicia, do you see any problems with the current patch? Sorry, I have not finished reviewing them yet.
(In reply to Sebastian Dröge (slomo) from comment #7) > (In reply to Alicia Boya García from comment #6) > > Review of attachment 372632 [details] [review] [review] [review]: > > > > ::: gst/isomp4/qtdemux.c > > @@ +1566,3 @@ > > + /* TODO: If !update we can keep all the streams at their current > > + * position and don't have to go through all the edst/segment finding > > + * machinary. We only need to send an updated segment in that case. > > > > Isn't sending an updated segment required for all kinds of seeks? > > Yes, "only" is the keyword here. Only a new segment is needed, not all the > other stuff that is done to find the correct edst segment, and then finding > the correct sample in there, etc. I see... the wording is ambiguous so I would rephrase it as: "In that case, we only need to send an updated segment."
Review of attachment 372632 [details] [review]: Looks good to me.
Review of attachment 372633 [details] [review]: Conceptually looks good to me. Needs a rebase on master, but it may be better to do it after https://bugzilla.gnome.org/show_bug.cgi?id=752603 is committed to avoid dealing with min_ts. Unfortunately, it regresses a couple tests in elements_splitmux, probably related somehow with reverse playback. 75%: Checks: 8, Failures: 2, Errors: 0 ../subprojects/gstreamer/libs/gst/check/gstcheck.c:286:F:caps_change:test_splitmuxsrc_caps_change:0: Unexpected critical/warning: gst_segment_to_running_time_full: assertion 'stop != -1' failed ../subprojects/gstreamer/libs/gst/check/gstcheck.c:286:F:caps_change:test_splitmuxsrc_robust_mux:0: Unexpected critical/warning: gst_segment_to_running_time_full: assertion 'stop != -1' failed Note gst_segment_get_running_time() requires `stop != -1` when `rate < 0`. `duration != -1` is not enough (modifying gst_segment_get_running_time_full() to allow it makes the test pass, but I'm not sure if that's the way to go... Indeed, we still have no consensus on what GstSegment.duration is about and the documentation states contradictory information (the duration of the stream/movie vs the duration of the segment).
Review of attachment 372635 [details] [review]: Makes sense.
(In reply to Alicia Boya García from comment #12) > Note gst_segment_get_running_time() requires `stop != -1` when `rate < 0`. > `duration != -1` is not enough Who is creating segments with stop == -1 and rate < 0 here? That's just wrong, you really need stop because playback goes from stop to start :)
Created attachment 372722 [details] [review] splitmuxpartreader: set stop != -1 when doing reverse playback
Review of attachment 372633 [details] [review]: Now that the splitmux issue has been discussed and fixed, the patch is good to go.
Created attachment 372723 [details] [review] gstelement: assert start/stop are set according to seek rate Seeks are supposed to have a valid start or stop time depending on whether the rate is positive or negative respectively. Before, this was left to be enforced by the elements themselves, who sometimes did not account for these invalid seeks. This patch introduces assertions in gst_event_new_seek() to ensure such seeks are not created in the first place. This patch also fixes one instance of such a seek in filesrc tests. No other new test failures have become apparent after running `meson test`, `gst-validate-launcher` and `gst-validate-launcher adaptive`.
Not sure for this last one, have you ran ges test suite in validate. I have vague memory that seeking this way meant something. Like doing a flushing seek without changing the playback position to redender with some new filters or new filter settings.
(In reply to Nicolas Dufresne (ndufresne) from comment #18) > Not sure for this last one, have you ran ges test suite in validate. I have > vague memory that seeking this way meant something. Like doing a flushing > seek without changing the playback position to redender with some new > filters or new filter settings. I've never run ges testsuite. How can I do so? $ gst-validate-launcher ges Could not load testsuite: <module 'ges' from '/home/ntrrgc/gst-validate/gst-integration-testsuites/testsuites/ges.py'> maybe because of missing TestManager Note that the assertion does not require to specify a new start or end, just that in case you do (start_type or stop_type different than GST_SEEK_TYPE_NONE, then then start or stop must be valid timestamps).
(In reply to Nicolas Dufresne (ndufresne) from comment #18) > Not sure for this last one, have you ran ges test suite in validate. I have > vague memory that seeking this way meant something. Like doing a flushing > seek without changing the playback position to redender with some new > filters or new filter settings. Yes but that requires start_type and/or stop_type to be NONE. Apart from that stop==-1 is invalid for negative rates and start==-1 for positive rates.
Review of attachment 372723 [details] [review]: The commit message is wrong, this is mostly about "event" not "element" :) Otherwise seems fine to me ::: tests/check/elements/filesrc.c @@ +144,3 @@ /* reverse seek from end to start */ gst_element_seek (src, -1.0, GST_FORMAT_BYTES, 0, GST_SEEK_TYPE_SET, 100, + GST_SEEK_TYPE_END, 0); This is very optimistic. I'm surprised it works... and that filesrc is doing something meaningful with negative rate at all
Review of attachment 372723 [details] [review]: ::: gst/gstelement.c @@ +1897,3 @@ gst_event_new_seek (rate, format, flags, start_type, start, stop_type, stop); + g_return_val_if_fail (GST_IS_EVENT (event), FALSE); That should just be a regular "if (event) return FALSE;" g_return* functions should only be used for inputs. And you will already have an assert triggered in gst_event_new_seek() ::: gst/gstevent.c @@ +1267,3 @@ g_return_val_if_fail (rate != 0.0, NULL); + if (rate > 0) + g_return_val_if_fail (GST_CLOCK_TIME_IS_VALID (start) || start is a 'gint64' If you do a (GST_FORMAT_BYTES, SEEK_TYPE_END, -1) (i.e. the end position in bytes minus 1) it is valid.
Review of attachment 372723 [details] [review]: ::: gst/gstevent.c @@ +1267,3 @@ g_return_val_if_fail (rate != 0.0, NULL); + if (rate > 0) + g_return_val_if_fail (GST_CLOCK_TIME_IS_VALID (start) || Good point. I guess then this check should only be made for GST_SEEK_TYPE_SET, right? Or would that be wrong too? I'm supposing doing a GST_SEEK_TYPE_SET to a negative position is not supported. ::: tests/check/elements/filesrc.c @@ +144,3 @@ /* reverse seek from end to start */ gst_element_seek (src, -1.0, GST_FORMAT_BYTES, 0, GST_SEEK_TYPE_SET, 100, + GST_SEEK_TYPE_END, 0); Well, "it works" may be a overstatement. filesrc does not support reverse playback... and I don't blame it, as reading a file byte per byte in reverse order is rarely useful. I have no idea why is there a `test_reverse` in filesrc in the first place, given that it does not support reverse playback. It does not really support GST_SEEK_TYPE_END either. Whenever stop_type != GST_SEEK_TYPE_SET it just reads until the end of the file... which keeps the same behavior before the patch without hitting the new assertion.
This broke indeed 2 `validate ges` tests: ges.playback.scrub_backward_seeking.test_effect.audio_video.vorbis_theora_ogg: Failed 'Application returned 18 (critical errors: [A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek])' You can reproduce with: 'DISPLAY=':0' GST_GL_XINITTHREADS='1' GST_VALIDATE_SCENARIOS_PATH='/home/ntrrgc/Apps/gst-build/prefix/share/gstreamer-1.0/validate/scenarios:/home/ntrrgc/Apps/gst-build/subprojects/gst-devtools/validate/data/scenarios' GST_VALIDATE_SCENARIO='scrub_backward_seeking' ges-launch-1.0 --mute --ges-sample-path-recurse file:///home/ntrrgc/gst-validate/gst-integration-testsuites/medias/defaults -l file:///home/ntrrgc/gst-validate/gst-integration-testsuites/ges/ges-projects/vorbis_theora/test_effect.audio_video.vorbis_theora_ogg.xges' Logs: - /home/ntrrgc/gst-validate/logs/ges/playback/scrub_backward_seeking/test_effect/audio_video/vorbis_theora_ogg ges.playback.scrub_backward_seeking.test_effect.audio_video.vorbis_vp8_webm: Failed 'Application returned 18 (critical errors: [A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek, A segment doesn't have the proper time value after an ACCURATE seek])' You can reproduce with: 'DISPLAY=':0' GST_GL_XINITTHREADS='1' GST_VALIDATE_SCENARIOS_PATH='/home/ntrrgc/Apps/gst-build/prefix/share/gstreamer-1.0/validate/scenarios:/home/ntrrgc/Apps/gst-build/subprojects/gst-devtools/validate/data/scenarios' GST_VALIDATE_SCENARIO='scrub_backward_seeking' ges-launch-1.0 --mute --ges-sample-path-recurse file:///home/ntrrgc/gst-validate/gst-integration-testsuites/medias/defaults -l file:///home/ntrrgc/gst-validate/gst-integration-testsuites/ges/ges-projects/vorbis_vp8_webm/test_effect.audio_video.vorbis_vp8_webm.xges' Logs: - /home/ntrrgc/gst-validate/logs/ges/playback/scrub_backward_seeking/test_effect/audio_video/vorbis_vp8_webm
Some of these patches are based upon an invalid assumption that you aren't allowed to seek with stop = -1 and rate < 0. GstSegment explicitly has code to support that and set the position = seg->duration in gst_segment_do_seek(). There's no requirement for stop != -1 just because rate < 0.0
gst_segment_to_running_time_full() does however enforce stop != 0 for rate < 0.0, so that's inconsistent. I think modifying gst_segment_to_running_time_full() to allow it is the better path, because changing gst_element_do_seek() to reject it breaks existing elements, so it's not backwards compatible
Created attachment 373713 [details] [review] segment: Allow stop == -1 in gst_segment_to_running_time() and rate < 0 If a segment has stop == -1, then gst_segment_to_running_time() would refuse to calculate a running time, but gst_segment_do_seek() allows this scenario and uses a valid duration for calculates. Make the 2 functions consistent by using any configured duration to calculate a running time too.
Comment on attachment 373713 [details] [review] segment: Allow stop == -1 in gst_segment_to_running_time() and rate < 0 commit 80015d69a778333166f1d45de7341019e2180557 (HEAD -> master) Author: Jan Schmidt <jan@centricular.com> Date: Thu Sep 20 23:17:52 2018 +1000 segment: Allow stop == -1 in gst_segment_to_running_time() and rate < 0 If a segment has stop == -1, then gst_segment_to_running_time() would refuse to calculate a running time for negative rates, but gst_segment_do_seek() allows this scenario and uses a valid duration for calculations. Make the 2 functions consistent by using any configured duration to calculate a running time too in that case. https://bugzilla.gnome.org/show_bug.cgi?id=796559
Created attachment 374092 [details] [review] qtdemux: Update the segment for KEY_UNIT seeks via gst_segment_do_seek() Manually we were not adjusting the segment.offset accordingly, which caused jumps in running time if cur_type==GST_SEEK_TYPE_NONE in the seek. The demuxer segment is now always the actual seek segment instead of any manually adjusted segment, and it is taken as the base for generating the per-stream / per edit list segment segments.
Created attachment 374093 [details] [review] qtdemux: Calculate per-stream segment base directly from the segment There is no need to manually calculate it, we have the expected value directly available in the demuxer segment. And the previous calculations were not adjusting the segment.offset accordingly, thus shifting the running time. As a nice side-effect this makes it unnecessary to accumulate the base time of the different edit list segments.
Created attachment 374094 [details] [review] qtdemux: Correctly set segment.position for each stream segment if rate < 0
Created attachment 374095 [details] [review] splitmuxpartreader: set stop != -1 when doing reverse playback
Comment on attachment 372723 [details] [review] gstelement: assert start/stop are set according to seek rate This changes a valid assumption in 1.0 that seeks with stop==-1 imply the stop time is the end of the stream.
Rebased the remaining -good patches onto git master.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/483.