[Spice-devel] [PATCH spice-gtk] channel-display-gst: Avoid PTS to decrease

[Uri Lublin]

On 1/31/19 5:43 PM, Frediano Ziglio wrote:
> GStreamer does not accept PTS to decrease and set the new PTS
> with the value of the previous one.

Hi,

I looked in gstreamer documentation [0] that mentions:
"The buffer PTS refers to the timestamp when the buffer
  content should be presented to the user and is not always
  monotonically increasing."

It does not mean your claim is not true - maybe a decreasing
PTS is only allowed for H264 B frames (and similar).

[0] 
https://gstreamer.freedesktop.org/data/doc/gstreamer/1.14/gstreamer/html/GstBuffer.html

> This causes our code to not match output frames with input ones.
> Make sure PTS always increases (even if very slowly).

Consider adding that this can happen when "latency" is changing
and becomes smaller.

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Other than that, looks good to me.

Uri.


> ---
>   src/channel-display-gst.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> This should avoid the issues the following patch is trying
> also to fix:
> https://patchwork.freedesktop.org/patch/277721/
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 4272ade8..7d7b435d 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -42,6 +42,8 @@ typedef struct SpiceGstDecoder {
>       GstAppSink *appsink;
>       GstElement *pipeline;
>       GstClock *clock;
> +    // minimum PTS attached to the next frame to avoid decreasing it
> +    GstClockTime next_minimum_pts;
>   
>       /* ---------- Decoding and display queues ---------- */
>   
> @@ -657,9 +659,14 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>                                                       frame->data, frame->size, 0, frame->size,
>                                                       frame, (GDestroyNotify) spice_frame_free);
>   
> +    GstClockTime pts = gst_clock_get_time(decoder->clock) -
> +                       gst_element_get_base_time(decoder->pipeline) +
> +                       ((uint64_t)MAX(0, latency)) * 1000 * 1000;
> +    pts = MAX(pts, decoder->next_minimum_pts);
> +    decoder->next_minimum_pts = pts + 1;
>       GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
>       GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> -    GST_BUFFER_PTS(buffer) = gst_clock_get_time(decoder->clock) - gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) * 1000 * 1000;
> +    GST_BUFFER_PTS(buffer) = pts;
>   
>       SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
>       g_mutex_lock(&decoder->queues_mutex);
>