[Spice-devel] [PATCH v3 spice-gtk] 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 v2:
> -Use buffer's PTS value, if GstReferenceTimestampMeta exists, use its
> timestamp instead.
> 

Surely this version won't crash but I'd like to understand better
the various behaviours:
- why Gstreamer changes PTS? Now that you I think I remember that GStreamer
  make sure that PTS is monotonic, in the sense that has always to
  grow, if you pass a buffer with a PTS smaller than the previous it
  changes the value to keep it monotonic. Is this your case?
  Maybe we should prevent this in our code instead.
- why for you the metadata is at the output and in my case not?
  Which codec where you using? Normal videos or streaming?

> ---
>  src/channel-display-gst.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 2f556fe..a281032 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,16 @@ struct SpiceGstFrame {
>  static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
>  {
>      SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
> +
>      gstframe->timestamp = GST_BUFFER_PTS(buffer);
> +#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));
> +    if (time_meta) {
> +        gstframe->timestamp = time_meta->timestamp;
> +    }
> +#endif
>      gstframe->frame = frame;
>      gstframe->sample = NULL;
>      return gstframe;
> @@ -211,6 +224,15 @@ static void fetch_pending_sample(SpiceGstDecoder
> *decoder)
>          decoder->pending_samples--;
>  
>          GstBuffer *buffer = gst_sample_get_buffer(sample);
> +        GstClockTime buffer_ts = GST_BUFFER_PTS(buffer);
> +#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));
> +        if (time_meta) {
> +            buffer_ts = time_meta->timestamp;
> +        }
> +#endif
>  
>          /* gst_app_sink_pull_sample() sometimes returns the same buffer
>          twice
>           * or buffers that have a modified, and thus unrecognizable, PTS.
> @@ -223,7 +245,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 +254,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 +648,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);