Hi,
On Wed, Jan 23, 2019 at 08:11:17AM -0500, Frediano Ziglio wrote:
> >
> > 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.
Yes, it is preferable to do it before or in a follow up. Just
arguing about something that I haven't noticed before.
> > > + }
> > > + /* 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.
Not following about queues_mutex() but I see now my mistake in
the comment above. The last iteration of
while ((gstframe = g_queue_pop_head(decoder->decoding_queue))) {
Will set gstframe to NULL, so, no dangling pointer indeed.
Acked-by: Victor Toso <victortoso@xxxxxxxxxx>
> > > + }
> > > + 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
> >
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel