[Spice-commits] src/channel-display.c src/channel-display-gst.c src/channel-display-mjpeg.c src/channel-display-priv.h

[GitLab Mirror]

 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(-)

New commits:
commit 5f683e7686882835e6721b053a5676f4b1996a88
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Jan 21 16:29:30 2019 +0000

    channel-display: add spice_frame_free() helper
    
    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 it
    was called, etc.
    
    In channel-display-mjpeg.c this means removing free_spice_frame()
    function.
    
    The patch is using g_clear_pointer() everywhere that makes sense
    (checking for NULL before calling free and setting pointer to NULL
    afterwards)
    
    The ownership 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>
    Acked-by: Christophe Fergeau <cfergeau at redhat.com>

diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index 0b871a7..a6ad4f1 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 e79fd86..ba7f266 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 6c6b6c4..c5c3f5c 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 ae949eb..ae2c4fb 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)
 {