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