[Spice-devel] [spice-gtk v1 1/2] display-gst: small refactor get_decoded_frame()

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> Small refactor to make each code block a bit more obvious.
> 
> This code should (1) find the @buffer in the queue; (2) remove all old
> elements from queue. That perfectly fit in two loops in sequence, but
> they don't need to be nested and they don't need to use the same
> pointer (gstframe).
> 
> As @num_frames_dropped is only used in the second block, this was
> moved as well, together with the debug.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> ---
>  src/channel-display-gst.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 08c9f8f..2b42053 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -212,8 +212,6 @@ static void schedule_frame(SpiceGstDecoder *decoder)
>   */
>  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
> @@ -226,27 +224,30 @@ static SpiceGstFrame *get_decoded_frame(SpiceGstDecoder
> *decoder, GstBuffer *buf
>      while (l) {

I wonder if this would be more readable with a

      for (;;) {
          if (l == NULL) {
              return NULL;
          }

saving all the NULL checks later

>          gstframe = l->data;
>          if (gstframe->timestamp == GST_BUFFER_PTS(buffer)) {
> -
> -            /* 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;
> -                }
> -                /* The GStreamer pipeline dropped the corresponding
> -                 * buffer.
> -                 */
> -                num_frames_dropped++;
> -                free_gst_frame(gstframe);
> -            }
>              break;
>          }
>          gstframe = NULL;
>          l = l->next;
>      }
> -    if (num_frames_dropped != 0) {
> -        SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
> num_frames_dropped);
> +
> +    /* Now that we know there is a match, remove it and the older
> +     * frames from the decoding queue.
> +     */
> +    if (gstframe != NULL) {
> +        guint num_frames_dropped = 0;
> +        SpiceGstFrame *late_frame = NULL;
> +        /* The GStreamer pipeline dropped the corresponding
> +         * buffer.
> +         */
> +        while ((late_frame = g_queue_pop_head(decoder->decoding_queue)) !=
> NULL &&
> +               late_frame->timestamp > GST_BUFFER_PTS(buffer)) {

This can simply be

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

> +            num_frames_dropped++;
> +            free_gst_frame(late_frame);
> +        }
> +
> +        if (num_frames_dropped != 0) {
> +            SPICE_DEBUG("the GStreamer pipeline dropped %u frames",
> num_frames_dropped);
> +        }
>      }
>      return gstframe;
>  }

Frediano