[Spice-devel] [PATCH spice-gtk] channel-display: add spice_frame_free() helper

Frediano Ziglio fziglio at redhat.com
Mon Jan 21 16:46:02 UTC 2019


> 
> Heavily based on a former patch from Victor Toso removing some issues
> (https://lists.freedesktop.org/archives/spice-devel/2018-April/043168.html)
> 
> 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();
> 

I forget to remove this sentence, now not more valid.
Beside that the patch is fine.

> The patch is using g_clear_pointer() everywhere that makes sense
> (checking for NULL before calling free and setting pointer to NULL
> afterwards)
> 
> The ownedship management is more clear:
> - SpiceFrame owns frame data (as it has a pointer to it);
> - spice_frame_free releases frame data;
> - SpiceFrame interface is simplified;
> - GstBuffer owns SpiceFrame (not only frame data);
> - SpiceGstFrame owns GstBuffer.
> 
> Signed-off-by: Victor Toso <victortoso at redhat.com>
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  src/channel-display-gst.c   | 25 +++++++++++--------------
>  src/channel-display-mjpeg.c | 28 +++++++---------------------
>  src/channel-display-priv.h  |  5 +----
>  src/channel-display.c       | 15 ++++++++++++---
>  4 files changed, 31 insertions(+), 42 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 0b871a71..a6ad4f1c 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -79,6 +79,7 @@ typedef enum {
>  
>  struct SpiceGstFrame {
>      GstClockTime timestamp;
> +    GstBuffer *buffer;
>      SpiceFrame *frame;
>      GstSample *sample;
>  };
> @@ -87,6 +88,7 @@ static SpiceGstFrame *create_gst_frame(GstBuffer *buffer,
> SpiceFrame *frame)
>  {
>      SpiceGstFrame *gstframe = g_new(SpiceGstFrame, 1);
>      gstframe->timestamp = GST_BUFFER_PTS(buffer);
> +    gstframe->buffer = gst_buffer_ref(buffer);
>      gstframe->frame = frame;
>      gstframe->sample = NULL;
>      return gstframe;
> @@ -94,10 +96,9 @@ 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);
> -    }
> +    gst_buffer_unref(gstframe->buffer);
> +    // frame was owned by the buffer, don't release it
> +    g_clear_pointer(&gstframe->sample, gst_sample_unref);
>      g_free(gstframe);
>  }
>  
> @@ -590,7 +591,7 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>  
>      if (frame->size == 0) {
>          SPICE_DEBUG("got an empty frame buffer!");
> -        frame->free(frame);
> +        spice_frame_free(frame);
>          return TRUE;
>      }
>  
> @@ -608,14 +609,14 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>           * saves CPU so do it.
>           */
>          SPICE_DEBUG("dropping a late MJPEG frame");
> -        frame->free(frame);
> +        spice_frame_free(frame);
>          return TRUE;
>      }
>  
>      if (decoder->pipeline == NULL) {
>          /* An error occurred, causing the GStreamer pipeline to be freed */
>          spice_warning("An error occurred, stopping the video stream");
> -        frame->free(frame);
> +        spice_frame_free(frame);
>          return FALSE;
>      }
>  
> @@ -623,16 +624,15 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>      if (decoder->appsrc == NULL) {
>          spice_warning("Error: Playbin has not yet initialized the Appsrc
>          element");
>          stream_dropped_frame_on_playback(decoder->base.stream);
> -        frame->free(frame);
> +        spice_frame_free(frame);
>          return TRUE;
>      }
>  #endif
>  
> -    /* ref() the frame data for the buffer */
> -    frame->ref_data(frame->data_opaque);
> +    /* frame ownership is moved to the buffer */
>      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, (GDestroyNotify)
> spice_frame_free);
>  
>      GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
>      GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
> @@ -643,9 +643,6 @@ static gboolean
> spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>          g_mutex_lock(&decoder->queues_mutex);
>          g_queue_push_tail(decoder->decoding_queue, gst_frame);
>          g_mutex_unlock(&decoder->queues_mutex);
> -    } else {
> -        frame->free(frame);
> -        frame = NULL;
>      }
>  
>      if (gst_app_src_push_buffer(decoder->appsrc, buffer) != GST_FLOW_OK) {
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index e79fd865..ba7f266e 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);
> @@ -168,8 +159,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 */
> @@ -202,7 +192,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);
> @@ -210,9 +200,9 @@ static void mjpeg_decoder_schedule(MJpegDecoder *decoder)
>  
>  
>  /* mjpeg_decoder_drop_queue() helper */
> -static void _msg_in_unref_func(gpointer data, gpointer user_data)
> +static void spice_frame_unref_func(gpointer data, gpointer user_data)
>  {
> -    free_spice_frame((SpiceFrame*)data);
> +    spice_frame_free(data);
>  }
>  
>  static void mjpeg_decoder_drop_queue(MJpegDecoder *decoder)
> @@ -221,11 +211,8 @@ 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_queue_foreach(decoder->msgq, _msg_in_unref_func, NULL);
> +    g_clear_pointer(&decoder->cur_frame, spice_frame_free);
> +    g_queue_foreach(decoder->msgq, spice_frame_unref_func, NULL);
>      g_queue_clear(decoder->msgq);
>  }
>  
> @@ -254,11 +241,10 @@ static gboolean mjpeg_decoder_queue_frame(VideoDecoder
> *video_decoder,
>       */
>      if (latency < 0) {
>          SPICE_DEBUG("dropping a late MJPEG frame");
> -        frame->free(frame);
> +        spice_frame_free(frame);
>          return TRUE;
>      }
>  
> -    frame->ref_data(frame->data_opaque);
>      g_queue_push_tail(decoder->msgq, frame);
>      mjpeg_decoder_schedule(decoder);
>      return TRUE;
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 6c6b6c40..c5c3f5c8 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -45,10 +45,6 @@ struct SpiceFrame {
>      uint8_t *data;
>      uint32_t size;
>      gpointer data_opaque;
> -    void (*ref_data)(gpointer data_opaque);
> -    void (*unref_data)(gpointer data_opaque);
> -
> -    void (*free)(SpiceFrame *frame);
>  };
>  
>  typedef struct VideoDecoder VideoDecoder;
> @@ -201,6 +197,7 @@ void stream_display_frame(display_stream *st, SpiceFrame
> *frame, uint32_t width,
>  guintptr get_window_handle(display_stream *st);
>  gboolean hand_pipeline_to_widget(display_stream *st,  GstPipeline
>  *pipeline);
>  
> +void spice_frame_free(SpiceFrame *frame);
>  
>  G_END_DECLS
>  
> diff --git a/src/channel-display.c b/src/channel-display.c
> index ae949eb0..ae2c4fbc 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1656,12 +1656,21 @@ static SpiceFrame *spice_frame_new(display_stream
> *st,
>      frame->data = data_ptr;
>      frame->size = data_size;
>      frame->data_opaque = in;
> -    frame->ref_data = (void*)spice_msg_in_ref;
> -    frame->unref_data = (void*)spice_msg_in_unref;
> -    frame->free = (void*)g_free;
> +    spice_msg_in_ref(in);
>      return frame;
>  }
>  
> +G_GNUC_INTERNAL
> +void spice_frame_free(SpiceFrame *frame)
> +{
> +    if (frame == NULL) {
> +        return;
> +    }
> +
> +    spice_msg_in_unref(frame->data_opaque);
> +    g_free(frame);
> +}
> +
>  /* coroutine context */
>  static void display_handle_stream_data(SpiceChannel *channel, SpiceMsgIn
>  *in)
>  {

Frediano


More information about the Spice-devel mailing list