[Spice-devel] [PATCH spice-gtk 2/3] channel-display-gst: factor out get_decoded_frame

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, Jan 23, 2019 at 12:08:53AM +0000, Frediano Ziglio wrote:
> > Separate the code from fetch_pending_sample that extracts the
> > SpiceGstFrame relative to a give GstBuffer.
> > This new function will be reused later.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  src/channel-display-gst.c | 84 ++++++++++++++++++++++++---------------
> >  1 file changed, 53 insertions(+), 31 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 1ad06f15..08c9f8fb 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -107,6 +107,7 @@ static void free_gst_frame(SpiceGstFrame *gstframe)
> >  
> >  static void schedule_frame(SpiceGstDecoder *decoder);
> >  static void fetch_pending_sample(SpiceGstDecoder *decoder);
> > +static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder,
> > GstBuffer *buffer);
> >  
> >  static int spice_gst_buffer_get_stride(GstBuffer *buffer)
> >  {
> > @@ -204,6 +205,52 @@ static void schedule_frame(SpiceGstDecoder *decoder)
> >      g_mutex_unlock(&decoder->queues_mutex);
> >  }
> >  
> > +/* Get the decoded frame relative to buffer or NULL if not found.
> > + * Dequeue the frame from decoding_queue and return it, caller
> > + * is responsible to free the pointer.
> > + * queues_mutex must be held.
> > + */
> > +static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder *decoder,
> > GstBuffer *buffer)
> > +{
> > +    guint num_frames_dropped = 0;
> > +
> > +    /* Gstreamer sometimes returns the same buffer twice
> > +     * or buffers that have a modified, and thus unrecognizable, PTS.
> > +     * Blindly removing frames from the decoding_queue until we find a
> > +     * match would only empty the queue, resulting in later buffers not
> > +     * finding a match either, etc. So check the buffer has a matching
> > +     * frame first.
> > +     */
> > +    SpiceGstFrame *gstframe = NULL;
> > +    GList *l = g_queue_peek_head_link(decoder->decoding_queue);
> 
> (1)
> I'm a bit confused. Here, g_queue_peek_head_link() gives the
> pointer to the head, without removing it from the queue.
> 
> > +    while (l) {
> > +        gstframe = l->data;
> > +        if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> 
> (2)
> Here we check if payload of the queue. If yes, then
> 
> > +            /* Now that we know there is a match, remove it and the older
> > +             * frames from the decoding queue.
> > +             */
> > +            while ((gstframe = g_queue_pop_head(decoder->decoding_queue)))
> > {
> 
> It gets the same pointer, because still is the head as it was not
> popped above on (1).
> 
> > +                if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> > +                    break;
> 
> It should reach this break for the same reason we are in this
> code path due check on (2)
> 

All these are not changed, I just factored out a function.
Yes, can be surely be optimized or changed.

> > +                }
> > +                /* The GStreamer pipeline dropped the corresponding
> > +                 * buffer.
> > +                 */
> > +                num_frames_dropped++;
> > +                free_gst_frame(gstframe);
> > +            }
> > +            break;
> 
> Considering that gstframe might have changed after entering in
> (2), we are breaking the while (l) loop with dangling pointer and
> returning that.
> 

No, see the "queues_mutex must be held" comment.

> > +        }
> > +        gstframe = NULL;
> > +        l = l->next;
> > +    }
> > +    if (num_frames_dropped != 0) {
> > +        SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
> > num_frames_dropped);
> > +    }
> > +    return gstframe;
> > +}
> > +
> >  static void fetch_pending_sample(SpiceGstDecoder *decoder)
> >  {
> >      GstSample *sample = gst_app_sink_pull_sample(decoder->appsink);
> > @@ -212,7 +259,6 @@ static void fetch_pending_sample(SpiceGstDecoder
> > *decoder)
> >          decoder->pending_samples--;
> >  
> >          GstBuffer *buffer = gst_sample_get_buffer(sample);
> > -        guint num_frames_dropped = 0;
> >  
> >          /* gst_app_sink_pull_sample() sometimes returns the same buffer
> >          twice
> >           * or buffers that have a modified, and thus unrecognizable, PTS.
> > @@ -221,36 +267,12 @@ static void fetch_pending_sample(SpiceGstDecoder
> > *decoder)
> >           * finding a match either, etc. So check the buffer has a matching
> >           * frame first.
> >           */
> > -        SpiceGstFrame *gstframe;
> > -        GList *l = g_queue_peek_head_link(decoder->decoding_queue);
> 
> You did not introduced what I consider to be my confusion (as this
> code seems to be working fine).
> 
> > -        while (l) {
> > -            gstframe = l->data;
> > -            if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> > -                /* The frame is now ready for display */
> > -                gstframe->decoded_sample = sample;
> > -                decoder->display_frame = gstframe;
> > -
> > -                /* Now that we know there is a match, remove it and the
> > older
> > -                 * frames from the decoding queue.
> > -                 */
> > -                while ((gstframe =
> > g_queue_pop_head(decoder->decoding_queue))) {
> > -                    if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> > -                        break;
> > -                    }
> 
> Same
> 
> > -                    /* The GStreamer pipeline dropped the corresponding
> > -                     * buffer.
> > -                     */
> > -                    num_frames_dropped++;
> > -                    free_gst_frame(gstframe);
> > -                }
> > -                break;
> > -            }
> > -            l = l->next;
> > -        }
> > -        if (num_frames_dropped != 0) {
> > -            SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
> > num_frames_dropped);
> > -        }
> > -        if (!l) {
> > +        SpiceGstFrame *gstframe = get_decoded_frame(decoder, buffer);
> > +        if (gstframe) {
> > +            /* The frame is now ready for display */
> > +            gstframe->decoded_sample = sample;
> > +            decoder->display_frame = gstframe;
> > +        } else {
> 
> Cheers,
> Victor
>