Hi, On Wed, Jan 23, 2019 at 04:45:23PM -0500, Frediano Ziglio wrote: > > > > From: Victor Toso <[email protected]> > > > > 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 <[email protected]> > > --- > > 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
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
