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 <[email protected]> > --- > 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)
> + }
> + /* 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.
> + }
> + 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
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/spice-devel
