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

[Victor Toso]

Hi,

On Wed, Jan 23, 2019 at 04:45:23PM -0500, Frediano Ziglio wrote:
> > 
> > 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;
>           }

I have this kinda of thing sealed in my brain due to the proposed
use of _for_ and _while_. So, _for_ in general, is to interact for a
well defined range of values while _while_ is to the situation
where we don't know exactly how much iterations it will take.

So, what you proposed makes sense but every time that I see, this
kind of things comes to mind.

> saving all the NULL checks later

Just one, the gstframe != NULL, right?

> 
> >          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) {

True, thanks!
I'll resend later today.

Cheers,

> 
> > +            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
-------------- 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/20190124/28ba4c5b/attachment-0001.sig>