> > 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@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) > { Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel