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

[Victor Toso]

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)

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

> +        }
> +        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/0360a21d/attachment.sig>