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

[Frediano Ziglio]

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

> +#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