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

[Snir Sheriber]

Hi,

On 1/15/19 3:42 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 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.


IIUC It should:

"A GstBuffer is the object that is passed from an upstream element to a 
downstream element and contains memory and metadata information."

Actually this is pretty similar to what we are currently doing- counting 
on the PTS (which is may be
changed by the elements in oppose to GstReferenceTimestampMeta which 
should be used as an
alternative timestamp to PTS which is not being touched)


Anyway this could be fixed with a fallback to PTS which i set in any 
case but i want to understand if
there's some bug here, does it always happen to you? can you reproduce 
it consistently?

Snir.


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