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 <victort...@redhat.com>
Signed-off-by: Frediano Ziglio <fzig...@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)
{
--
2.20.1
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
