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

Victor Toso victortoso at redhat.com
Wed Jan 23 13:32:55 UTC 2019


Hi,

On Wed, Jan 23, 2019 at 08:11:17AM -0500, Frediano Ziglio wrote:
> > 
> > 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.

Yes, it is preferable to do it before or in a follow up. Just
arguing about something that I haven't noticed before.

> > > +                }
> > > +                /* 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.

Not following about queues_mutex() but I see now my mistake in
the comment above. The last iteration of

    while ((gstframe = g_queue_pop_head(decoder->decoding_queue))) {

Will set gstframe to NULL, so, no dangling pointer indeed.

Acked-by: Victor Toso <victortoso at redhat.com>

> > > +        }
> > > +        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
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190123/f5916559/attachment-0001.sig>


More information about the Spice-devel mailing list