[Bug 701101] New: If a sink is in async, pipeline reports wrong position

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=701101
  GStreamer | gstreamer (core) | git

           Summary: If a sink is in async, pipeline reports wrong position
    Classification: Platform
           Product: GStreamer
           Version: git
        OS/Version: Linux
            Status: UNCONFIRMED
          Severity: major
          Priority: Normal
         Component: gstreamer (core)
        AssignedTo: gstreamer-bugs at lists.freedesktop.org
        ReportedBy: youness.alaoui at collabora.co.uk
         QAContact: gstreamer-bugs at lists.freedesktop.org
     GNOME version: ---


Created an attachment (id=245411)
 View: https://bugzilla.gnome.org/attachment.cgi?id=245411
 Review: https://bugzilla.gnome.org/review?bug=701101&attachment=245411

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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.