[Spice-devel] [PATCH v2] gstreamer: set timestamp in buffer's GstReferenceTimestampMeta

[Frediano Ziglio]

> 
> Hi,
> 
> On 1/15/19 3:42 PM, Frediano Ziglio wrote:
> >> Currently we set timestamps as buffer's PTS, this value may be changed by
> >> the pipeline in some cases and cause an unexpected buffer warnings (when
> >> GstVideoOverlay is not used). Use GstReferenceTimestampMeta when
> >> synchronization is made by spice.
> >>
> >> Before applying this patch you can reproduce the warnings by runing with
> >> DISABLE_GSTVIDEOOVERLAY=1 and starting some audio playback in the guest.
> >>
> >> Signed-off-by: Snir Sheriber <ssheribe at redhat.com>
> >> ---
> >> Changes from v1:
> >> -Commit message
> >> ---
> >>   src/channel-display-gst.c | 27 ++++++++++++++++++++++++---
> >>   1 file changed, 24 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> >> index 2f556fe..a90fa81 100644
> >> --- a/src/channel-display-gst.c
> >> +++ b/src/channel-display-gst.c
> >> @@ -33,6 +33,10 @@ typedef struct SpiceGstFrame SpiceGstFrame;
> >>   
> >>   /* GStreamer decoder implementation */
> >>   
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +static GstStaticCaps stream_reference =
> >> GST_STATIC_CAPS("timestamp/spice-stream");
> >> +#endif
> >> +
> >>   typedef struct SpiceGstDecoder {
> >>       VideoDecoder base;
> >>   
> >> @@ -86,7 +90,14 @@ struct SpiceGstFrame {
> >>   static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame
> >>   *frame)
> >>   {
> >>       SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +    GstReferenceTimestampMeta *time_meta;
> >> +
> >> +    time_meta = gst_buffer_get_reference_timestamp_meta(buffer,
> >> gst_static_caps_get(&stream_reference));
> >> +    gstframe->timestamp = time_meta->timestamp;
> >> +#else
> >>       gstframe->timestamp = GST_BUFFER_PTS(buffer);
> >> +#endif
> >>       gstframe->frame = frame;
> >>       gstframe->sample = NULL;
> >>       return gstframe;
> >> @@ -211,6 +222,12 @@ static void fetch_pending_sample(SpiceGstDecoder
> >> *decoder)
> >>           decoder->pending_samples--;
> >>   
> >>           GstBuffer *buffer = gst_sample_get_buffer(sample);
> >> +        GstClockTime buffer_ts;
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +        buffer_ts = gst_buffer_get_reference_timestamp_meta(buffer,
> >> gst_static_caps_get(&stream_reference))->timestamp;
> > This line crashes for me, gst_buffer_get_reference_timestamp_meta returns
> > NULL. It does not surprise me, you attach metadata to an input buffer
> > and you expect these metadata to be attached to output buffers.
> > Apparently this is not guaranteed.
> 
> 
> IIUC It should:
> 
> "A GstBuffer is the object that is passed from an upstream element to a
> downstream element and contains memory and metadata information."
> 

However does not specify that no additional GstBuffers are created.
Also does not mean that the next downstream element can use and create
another to pass to next in the chain.

> Actually this is pretty similar to what we are currently doing- counting
> on the PTS (which is may be
> changed by the elements in oppose to GstReferenceTimestampMeta which
> should be used as an
> alternative timestamp to PTS which is not being touched)
> 

Is similar but reading the PTS from the buffer cannot cause a NULL
pointer dereference, the field is always there, maybe changed.

> 
> Anyway this could be fixed with a fallback to PTS which i set in any
> case but i want to understand if
> there's some bug here, does it always happen to you? can you reproduce
> it consistently?
> 

I would say 95%, but even 1% crash would be something we should fix.
I use a script to replay videos and with that is really consistent.

> Snir.
> 
> 
> >
> >> +#else
> >> +        buffer_ts = GST_BUFFER_PTS(buffer);
> >> +#endif
> >>   
> >>           /* gst_app_sink_pull_sample() sometimes returns the same buffer
> >>           twice
> >>            * or buffers that have a modified, and thus unrecognizable,
> >>            PTS.
> >> @@ -223,7 +240,7 @@ static void fetch_pending_sample(SpiceGstDecoder
> >> *decoder)
> >>           GList *l = g_queue_peek_head_link(decoder->decoding_queue);
> >>           while (l) {
> >>               gstframe = l->data;
> >> -            if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> >> +            if (gstframe->timestamp == buffer_ts) {
> >>                   /* The frame is now ready for display */
> >>                   gstframe->sample = sample;
> >>                   decoder->display_frame = gstframe;
> >> @@ -232,7 +249,7 @@ static void fetch_pending_sample(SpiceGstDecoder
> >> *decoder)
> >>                    * frames from the decoding queue.
> >>                    */
> >>                   while ((gstframe =
> >>                   g_queue_pop_head(decoder->decoding_queue))) {
> >> -                    if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> >> +                    if (gstframe->timestamp == buffer_ts) {
> >>                           break;
> >>                       }
> >>                       /* The GStreamer pipeline dropped the corresponding
> >> @@ -626,9 +643,13 @@ static gboolean
> >> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> >>                                                       frame->data,
> >>                                                       frame->size, 0,
> >>                                                       frame->size,
> >>                                                       frame->data_opaque,
> >>                                                       frame->unref_data);
> >>   
> >> +    GstClockTime pts = gst_clock_get_time(decoder->clock) -
> >> gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency))
> >> *
> >> 1000 * 1000;
> >>       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;
> >> +#if GST_CHECK_VERSION(1,14,0)
> >> +    gst_buffer_add_reference_timestamp_meta(buffer, gst_static_caps_get
> >> (&stream_reference), pts, GST_CLOCK_TIME_NONE);
> >> +#endif
> >>   
> >>       if (decoder->appsink != NULL) {
> >>           SpiceGstFrame *gst_frame = create_gst_frame(buffer, frame);
> > Frediano
>