Re: [PATCH v3 spice-gtk] gstreamer: set timestamp in buffer's GstReferenceTimestampMeta

[Snir Sheriber <ssheribe@xxxxxxxxxx>]

Hi,

On 1/17/19 2:27 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@xxxxxxxxxx>
> > ---
> >
> > 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?

I think this indeed what happened, when stream started, mm_time was
reset so it caused some frames timestamps to have smaller values
than the previous ones. in response, gstreamer just used the last
value it has for the next couple of frames.

This would have work if we were letting appsink to synchronize using
gsteramer, as long as we are doing the synchronization outside of
gstreamer it seems to me that it's preferred that timestamps will be
also "outside of gstreamer" (not really outside but their values are
untouchable).


>    Maybe we should prevent this in our code instead.


Maybe we should.


> - why for you the metadata is at the output and in my case not?


It is surprising but i just noticed this comment i channel-display-gst.c

"gst_app_sink_pull_sample() sometimes returns the same buffer twice
 or buffers that have a modified, and thus unrecognizable, PTS."

This is indeed weird


>    Which codec where you using? Normal videos or streaming?


I used streaming + vaapi

Snir.

> > ---
> >   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);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel