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

[Frediano Ziglio]

> 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 looks like something in the pipeline is making it monotonic.
Maybe is something related to H264, maybe not but happened to
me and Snir too.

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

Well, no even streams with B-frames output frames in the
right sequence or you would see weird artifacts :-)

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

What about:

"This can happen if "latency" argument becomes smaller or during
the synchronization of audio and video times."

?

> > 
> > 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);
> > 
> 
>