After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796559 - qtdemux: Various segment fixes to properly take segment.offset into account
qtdemux: Various segment fixes to properly take segment.offset into account
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: High critical
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-06-11 10:19 UTC by Sebastian Dröge (slomo)
Modified: 2018-11-03 15:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
qtdemux: Update the segment for KEY_UNIT seeks via gst_segment_do_seek() (5.32 KB, patch)
2018-06-11 10:20 UTC, Sebastian Dröge (slomo)
none Details | Review
qtdemux: Calculate per-stream segment base directly from the segment (3.09 KB, patch)
2018-06-11 10:20 UTC, Sebastian Dröge (slomo)
none Details | Review
qtdemux: Correctly set segment.position for each stream segment if rate < 0 (1.03 KB, patch)
2018-06-11 10:56 UTC, Sebastian Dröge (slomo)
none Details | Review
splitmuxpartreader: set stop != -1 when doing reverse playback (1.00 KB, patch)
2018-06-19 17:33 UTC, Alicia Boya García
none Details | Review
gstelement: assert start/stop are set according to seek rate (2.57 KB, patch)
2018-06-19 19:28 UTC, Alicia Boya García
rejected Details | Review
segment: Allow stop == -1 in gst_segment_to_running_time() and rate < 0 (1.16 KB, patch)
2018-09-20 13:26 UTC, Jan Schmidt
committed Details | Review
qtdemux: Update the segment for KEY_UNIT seeks via gst_segment_do_seek() (5.32 KB, patch)
2018-10-28 17:47 UTC, Jan Schmidt
none Details | Review
qtdemux: Calculate per-stream segment base directly from the segment (3.54 KB, patch)
2018-10-28 17:47 UTC, Jan Schmidt
none Details | Review
qtdemux: Correctly set segment.position for each stream segment if rate < 0 (1.03 KB, patch)
2018-10-28 17:48 UTC, Jan Schmidt
none Details | Review
splitmuxpartreader: set stop != -1 when doing reverse playback (1.00 KB, patch)
2018-10-28 17:48 UTC, Jan Schmidt
none Details | Review

Description Sebastian Dröge (slomo) 2018-06-11 10:19:54 UTC
See commit messages for details
Comment 1 Sebastian Dröge (slomo) 2018-06-11 10:20:00 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2018-06-11 10:20:07 UTC
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.
Comment 3 Sebastian Dröge (slomo) 2018-06-11 10:27:53 UTC
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.
Comment 4 Sebastian Dröge (slomo) 2018-06-11 10:56:45 UTC
Created attachment 372635 [details] [review]
qtdemux: Correctly set segment.position for each stream segment if rate < 0
Comment 5 Sebastian Dröge (slomo) 2018-06-11 10:58:19 UTC
(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).
Comment 6 Alicia Boya García 2018-06-11 15:57:22 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2018-06-11 20:33:19 UTC
(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.
Comment 8 Sebastian Dröge (slomo) 2018-06-12 15:59:32 UTC
Alicia, do you see any problems with the current patch?
Comment 9 Alicia Boya García 2018-06-12 17:53:52 UTC
(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.
Comment 10 Alicia Boya García 2018-06-12 19:09:40 UTC
(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."
Comment 11 Alicia Boya García 2018-06-12 22:38:36 UTC
Review of attachment 372632 [details] [review]:

Looks good to me.
Comment 12 Alicia Boya García 2018-06-12 23:12:56 UTC
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).
Comment 13 Alicia Boya García 2018-06-12 23:13:41 UTC
Review of attachment 372635 [details] [review]:

Makes sense.
Comment 14 Sebastian Dröge (slomo) 2018-06-13 05:56:42 UTC
(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 :)
Comment 15 Alicia Boya García 2018-06-19 17:33:11 UTC
Created attachment 372722 [details] [review]
splitmuxpartreader: set stop != -1 when doing reverse playback
Comment 16 Alicia Boya García 2018-06-19 17:37:17 UTC
Review of attachment 372633 [details] [review]:

Now that the splitmux issue has been discussed and fixed, the patch is good to go.
Comment 17 Alicia Boya García 2018-06-19 19:28:36 UTC
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`.
Comment 18 Nicolas Dufresne (ndufresne) 2018-06-19 19:37:30 UTC
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.
Comment 19 Alicia Boya García 2018-06-19 22:30:50 UTC
(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).
Comment 20 Sebastian Dröge (slomo) 2018-06-20 05:56:45 UTC
(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.
Comment 21 Sebastian Dröge (slomo) 2018-06-20 05:59:17 UTC
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
Comment 22 Edward Hervey 2018-06-20 08:22:12 UTC
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.
Comment 23 Alicia Boya García 2018-06-20 20:47:54 UTC
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.
Comment 24 Alicia Boya García 2018-06-21 19:47:08 UTC
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
Comment 25 Jan Schmidt 2018-09-20 13:07:48 UTC
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
Comment 26 Jan Schmidt 2018-09-20 13:14:24 UTC
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
Comment 27 Jan Schmidt 2018-09-20 13:26:01 UTC
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 28 Jan Schmidt 2018-10-28 17:05:49 UTC
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
Comment 29 Jan Schmidt 2018-10-28 17:47:44 UTC
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.
Comment 30 Jan Schmidt 2018-10-28 17:47:53 UTC
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.
Comment 31 Jan Schmidt 2018-10-28 17:48:00 UTC
Created attachment 374094 [details] [review]
qtdemux: Correctly set segment.position for each stream segment if rate < 0
Comment 32 Jan Schmidt 2018-10-28 17:48:08 UTC
Created attachment 374095 [details] [review]
splitmuxpartreader: set stop != -1 when doing reverse playback
Comment 33 Jan Schmidt 2018-10-28 17:50:57 UTC
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.
Comment 34 Jan Schmidt 2018-10-28 17:51:34 UTC
Rebased the remaining -good patches onto git master.
Comment 35 GStreamer system administrator 2018-11-03 15:31:09 UTC
-- 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.