[Spice-devel] [client 2/3] streaming: Remove the video decoder's dependency on SpiceMsgIn messages

[Christophe Fergeau]

On Tue, Apr 04, 2017 at 05:45:35PM +0200, Francois Gouget wrote:
> Code-wise this improves the separation between networking and the video
> decoding.
> It also makes it easier to reuse the latter should the client one day
> receive video streams through other messages.
> 
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  src/channel-display-gst.c   | 128 ++++++++++++++++++++------------------------
>  src/channel-display-mjpeg.c |  74 +++++++++++++------------
>  src/channel-display-priv.h  |  24 +++++++--
>  src/channel-display.c       |  35 ++++++------
>  4 files changed, 134 insertions(+), 127 deletions(-)
> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index c4190b2d..93b61bab 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -86,31 +86,30 @@ G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) <= SPICE_VIDEO_CODEC_TYPE_ENUM_END);
>  #define VALID_VIDEO_CODEC_TYPE(codec) \
>      (codec > 0 && codec < G_N_ELEMENTS(gst_opts))
>  
> -/* ---------- SpiceFrame ---------- */
> +/* ---------- SpiceMetaFrame ---------- */
>  
> -typedef struct _SpiceFrame {
> -    GstClockTime timestamp;
> -    SpiceMsgIn *msg;
> -    GstSample *sample;
> -} SpiceFrame;
> +typedef struct SpiceMetaFrame {
> +    SpiceFrame *frame;
> +    GstClockTime pts;
> +    void *sample;

Why not GstSample *sample?

> +} SpiceMetaFrame;

SpiceGstFrame rather than SpiceMetaFrame?

> @@ -455,22 +445,22 @@ static gboolean spice_gst_decoder_queue_frame(VideoDecoder *video_decoder,
>          return FALSE;
>      }
>  
> -    /* ref() the frame_msg for the buffer */
> -    spice_msg_in_ref(frame_msg);
> +    /* ref() the frame data for the buffer */
> +    frame->ref_data(frame->data_opaque);
>      GstBuffer *buffer = gst_buffer_new_wrapped_full(GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS,
> -                                                    data, size, 0, size,
> -                                                    frame_msg, &release_buffer_data);
> +                                                    frame->data, frame->size, 0, frame->size,
> +                                                    frame->data_opaque, frame->unref_data);
>  
>      GST_BUFFER_DURATION(buffer) = GST_CLOCK_TIME_NONE;
>      GST_BUFFER_DTS(buffer) = GST_CLOCK_TIME_NONE;
>      GST_BUFFER_PTS(buffer) = gst_clock_get_time(decoder->clock) - gst_element_get_base_time(decoder->pipeline) + ((uint64_t)MAX(0, latency)) * 1000 * 1000;
>  
>      g_mutex_lock(&decoder->queues_mutex);
> -    g_queue_push_tail(decoder->decoding_queue, create_frame(buffer, frame_msg));
> +    g_queue_push_tail(decoder->decoding_queue, create_meta_frame(frame, buffer));

I believe this is my main grip with this patch, the somehow disjoint
SpiceFrame::free and SpiceFrame::unref_data which needs to get called at
different times, sounds like something we'll get wrong in the future.
I'm wondering if passing a SpiceGstFrame (aka SpiceMetaFrame) as the
last arg to gst_buffer_new_wrapped_full() would help here by grouping
the time we unref the message, and the time we free the SpiceFrame.

Or does the SpiceGstFrame need to outlive this buffer? (lifetime of
everything involved is not clear, the SpiceGstFrame moves from
decoding_queue to display_queue, and is mostly needed for display_queue
consumers, I did not check how long the GstBuffer created here stays
alive compared to a buffer sitting in these queues).

> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 722494ee..85e8487f 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -38,7 +38,7 @@ typedef struct MJpegDecoder {
>      /* ---------- Frame queue ---------- */
>  
>      GQueue *msgq;
> -    SpiceMsgIn *cur_frame_msg;
> +    SpiceFrame *cur_frame;
>      guint timer_id;
>  
>      /* ---------- Output frame data ---------- */
> @@ -53,10 +53,8 @@ typedef struct MJpegDecoder {
>  static void mjpeg_src_init(struct jpeg_decompress_struct *cinfo)
>  {
>      MJpegDecoder *decoder = SPICE_CONTAINEROF(cinfo->src, MJpegDecoder, mjpeg_src);
> -
> -    uint8_t *data;
> -    cinfo->src->bytes_in_buffer = spice_msg_in_frame_data(decoder->cur_frame_msg, &data);
> -    cinfo->src->next_input_byte = data;
> +    cinfo->src->bytes_in_buffer = decoder->cur_frame->size;
> +    cinfo->src->next_input_byte = decoder->cur_frame->data;
>  }
>  
>  static boolean mjpeg_src_fill(struct jpeg_decompress_struct *cinfo)
> @@ -166,12 +164,13 @@ static gboolean mjpeg_decoder_decode_frame(gpointer video_decoder)
>          dest = &(decoder->out_frame[decoder->mjpeg_cinfo.output_scanline * width * 4]);
>      }
>      jpeg_finish_decompress(&decoder->mjpeg_cinfo);
> +    decoder->cur_frame->unref_data(decoder->cur_frame->data_opaque);
>  
>      /* Display the frame and dispose of it */
> -    stream_display_frame(decoder->base.stream, decoder->cur_frame_msg,
> +    stream_display_frame(decoder->base.stream, decoder->cur_frame,
>                           width, height, decoder->out_frame);
> -    spice_msg_in_unref(decoder->cur_frame_msg);
> -    decoder->cur_frame_msg = NULL;
> +    decoder->cur_frame->free(decoder->cur_frame);
> +    decoder->cur_frame = NULL;
>      decoder->timer_id = 0;

->unref_data + ->free can probably be done in a helper for the mjpeg
decoder?

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170405/67b29411/attachment.sig>