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

[Snir Sheriber <ssheribe@xxxxxxxxxx>]

Hi,

On 1/1/19 11:51 AM, 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.
> >
> > Signed-off-by: Snir Sheriber <ssheribe@xxxxxxxxxx>
> > ---
> >
> > Currently it's causing to an error if you running with
s/error/warning/
> > DISABLE_GSTVIDEOOVERLAY=1
> > and start some audio playback in the guest.
> The patch make sense but looking at the commit message looks like that
> to remove a warning you introduced an error.
> It would be worth mentioning in which case(s) you have the warning and
> which warning (I supposed "got an unexpected decoded buffer!" or
> "the GStreamer pipeline dropped a frame").
> Which error are you getting now?


You are right, a warning, not an error,I meant to explain how i got the 
"got an unexpected decoded buffer!" warning

before applying this patch :p

Snir.

> > ---
> >   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 2c07f35..a00f53e 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;
> >   
> > @@ -88,7 +92,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;
> > @@ -213,6 +224,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;
> > +#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.
> > @@ -225,7 +242,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;
> > @@ -234,7 +251,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
> > @@ -648,9 +665,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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel