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(); 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@xxxxxxxxxx> Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> --- 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) { -- 2.20.1 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel