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 701101 - If a sink is in async, pipeline reports wrong position
If a sink is in async, pipeline reports wrong position
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal major
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-27 20:44 UTC by Youness Alaoui
Modified: 2018-11-03 12:18 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
BaseSink: Must preserve invalid start/stop times instead of clipping them to full stream duration (1.49 KB, patch)
2013-05-27 20:44 UTC, Youness Alaoui
needs-work Details | Review

Description Youness Alaoui 2013-05-27 20:44:57 UTC
Created attachment 245411 [details] [review]
BaseSink: Must preserve invalid start/stop times instead of clipping them to full stream duration

I'm currently working with the gst-rtsp-server using GStreamer 1.0 (all repos on git master) and I have a problem running more than one client when the media is shared. Problem is that the rtsp server gives a range from 'duration to duration' instead of 'position to duration' which results in all buffers being dropped in the client since they are out of segment.

The cause seems to be the get_position query done by the rtsp-server on the pipeline, it actually returns the duration instead of the position. Enabling appropriate debugs, shows that most elements return the right position apart from 'appsink' which returns the duration, then the bin simply choses the maximum reported position as the result, thus leading in the duration-as-position bug.

Debugging the issue further down, it seems to be caused by the gstbasesink code coupled with the fact that appsink is created with async=false. Since async is false, basesink does not have a clock and must return the 'last seen timestamp' as the current position (stored in priv->current_sstop). This is fine in itself, the problem is that the 'last seen timestamp' is wrong. The code that sets it (in do_sync) calls gst_base_sink_get_sync_times then does this :
  priv->current_sstart = sstart;
  priv->current_sstop = (GST_CLOCK_TIME_IS_VALID (sstop) ? sstop : sstart);

The current_sstart represents the current buffer's timestamp, and the current_sstop represents the current buffer's ending time (buffer->timestamp + buffer->duration). If the duration is invalid, then sstop is invalid, and current_sstop should be set to sstart.

The problem/bug is that gst_base_sink_get_sync_times will never return an invalid time for sstop because it will get clipped to the segment :
basesink gstbasesink.c:1877:gst_base_sink_get_sync_times:<multiudpsink2> got times start: 0:00:02.816000000, stop: 99:99:99.999999999, do_sync 1
basesink gstbasesink.c:1903:gst_base_sink_get_sync_times:<multiudpsink2> clipped to: start 0:00:02.816000000, stop: 0:58:53.695000000

This causes the check for invalid stop time to become dead code :
  priv->current_sstop = (GST_CLOCK_TIME_IS_VALID (sstop) ? sstop : sstart);

This issue with the start/stop never being invalid as returned from get_sync_times will also affect other parts of the code, like for example in gst_base_sink_is_too_late which uses the average diff in frame in case stop is invalid, this bug making that code useless now as well.

I believe the fix would be to simply not clip the sstop/sstart or rstart/rstop values if they are originally invalid, in order to preserve the invalid timestamps where they should be. 
I'm attaching a small patch that should fix it, but since I'm not familiar with that part of the code, I'd appreciate a review from someone who knows more about the synchronization code in basesink.

Thanks.
Comment 1 Wim Taymans 2013-05-30 05:08:22 UTC
commit 10099e1e704354ec45a1ea8ae394af07c868c1f3
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Thu May 30 07:03:40 2013 +0200

    check: fix position unit test

commit cf4334fbfb80abc40172897e54ce4a5faf42e3fb
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Thu May 30 06:51:24 2013 +0200

    basesink: improve position reporting without clock
    
    When no base time or when sync is disabled, use the same logic as
    in paused to report position. The logic in PLAYING assumes we use the
    clock.
Comment 2 Youness Alaoui 2013-06-10 16:49:44 UTC
Thanks Wim for the fix but this doesn't fix the issue of dead code I mentioned in the bug report, which might affect other logic in the basesink.

For example, the current_sstop and current_rstop will still be set to the segment stop if a buffer's duration is invalid, which will affect other parts of the code like the calculation of max_lateness, or the perform_qos.
Was there any reason that the patch I attached was rejected and you decided to fix it differently ? Let me know if there's anything I need to change in the attached patch.
Thanks.
Comment 3 Wim Taymans 2013-06-25 18:42:11 UTC
(In reply to comment #2)
> Thanks Wim for the fix but this doesn't fix the issue of dead code I mentioned
> in the bug report, which might affect other logic in the basesink.
> 
> For example, the current_sstop and current_rstop will still be set to the
> segment stop if a buffer's duration is invalid, which will affect other parts
> of the code like the calculation of max_lateness, or the perform_qos.
> Was there any reason that the patch I attached was rejected and you decided to
> fix it differently ? Let me know if there's anything I need to change in the
> attached patch.
> Thanks.

I didn't want to touch that logic, mostly. A -1 duration by definition means 'until the next buffer' or end of segment, so this logic extends the end time of a buffer with -1 duration to the end of the segment.

I will have to take a deeper look later.
Comment 4 Wim Taymans 2013-06-27 12:59:35 UTC
> This issue with the start/stop never being invalid as returned from
> get_sync_times will also affect other parts of the code, like for example in
> gst_base_sink_is_too_late which uses the average diff in frame in case stop is
> invalid, this bug making that code useless now as well.

start/stop can be invalid when the segment has no stop time.

> Let me know if there's anything I need to change in the attached patch.

For stepping it seems like it needs to snap to the end of the segment, when known. Before we try to change this, we should really have some unit tests for this..
Comment 5 Sebastian Dröge (slomo) 2013-06-30 16:09:09 UTC
(In reply to comment #3)
> I will have to take a deeper look later.

So let's reopen this?
Comment 6 Tim-Philipp Müller 2013-07-10 18:13:53 UTC
Why is this milestoned for 1.0.8 ? Is there a regression somewhere?
Comment 7 Sebastian Dröge (slomo) 2013-07-24 09:31:57 UTC
Comment on attachment 245411 [details] [review]
BaseSink: Must preserve invalid start/stop times instead of clipping them to full stream duration

Needs at least a unit test
Comment 8 Sebastian Dröge (slomo) 2013-07-24 09:32:40 UTC
So what should we do about this? Is there still anything that must be fixed or is it just cleanup now and someone might or might not work on it?
Comment 9 Wim Taymans 2013-07-24 09:42:02 UTC
First we badly need a unit test for the stepping scenarios. The stepping bits are a bit everywhere and it's not so easy to be sure that you don't break anything. Then, possibly, we could clean up some things.
Comment 10 GStreamer system administrator 2018-11-03 12:18:25 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/gstreamer/issues/39.