If a sink is in async, pipeline reports wrong position

Submitted by Youness Alaoui

Link to original bug (#701101)

Description

Created 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.

Patch 245411, "BaseSink: Must preserve invalid start/stop times instead of clipping them to full stream duration":
get-sync-times-preserve-invalid-timestamps.patch