[spice-gtk,v1,05/11] channel-display: add spice_frame_free() helper

Submitted by Victor Toso on March 13, 2018, 11:25 a.m.

Details

Message ID 20180313112542.32238-6-victortoso@redhat.com
State New
Headers show
Series "misc improvements in channel-display" ( rev: 2 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso March 13, 2018, 11:25 a.m.
From: Victor Toso <me@victortoso.com>

The SpiceFrame is created in channel-display.c but it is currently
freed at each decoders' end. A helper function can reduce some code
and makes it easier to check if the function is called, what time was
called, etc.

In channel-display-mjpeg.c this means removing free_spice_frame()
function;

In channel-display-gst.c, SpiceFrame is under SpiceGstFrame so we just
need to be careful to call spice_frame_free() once by removing the
unref function parameter to gst_buffer_new_wrapped_full();

Note that I'm using g_clear_pointer() everywhere that makes sense
(checking for NULL before calling free and setting pointer to NULL
afterwards)

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/channel-display-gst.c   |  8 +++-----
 src/channel-display-mjpeg.c | 21 ++++-----------------
 src/channel-display-priv.h  |  1 +
 src/channel-display.c       | 11 +++++++++++
 4 files changed, 19 insertions(+), 22 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 8b23036..d2847ec 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -91,10 +91,8 @@  static SpiceGstFrame *create_gst_frame(GstBuffer *buffer, SpiceFrame *frame)
 
 static void free_gst_frame(SpiceGstFrame *gstframe)
 {
-    gstframe->frame->free(gstframe->frame);
-    if (gstframe->sample) {
-        gst_sample_unref(gstframe->sample);
-    }
+    g_clear_pointer(&gstframe->frame, spice_frame_free);
+    g_clear_pointer(&gstframe->sample, gst_sample_unref);
     g_free(gstframe);
 }
 
@@ -553,7 +551,7 @@  static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
     frame->ref_data(frame->data_opaque);
     GstBuffer *buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
                                                     frame->data, frame->size, 0, frame->size,
-                                                    frame->data_opaque, frame->unref_data);
+                                                    frame->data_opaque, NULL);
 
     GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
     GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index f0d55f6..66a6eb8 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -75,15 +75,6 @@  static void mjpeg_src_term(struct jpeg_decompress_struct *cinfo)
 }
 
 
-/* ---------- A SpiceFrame helper ---------- */
-
-static void free_spice_frame(SpiceFrame *frame)
-{
-    frame->unref_data(frame->data_opaque);
-    frame->free(frame);
-}
-
-
 /* ---------- Decoder proper ---------- */
 
 static void mjpeg_decoder_schedule(MJpegDecoder *decoder);
@@ -177,8 +168,7 @@  static gboolean mjpeg_decoder_decode_frame(gpointer video_decoder)
     /* Display the frame and dispose of it */
     stream_display_frame(decoder->base.stream, decoder->cur_frame,
                          width, height, SPICE_UNKNOWN_STRIDE, decoder->out_frame);
-    free_spice_frame(decoder->cur_frame);
-    decoder->cur_frame = NULL;
+    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
     decoder->timer_id = 0;
 
     /* Schedule the next frame */
@@ -212,7 +202,7 @@  static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
                         __FUNCTION__, time - frame->mm_time,
                         frame->mm_time, time);
             stream_dropped_frame_on_playback(decoder->base.stream);
-            free_spice_frame(frame);
+            spice_frame_free(frame);
         }
         frame = g_queue_pop_head(decoder->msgq);
     } while (frame);
@@ -222,7 +212,7 @@  static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
 /* mjpeg_decoder_drop_queue() helper */
 static void _msg_in_unref_func(gpointer data, gpointer user_data)
 {
-    free_spice_frame((SpiceFrame*)data);
+    spice_frame_free(data);
 }
 
 static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
@@ -231,10 +221,7 @@  static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
         g_source_remove(decoder->timer_id);
         decoder->timer_id = 0;
     }
-    if (decoder->cur_frame) {
-        free_spice_frame(decoder->cur_frame);
-        decoder->cur_frame = NULL;
-    }
+    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
     g_queue_foreach(decoder->msgq, _msg_in_unref_func, NULL);
     g_queue_clear(decoder->msgq);
 }
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 6a90a78..95ad7d8 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -196,6 +196,7 @@  void stream_dropped_frame_on_playback(display_stream *st);
 #define SPICE_UNKNOWN_STRIDE 0
 void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t width, uint32_t height, int stride, uint8_t* data);
 
+void spice_frame_free(SpiceFrame *frame);
 
 G_END_DECLS
 
diff --git a/src/channel-display.c b/src/channel-display.c
index ec94cf8..a134516 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1485,6 +1485,17 @@  static void display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
 
 #define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
 
+G_GNUC_INTERNAL
+void spice_frame_free(SpiceFrame *frame)
+{
+    if (frame == NULL) {
+        return;
+    }
+
+    frame->unref_data(frame->data_opaque);
+    frame->free(frame);
+}
+
 /* coroutine context */
 static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn *in)
 {

Comments

> 
> From: Victor Toso <me@victortoso.com>
> 
> The SpiceFrame is created in channel-display.c but it is currently
> freed at each decoders' end. A helper function can reduce some code
> and makes it easier to check if the function is called, what time was
> called, etc.
> 
> In channel-display-mjpeg.c this means removing free_spice_frame()
> function;
> 
> In channel-display-gst.c, SpiceFrame is under SpiceGstFrame so we just
> need to be careful to call spice_frame_free() once by removing the
> unref function parameter to gst_buffer_new_wrapped_full();
> 
> Note that I'm using g_clear_pointer() everywhere that makes sense
> (checking for NULL before calling free and setting pointer to NULL
> afterwards)
> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>

Reading this patch I realize how this structure is weird and that
due to it you are introducing a bug.

The problem with SpiceFrame is that the "data" (so data, data_size and
data_opaque fields) are valid until in VideoDecoder::queue_frame OR
SpiceFrame::ref_data is called. The ownership of SpiceFrame is passed
to VideoDecoder::queue_frame. Usually in VideoDecoder::queue_frame
the pointer is retained but as soon of VideoDecoder::queue_frame exit
if you didn't call SpiceFrame::ref_data all the above mentioned fields
became invalid (dangling pointers!).
That's the reason why SpiceFrame::free is just a g_free and not free
the data message, because SpiceFrame does not own the data (but have
a pointer to it!).
I think would be a good way to have SpiceFrame::free to unreference
data and obviously to own it (in spice_frame_new).

> ---
>  src/channel-display-gst.c   |  8 +++-----
>  src/channel-display-mjpeg.c | 21 ++++-----------------
>  src/channel-display-priv.h  |  1 +
>  src/channel-display.c       | 11 +++++++++++
>  4 files changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 8b23036..d2847ec 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -91,10 +91,8 @@ static SpiceGstFrame *create_gst_frame(GstBuffer *buffer,
> SpiceFrame *frame)
>  
>  static void free_gst_frame(SpiceGstFrame *gstframe)
>  {
> -    gstframe->frame->free(gstframe->frame);
> -    if (gstframe->sample) {
> -        gst_sample_unref(gstframe->sample);
> -    }
> +    g_clear_pointer(&gstframe->frame, spice_frame_free);

Note that previously you didn't unreference the data.

> +    g_clear_pointer(&gstframe->sample, gst_sample_unref);
>      g_free(gstframe);
>  }
>  
> @@ -553,7 +551,7 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>      frame->ref_data(frame->data_opaque);
>      GstBuffer *buffer =
>      gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
>                                                      frame->data,
>                                                      frame->size, 0,
>                                                      frame->size,
> -                                                    frame->data_opaque,
> frame->unref_data);
> +                                                    frame->data_opaque,
> NULL);

Bug: you are retaining a pointer without ownership, looking for trouble!

>  
>      GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
>      GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index f0d55f6..66a6eb8 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -75,15 +75,6 @@ static void mjpeg_src_term(struct jpeg_decompress_struct
> *cinfo)
>  }
>  
>  
> -/* ---------- A SpiceFrame helper ---------- */
> -
> -static void free_spice_frame(SpiceFrame *frame)
> -{
> -    frame->unref_data(frame->data_opaque);
> -    frame->free(frame);
> -}
> -

This was correct as the code added the ownership calling ref_data, in previous
file this was done differently.

> -
>  /* ---------- Decoder proper ---------- */
>  
>  static void mjpeg_decoder_schedule(MJpegDecoder *decoder);
> @@ -177,8 +168,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
>      /* Display the frame and dispose of it */
>      stream_display_frame(decoder->base.stream, decoder->cur_frame,
>                           width, height, SPICE_UNKNOWN_STRIDE,
>                           decoder->out_frame);
> -    free_spice_frame(decoder->cur_frame);
> -    decoder->cur_frame = NULL;
> +    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
>      decoder->timer_id = 0;
>  
>      /* Schedule the next frame */
> @@ -212,7 +202,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
>                          __FUNCTION__, time - frame->mm_time,
>                          frame->mm_time, time);
>              stream_dropped_frame_on_playback(decoder->base.stream);
> -            free_spice_frame(frame);
> +            spice_frame_free(frame);
>          }
>          frame = g_queue_pop_head(decoder->msgq);
>      } while (frame);
> @@ -222,7 +212,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
>  /* mjpeg_decoder_drop_queue() helper */
>  static void _msg_in_unref_func(gpointer data, gpointer user_data)
>  {
> -    free_spice_frame((SpiceFrame*)data);
> +    spice_frame_free(data);
>  }
>  
>  static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
> @@ -231,10 +221,7 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder
> *decoder)
>          g_source_remove(decoder->timer_id);
>          decoder->timer_id = 0;
>      }
> -    if (decoder->cur_frame) {
> -        free_spice_frame(decoder->cur_frame);
> -        decoder->cur_frame = NULL;
> -    }
> +    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
>      g_queue_foreach(decoder->msgq, _msg_in_unref_func, NULL);
>      g_queue_clear(decoder->msgq);
>  }
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 6a90a78..95ad7d8 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -196,6 +196,7 @@ void stream_dropped_frame_on_playback(display_stream
> *st);
>  #define SPICE_UNKNOWN_STRIDE 0
>  void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
>  width, uint32_t height, int stride, uint8_t* data);
>  
> +void spice_frame_free(SpiceFrame *frame);
>  
>  G_END_DECLS
>  
> diff --git a/src/channel-display.c b/src/channel-display.c
> index ec94cf8..a134516 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1485,6 +1485,17 @@ static void
> display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
>  
>  #define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
>  
> +G_GNUC_INTERNAL
> +void spice_frame_free(SpiceFrame *frame)
> +{
> +    if (frame == NULL) {
> +        return;
> +    }

not much sure about.

> +
> +    frame->unref_data(frame->data_opaque);
> +    frame->free(frame);
> +}
> +
>  /* coroutine context */
>  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn
>  *in)
>  {

Oh... note that with gstreamer code the spice_msg_in_unref is potentially
called from a different thread leading to possible race conditions as
the operation is not atomic. Maybe would be safer (even if I don't like
the cost for just not much occurrences) to change in an atomic operation.

Frediano
Hi,

Many thanks for the reviews.

On Tue, Apr 17, 2018 at 07:08:13AM -0400, Frediano Ziglio wrote:
> > 
> > From: Victor Toso <me@victortoso.com>
> > 
> > The SpiceFrame is created in channel-display.c but it is currently
> > freed at each decoders' end. A helper function can reduce some code
> > and makes it easier to check if the function is called, what time was
> > called, etc.
> > 
> > In channel-display-mjpeg.c this means removing free_spice_frame()
> > function;
> > 
> > In channel-display-gst.c, SpiceFrame is under SpiceGstFrame so we just
> > need to be careful to call spice_frame_free() once by removing the
> > unref function parameter to gst_buffer_new_wrapped_full();
> > 
> > Note that I'm using g_clear_pointer() everywhere that makes sense
> > (checking for NULL before calling free and setting pointer to NULL
> > afterwards)
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> 
> Reading this patch I realize how this structure is weird and that
> due to it you are introducing a bug.
> 
> The problem with SpiceFrame is that the "data" (so data, data_size and
> data_opaque fields) are valid until in VideoDecoder::queue_frame OR
> SpiceFrame::ref_data is called. The ownership of SpiceFrame is passed
> to VideoDecoder::queue_frame. Usually in VideoDecoder::queue_frame
> the pointer is retained but as soon of VideoDecoder::queue_frame exit
> if you didn't call SpiceFrame::ref_data all the above mentioned fields
> became invalid (dangling pointers!).
> That's the reason why SpiceFrame::free is just a g_free and not free
> the data message, because SpiceFrame does not own the data (but have
> a pointer to it!).
> I think would be a good way to have SpiceFrame::free to unreference
> data and obviously to own it (in spice_frame_new).

I agree, many thanks for this analyses is indeed quite important.
I'll change it for the next version.

> 
> > ---
> >  src/channel-display-gst.c   |  8 +++-----
> >  src/channel-display-mjpeg.c | 21 ++++-----------------
> >  src/channel-display-priv.h  |  1 +
> >  src/channel-display.c       | 11 +++++++++++
> >  4 files changed, 19 insertions(+), 22 deletions(-)
> > 
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 8b23036..d2847ec 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -91,10 +91,8 @@ static SpiceGstFrame *create_gst_frame(GstBuffer *buffer,
> > SpiceFrame *frame)
> >  
> >  static void free_gst_frame(SpiceGstFrame *gstframe)
> >  {
> > -    gstframe->frame->free(gstframe->frame);
> > -    if (gstframe->sample) {
> > -        gst_sample_unref(gstframe->sample);
> > -    }
> > +    g_clear_pointer(&gstframe->frame, spice_frame_free);
> 
> Note that previously you didn't unreference the data.
> 
> > +    g_clear_pointer(&gstframe->sample, gst_sample_unref);
> >      g_free(gstframe);
> >  }
> >  
> > @@ -553,7 +551,7 @@ static gboolean
> > spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
> >      frame->ref_data(frame->data_opaque);
> >      GstBuffer *buffer =
> >      gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
> >                                                      frame->data,
> >                                                      frame->size, 0,
> >                                                      frame->size,
> > -                                                    frame->data_opaque,
> > frame->unref_data);
> > +                                                    frame->data_opaque,
> > NULL);
> 
> Bug: you are retaining a pointer without ownership, looking for trouble!
> 
> >  
> >      GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
> >      GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> > diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> > index f0d55f6..66a6eb8 100644
> > --- a/src/channel-display-mjpeg.c
> > +++ b/src/channel-display-mjpeg.c
> > @@ -75,15 +75,6 @@ static void mjpeg_src_term(struct jpeg_decompress_struct
> > *cinfo)
> >  }
> >  
> >  
> > -/* ---------- A SpiceFrame helper ---------- */
> > -
> > -static void free_spice_frame(SpiceFrame *frame)
> > -{
> > -    frame->unref_data(frame->data_opaque);
> > -    frame->free(frame);
> > -}
> > -
> 
> This was correct as the code added the ownership calling ref_data, in previous
> file this was done differently.
> 
> > -
> >  /* ---------- Decoder proper ---------- */
> >  
> >  static void mjpeg_decoder_schedule(MJpegDecoder *decoder);
> > @@ -177,8 +168,7 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> > video_decoder)
> >      /* Display the frame and dispose of it */
> >      stream_display_frame(decoder->base.stream, decoder->cur_frame,
> >                           width, height, SPICE_UNKNOWN_STRIDE,
> >                           decoder->out_frame);
> > -    free_spice_frame(decoder->cur_frame);
> > -    decoder->cur_frame = NULL;
> > +    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
> >      decoder->timer_id = 0;
> >  
> >      /* Schedule the next frame */
> > @@ -212,7 +202,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
> >                          __FUNCTION__, time - frame->mm_time,
> >                          frame->mm_time, time);
> >              stream_dropped_frame_on_playback(decoder->base.stream);
> > -            free_spice_frame(frame);
> > +            spice_frame_free(frame);
> >          }
> >          frame = g_queue_pop_head(decoder->msgq);
> >      } while (frame);
> > @@ -222,7 +212,7 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
> >  /* mjpeg_decoder_drop_queue() helper */
> >  static void _msg_in_unref_func(gpointer data, gpointer user_data)
> >  {
> > -    free_spice_frame((SpiceFrame*)data);
> > +    spice_frame_free(data);
> >  }
> >  
> >  static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
> > @@ -231,10 +221,7 @@ static void mjpeg_decoder_drop_queue(MJpegDecoder
> > *decoder)
> >          g_source_remove(decoder->timer_id);
> >          decoder->timer_id = 0;
> >      }
> > -    if (decoder->cur_frame) {
> > -        free_spice_frame(decoder->cur_frame);
> > -        decoder->cur_frame = NULL;
> > -    }
> > +    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
> >      g_queue_foreach(decoder->msgq, _msg_in_unref_func, NULL);
> >      g_queue_clear(decoder->msgq);
> >  }
> > diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> > index 6a90a78..95ad7d8 100644
> > --- a/src/channel-display-priv.h
> > +++ b/src/channel-display-priv.h
> > @@ -196,6 +196,7 @@ void stream_dropped_frame_on_playback(display_stream
> > *st);
> >  #define SPICE_UNKNOWN_STRIDE 0
> >  void stream_display_frame(display_stream *st, SpiceFrame *frame, uint32_t
> >  width, uint32_t height, int stride, uint8_t* data);
> >  
> > +void spice_frame_free(SpiceFrame *frame);
> >  
> >  G_END_DECLS
> >  
> > diff --git a/src/channel-display.c b/src/channel-display.c
> > index ec94cf8..a134516 100644
> > --- a/src/channel-display.c
> > +++ b/src/channel-display.c
> > @@ -1485,6 +1485,17 @@ static void
> > display_session_mm_time_reset_cb(SpiceSession *session, gpointer dat
> >  
> >  #define STREAM_PLAYBACK_SYNC_DROP_SEQ_LEN_LIMIT 5
> >  
> > +G_GNUC_INTERNAL
> > +void spice_frame_free(SpiceFrame *frame)
> > +{
> > +    if (frame == NULL) {
> > +        return;
> > +    }
> 
> not much sure about.

Might make sense to be a warning instead of silent return.

> 
> > +
> > +    frame->unref_data(frame->data_opaque);
> > +    frame->free(frame);
> > +}
> > +
> >  /* coroutine context */
> >  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn
> >  *in)
> >  {
> 
> Oh... note that with gstreamer code the spice_msg_in_unref is potentially
> called from a different thread leading to possible race conditions as
> the operation is not atomic. Maybe would be safer (even if I don't like
> the cost for just not much occurrences) to change in an atomic operation.

Really good catch. Let's discuss it on your proposal patch.

Cheers,

> 
> Frediano