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

[Snir Sheriber]

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 at redhat.com>
>> ---
>>
>> 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