>
> 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@xxxxxxxxxx>
> > ---
> > 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)
>
All these are not changed, I just factored out a function.
Yes, can be surely be optimized or changed.
> > + }
> > + /* 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.
>
No, see the "queues_mutex must be held" comment.
> > + }
> > + 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
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel