[Spice-devel,v5,16/20] spice-gtk: Refactor the video decoding to use a more object oriented design.

Submitted by Francois Gouget on Aug. 27, 2015, 7:03 p.m.

Details

Message ID alpine.DEB.2.20.1508272041490.22172@amboise
State New
Headers show

Not browsing as part of any series.

Commit Message

Francois Gouget Aug. 27, 2015, 7:03 p.m.
The video decoder no longer stores its internal state in the display_stream struct, or depend on it to know which frame to decode or store the decoded frame.
This way adding alternative implementations will not pile all their implementation details in display_stream and have more flexibility for their implementation.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---

Changes since take 2:
 - None.


 src/channel-display-mjpeg.c | 140 +++++++++++++++++++++++++++++---------------
 src/channel-display-priv.h  |  56 +++++++++++++-----
 src/channel-display.c       |  65 +++++++++-----------
 3 files changed, 165 insertions(+), 96 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index 95d5b33..37b28f7 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -21,14 +21,38 @@ 
 #include "spice-common.h"
 #include "spice-channel-priv.h"
 
+typedef struct MJpegDecoder MJpegDecoder;
+#define VIDEO_DECODER_T MJpegDecoder
 #include "channel-display-priv.h"
 
+
+/* MJpeg decoder implementation */
+
+struct MJpegDecoder {
+    VideoDecoder base;
+
+    /* ---------- The builtin mjpeg decoder ---------- */
+
+    SpiceMsgIn *frame_msg;
+    struct jpeg_source_mgr         mjpeg_src;
+    struct jpeg_decompress_struct  mjpeg_cinfo;
+    struct jpeg_error_mgr          mjpeg_jerr;
+
+    /* ---------- Output frame data ---------- */
+
+    uint8_t *out_frame;
+    uint32_t out_size;
+};
+
+
+/* ---------- The JPEG library callbacks ---------- */
+
 static void mjpeg_src_init(struct jpeg_decompress_struct *cinfo)
 {
-    display_stream *st = SPICE_CONTAINEROF(cinfo->src, display_stream, mjpeg_src);
-    uint8_t *data;
+    MJpegDecoder *decoder = SPICE_CONTAINEROF(cinfo->src, MJpegDecoder, mjpeg_src);
 
-    cinfo->src->bytes_in_buffer = stream_get_current_frame(st, &data);
+    uint8_t *data;
+    cinfo->src->bytes_in_buffer = spice_msg_in_frame_data(decoder->frame_msg, &data);
     cinfo->src->next_input_byte = data;
 }
 
@@ -49,68 +73,62 @@  static void mjpeg_src_term(struct jpeg_decompress_struct *cinfo)
     /* nothing */
 }
 
-G_GNUC_INTERNAL
-void stream_mjpeg_init(display_stream *st)
-{
-    st->mjpeg_cinfo.err = jpeg_std_error(&st->mjpeg_jerr);
-    jpeg_create_decompress(&st->mjpeg_cinfo);
-
-    st->mjpeg_src.init_source         = mjpeg_src_init;
-    st->mjpeg_src.fill_input_buffer   = mjpeg_src_fill;
-    st->mjpeg_src.skip_input_data     = mjpeg_src_skip;
-    st->mjpeg_src.resync_to_restart   = jpeg_resync_to_restart;
-    st->mjpeg_src.term_source         = mjpeg_src_term;
-    st->mjpeg_cinfo.src               = &st->mjpeg_src;
-}
 
-G_GNUC_INTERNAL
-void stream_mjpeg_data(display_stream *st)
+/* ---------- VideoDecoder's public API ---------- */
+
+static uint8_t* mjpeg_decoder_decode_frame(MJpegDecoder *decoder,
+                                           SpiceMsgIn *frame_msg)
 {
-    gboolean back_compat = st->channel->priv->peer_hdr.major_version == 1;
+    gboolean back_compat = decoder->base.stream->channel->priv->peer_hdr.major_version == 1;
     int width;
     int height;
     uint8_t *dest;
     uint8_t *lines[4];
 
-    stream_get_dimensions(st, &width, &height);
-    dest = g_malloc0(width * height * 4);
-
-    g_free(st->out_frame);
-    st->out_frame = dest;
+    decoder->frame_msg = frame_msg;
+    stream_get_dimensions(decoder->base.stream, frame_msg, &width, &height);
+    if (decoder->out_size < width * height * 4) {
+        if (decoder->out_frame) {
+            free(decoder->out_frame);
+        }
+        decoder->out_size = width * height * 4;
+        decoder->out_frame = spice_malloc(decoder->out_size);
+    }
+    dest = decoder->out_frame;
 
-    jpeg_read_header(&st->mjpeg_cinfo, 1);
+    jpeg_read_header(&decoder->mjpeg_cinfo, 1);
 #ifdef JCS_EXTENSIONS
     // requires jpeg-turbo
     if (back_compat)
-        st->mjpeg_cinfo.out_color_space = JCS_EXT_RGBX;
+        decoder->mjpeg_cinfo.out_color_space = JCS_EXT_RGBX;
     else
-        st->mjpeg_cinfo.out_color_space = JCS_EXT_BGRX;
+        decoder->mjpeg_cinfo.out_color_space = JCS_EXT_BGRX;
 #else
 #warning "You should consider building with libjpeg-turbo"
-    st->mjpeg_cinfo.out_color_space = JCS_RGB;
+    decoder->mjpeg_cinfo.out_color_space = JCS_RGB;
 #endif
 
 #ifndef SPICE_QUALITY
-    st->mjpeg_cinfo.dct_method = JDCT_IFAST;
-    st->mjpeg_cinfo.do_fancy_upsampling = FALSE;
-    st->mjpeg_cinfo.do_block_smoothing = FALSE;
-    st->mjpeg_cinfo.dither_mode = JDITHER_ORDERED;
+    decoder->mjpeg_cinfo.dct_method = JDCT_IFAST;
+    decoder->mjpeg_cinfo.do_fancy_upsampling = FALSE;
+    decoder->mjpeg_cinfo.do_block_smoothing = FALSE;
+    decoder->mjpeg_cinfo.dither_mode = JDITHER_ORDERED;
 #endif
     // TODO: in theory should check cinfo.output_height match with our height
-    jpeg_start_decompress(&st->mjpeg_cinfo);
+    jpeg_start_decompress(&decoder->mjpeg_cinfo);
     /* rec_outbuf_height is the recommended size of the output buffer we
      * pass to libjpeg for optimum performance
      */
-    if (st->mjpeg_cinfo.rec_outbuf_height > G_N_ELEMENTS(lines)) {
-        jpeg_abort_decompress(&st->mjpeg_cinfo);
-        g_return_if_reached();
+    if (decoder->mjpeg_cinfo.rec_outbuf_height > G_N_ELEMENTS(lines)) {
+        jpeg_abort_decompress(&decoder->mjpeg_cinfo);
+        g_return_val_if_reached(NULL);
     }
 
-    while (st->mjpeg_cinfo.output_scanline < st->mjpeg_cinfo.output_height) {
+    while (decoder->mjpeg_cinfo.output_scanline < decoder->mjpeg_cinfo.output_height) {
         /* only used when JCS_EXTENSIONS is undefined */
         G_GNUC_UNUSED unsigned int lines_read;
 
-        for (unsigned int j = 0; j < st->mjpeg_cinfo.rec_outbuf_height; j++) {
+        for (unsigned int j = 0; j < decoder->mjpeg_cinfo.rec_outbuf_height; j++) {
             lines[j] = dest;
 #ifdef JCS_EXTENSIONS
             dest += 4 * width;
@@ -118,8 +136,8 @@  void stream_mjpeg_data(display_stream *st)
             dest += 3 * width;
 #endif
         }
-        lines_read = jpeg_read_scanlines(&st->mjpeg_cinfo, lines,
-                                st->mjpeg_cinfo.rec_outbuf_height);
+        lines_read = jpeg_read_scanlines(&decoder->mjpeg_cinfo, lines,
+                                decoder->mjpeg_cinfo.rec_outbuf_height);
 #ifndef JCS_EXTENSIONS
         {
             uint8_t *s = lines[0];
@@ -142,15 +160,45 @@  void stream_mjpeg_data(display_stream *st)
             }
         }
 #endif
-        dest = &st->out_frame[st->mjpeg_cinfo.output_scanline * width * 4];
+        dest = &(decoder->out_frame[decoder->mjpeg_cinfo.output_scanline * width * 4]);
     }
-    jpeg_finish_decompress(&st->mjpeg_cinfo);
+    jpeg_finish_decompress(&decoder->mjpeg_cinfo);
+
+    return decoder->out_frame;
+}
+
+static void mjpeg_decoder_destroy(MJpegDecoder* decoder)
+{
+    jpeg_destroy_decompress(&decoder->mjpeg_cinfo);
+    if (decoder->out_frame) {
+        free(decoder->out_frame);
+    }
+    free(decoder);
 }
 
 G_GNUC_INTERNAL
-void stream_mjpeg_cleanup(display_stream *st)
+MJpegDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream)
 {
-    jpeg_destroy_decompress(&st->mjpeg_cinfo);
-    g_free(st->out_frame);
-    st->out_frame = NULL;
+    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);
+
+    MJpegDecoder *decoder = spice_new0(MJpegDecoder, 1);
+
+    decoder->base.destroy = &mjpeg_decoder_destroy;
+    decoder->base.decode_frame = &mjpeg_decoder_decode_frame;
+    decoder->base.codec_type = codec_type;
+    decoder->base.stream = stream;
+
+    decoder->mjpeg_cinfo.err = jpeg_std_error(&decoder->mjpeg_jerr);
+    jpeg_create_decompress(&decoder->mjpeg_cinfo);
+
+    decoder->mjpeg_src.init_source         = mjpeg_src_init;
+    decoder->mjpeg_src.fill_input_buffer   = mjpeg_src_fill;
+    decoder->mjpeg_src.skip_input_data     = mjpeg_src_skip;
+    decoder->mjpeg_src.resync_to_restart   = jpeg_resync_to_restart;
+    decoder->mjpeg_src.term_source         = mjpeg_src_term;
+    decoder->mjpeg_cinfo.src               = &decoder->mjpeg_src;
+
+    /* All the other fields are initialized to zero by spice_new0(). */
+
+    return decoder;
 }
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 71f5d17..a01183b 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -34,6 +34,43 @@ 
 
 G_BEGIN_DECLS
 
+typedef struct display_stream display_stream;
+
+typedef struct VideoDecoder VideoDecoder;
+#ifndef VIDEO_DECODER_T
+# define VIDEO_DECODER_T VideoDecoder
+#endif
+
+struct VideoDecoder {
+    /* Releases the video decoder's resources */
+    void (*destroy)(VIDEO_DECODER_T *decoder);
+
+    /* Decompresses the specified frame.
+     *
+     * @decoder:   The video decoder.
+     * @frame_msg: The Spice message containing the compressed frame.
+     * @return:    A pointer to the buffer holding the decoded frame. This
+     *             buffer will be invalidated by the next call to
+     *             decode_frame().
+     */
+    uint8_t* (*decode_frame)(VIDEO_DECODER_T *decoder, SpiceMsgIn *frame_msg);
+
+    /* The format of the encoded video. */
+    int codec_type;
+
+    /* The associated display stream. */
+    display_stream *stream;
+};
+
+
+/* Instantiates the video decoder for the specified codec.
+ *
+ * @codec_type: The format of the video.
+ * @stream:     The associated video stream.
+ * @return:     A pointer to a structure implementing the VideoDecoder methods.
+ */
+VIDEO_DECODER_T* create_mjpeg_decoder(int codec_type, display_stream *stream);
+
 
 typedef struct display_surface {
     guint32                     surface_id;
@@ -54,7 +91,7 @@  typedef struct drops_sequence_stats {
     uint32_t duration;
 } drops_sequence_stats;
 
-typedef struct display_stream {
+struct display_stream {
     SpiceMsgIn                  *msg_create;
     SpiceMsgIn                  *msg_clip;
     SpiceMsgIn                  *msg_data;
@@ -64,14 +101,9 @@  typedef struct display_stream {
     SpiceClip                   *clip;
     QRegion                     region;
     int                         have_region;
-    int                         codec;
 
-    /* mjpeg decoder */
-    struct jpeg_source_mgr         mjpeg_src;
-    struct jpeg_decompress_struct  mjpeg_cinfo;
-    struct jpeg_error_mgr          mjpeg_jerr;
+    VideoDecoder                *video_decoder;
 
-    uint8_t                     *out_frame;
     GQueue                      *msgq;
     guint                       timeout;
     SpiceChannel                *channel;
@@ -98,15 +130,11 @@  typedef struct display_stream {
     uint32_t report_num_frames;
     uint32_t report_num_drops;
     uint32_t report_drops_seq_len;
-} display_stream;
+};
 
-void stream_get_dimensions(display_stream *st, int *width, int *height);
-uint32_t stream_get_current_frame(display_stream *st, uint8_t **data);
+void stream_get_dimensions(display_stream *st, SpiceMsgIn *frame_msg, int *width, int *height);
+uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data);
 
-/* channel-display-mjpeg.c */
-void stream_mjpeg_init(display_stream *st);
-void stream_mjpeg_data(display_stream *st);
-void stream_mjpeg_cleanup(display_stream *st);
 
 G_END_DECLS
 
diff --git a/src/channel-display.c b/src/channel-display.c
index a65f926..4033b2f 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -996,7 +996,6 @@  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
     st->msg_create = in;
     spice_msg_in_ref(in);
     st->clip = &op->clip;
-    st->codec = op->codec_type;
     st->surface = find_surface(c, op->surface_id);
     st->msgq = g_queue_new();
     st->channel = channel;
@@ -1005,10 +1004,15 @@  static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
     region_init(&st->region);
     display_update_stream_region(st);
 
-    switch (st->codec) {
+    switch (op->codec_type) {
     case SPICE_VIDEO_CODEC_TYPE_MJPEG:
-        stream_mjpeg_init(st);
+        st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
         break;
+    default:
+        st->video_decoder = NULL;
+    }
+    if (st->video_decoder == NULL) {
+        spice_printerr("could not create a video decoder for codec %d", op->codec_type);
     }
 }
 
@@ -1051,15 +1055,15 @@  static gboolean display_stream_schedule(display_stream *st)
     return FALSE;
 }
 
-static SpiceRect *stream_get_dest(display_stream *st)
+static SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg)
 {
-    if (st->msg_data == NULL ||
-        spice_msg_in_type(st->msg_data) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
+    if (frame_msg == NULL ||
+        spice_msg_in_type(frame_msg) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
         SpiceMsgDisplayStreamCreate *info = spice_msg_in_parsed(st->msg_create);
 
         return &info->dest;
     } else {
-        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
+        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
 
         return &op->dest;
    }
@@ -1074,22 +1078,17 @@  static uint32_t stream_get_flags(display_stream *st)
 }
 
 G_GNUC_INTERNAL
-uint32_t stream_get_current_frame(display_stream *st, uint8_t **data)
+uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data)
 {
-    if (st->msg_data == NULL) {
-        *data = NULL;
-        return 0;
-    }
-
-    if (spice_msg_in_type(st->msg_data) == SPICE_MSG_DISPLAY_STREAM_DATA) {
-        SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(st->msg_data);
+    if (spice_msg_in_type(frame_msg) == SPICE_MSG_DISPLAY_STREAM_DATA) {
+        SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(frame_msg);
 
         *data = op->data;
         return op->data_size;
     } else {
-        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
+        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
 
-        g_return_val_if_fail(spice_msg_in_type(st->msg_data) ==
+        g_return_val_if_fail(spice_msg_in_type(frame_msg) ==
                              SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, 0);
         *data = op->data;
         return op->data_size;
@@ -1098,19 +1097,19 @@  uint32_t stream_get_current_frame(display_stream *st, uint8_t **data)
 }
 
 G_GNUC_INTERNAL
-void stream_get_dimensions(display_stream *st, int *width, int *height)
+void stream_get_dimensions(display_stream *st, SpiceMsgIn *frame_msg, int *width, int *height)
 {
     g_return_if_fail(width != NULL);
     g_return_if_fail(height != NULL);
 
-    if (st->msg_data == NULL ||
-        spice_msg_in_type(st->msg_data) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
+    if (frame_msg == NULL ||
+        spice_msg_in_type(frame_msg) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
         SpiceMsgDisplayStreamCreate *info = spice_msg_in_parsed(st->msg_create);
 
         *width = info->stream_width;
         *height = info->stream_height;
     } else {
-        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
+        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
 
         *width = op->width;
         *height = op->height;
@@ -1128,24 +1127,21 @@  static gboolean display_stream_render(display_stream *st)
 
         g_return_val_if_fail(in != NULL, FALSE);
 
-        st->msg_data = in;
-        switch (st->codec) {
-        case SPICE_VIDEO_CODEC_TYPE_MJPEG:
-            stream_mjpeg_data(st);
-            break;
+        uint8_t *out_frame = NULL;
+        if (st->video_decoder) {
+            out_frame = st->video_decoder->decode_frame(st->video_decoder, in);
         }
-
-        if (st->out_frame) {
+        if (out_frame) {
             int width;
             int height;
             SpiceRect *dest;
             uint8_t *data;
             int stride;
 
-            stream_get_dimensions(st, &width, &height);
-            dest = stream_get_dest(st);
+            stream_get_dimensions(st, in, &width, &height);
+            dest = stream_get_dest(st, in);
 
-            data = st->out_frame;
+            data = out_frame;
             stride = width * sizeof(uint32_t);
             if (!(stream_get_flags(st) & SPICE_STREAM_FLAGS_TOP_DOWN)) {
                 data += stride * (height - 1);
@@ -1168,7 +1164,6 @@  static gboolean display_stream_render(display_stream *st)
                     dest->bottom - dest->top);
         }
 
-        st->msg_data = NULL;
         spice_msg_in_unref(in);
 
         in = g_queue_peek_head(st->msgq);
@@ -1477,10 +1472,8 @@  static void destroy_stream(SpiceChannel *channel, int id)
 
     g_array_free(st->drops_seqs_stats_arr, TRUE);
 
-    switch (st->codec) {
-    case SPICE_VIDEO_CODEC_TYPE_MJPEG:
-        stream_mjpeg_cleanup(st);
-        break;
+    if (st->video_decoder) {
+        st->video_decoder->destroy(st->video_decoder);
     }
 
     if (st->msg_clip)

Comments

Hey,

On Thu, Aug 27, 2015 at 09:03:01PM +0200, Francois Gouget wrote:
> The video decoder no longer stores its internal state in the display_stream struct, or depend on it to know which frame to decode or store the decoded frame.
> This way adding alternative implementations will not pile all their implementation details in display_stream and have more flexibility for their implementation.

Please (applies to all your patches), limit your short log summary to
about 50 chars, and wrap the commit lines, see
http://chris.beams.io/posts/git-commit/#seven-rules

> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
> 
> Changes since take 2:
>  - None.
> 
> 
>  src/channel-display-mjpeg.c | 140 +++++++++++++++++++++++++++++---------------
>  src/channel-display-priv.h  |  56 +++++++++++++-----
>  src/channel-display.c       |  65 +++++++++-----------
>  3 files changed, 165 insertions(+), 96 deletions(-)
> 
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 95d5b33..37b28f7 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -21,14 +21,38 @@
>  #include "spice-common.h"
>  #include "spice-channel-priv.h"
>  
> +typedef struct MJpegDecoder MJpegDecoder;
> +#define VIDEO_DECODER_T MJpegDecoder

I'd prefer that this #define VIDEO_DECODER_T templating magic is avoided

>  #include "channel-display-priv.h"
>  
> +
> +/* MJpeg decoder implementation */
> +
> +struct MJpegDecoder {
> +    VideoDecoder base;
> +
> +    /* ---------- The builtin mjpeg decoder ---------- */
> +
> +    SpiceMsgIn *frame_msg;
> +    struct jpeg_source_mgr         mjpeg_src;
> +    struct jpeg_decompress_struct  mjpeg_cinfo;
> +    struct jpeg_error_mgr          mjpeg_jerr;
> +
> +    /* ---------- Output frame data ---------- */
> +
> +    uint8_t *out_frame;
> +    uint32_t out_size;
> +};
> +
> +
> +/* ---------- The JPEG library callbacks ---------- */
> +
>  static void mjpeg_src_init(struct jpeg_decompress_struct *cinfo)
>  {
> -    display_stream *st = SPICE_CONTAINEROF(cinfo->src, display_stream, mjpeg_src);
> -    uint8_t *data;
> +    MJpegDecoder *decoder = SPICE_CONTAINEROF(cinfo->src, MJpegDecoder, mjpeg_src);
>  
> -    cinfo->src->bytes_in_buffer = stream_get_current_frame(st, &data);
> +    uint8_t *data;
> +    cinfo->src->bytes_in_buffer = spice_msg_in_frame_data(decoder->frame_msg, &data);
>      cinfo->src->next_input_byte = data;
>  }
>  
> @@ -49,68 +73,62 @@ static void mjpeg_src_term(struct jpeg_decompress_struct *cinfo)
>      /* nothing */
>  }
>  
> -G_GNUC_INTERNAL
> -void stream_mjpeg_init(display_stream *st)
> -{
> -    st->mjpeg_cinfo.err = jpeg_std_error(&st->mjpeg_jerr);
> -    jpeg_create_decompress(&st->mjpeg_cinfo);
> -
> -    st->mjpeg_src.init_source         = mjpeg_src_init;
> -    st->mjpeg_src.fill_input_buffer   = mjpeg_src_fill;
> -    st->mjpeg_src.skip_input_data     = mjpeg_src_skip;
> -    st->mjpeg_src.resync_to_restart   = jpeg_resync_to_restart;
> -    st->mjpeg_src.term_source         = mjpeg_src_term;
> -    st->mjpeg_cinfo.src               = &st->mjpeg_src;
> -}
>  
> -G_GNUC_INTERNAL
> -void stream_mjpeg_data(display_stream *st)
> +/* ---------- VideoDecoder's public API ---------- */
> +
> +static uint8_t* mjpeg_decoder_decode_frame(MJpegDecoder *decoder,
> +                                           SpiceMsgIn *frame_msg)
>  {
> -    gboolean back_compat = st->channel->priv->peer_hdr.major_version == 1;
> +    gboolean back_compat = decoder->base.stream->channel->priv->peer_hdr.major_version == 1;
>      int width;
>      int height;
>      uint8_t *dest;
>      uint8_t *lines[4];
>  
> -    stream_get_dimensions(st, &width, &height);
> -    dest = g_malloc0(width * height * 4);
> -
> -    g_free(st->out_frame);
> -    st->out_frame = dest;
> +    decoder->frame_msg = frame_msg;
> +    stream_get_dimensions(decoder->base.stream, frame_msg, &width, &height);
> +    if (decoder->out_size < width * height * 4) {
> +        if (decoder->out_frame) {
> +            free(decoder->out_frame);
> +        }

No need for the if()

> +        decoder->out_size = width * height * 4;
> +        decoder->out_frame = spice_malloc(decoder->out_size);
> +    }
> +    dest = decoder->out_frame;
>  
> -    jpeg_read_header(&st->mjpeg_cinfo, 1);
> +    jpeg_read_header(&decoder->mjpeg_cinfo, 1);
>  #ifdef JCS_EXTENSIONS
>      // requires jpeg-turbo
>      if (back_compat)
> -        st->mjpeg_cinfo.out_color_space = JCS_EXT_RGBX;
> +        decoder->mjpeg_cinfo.out_color_space = JCS_EXT_RGBX;
>      else
> -        st->mjpeg_cinfo.out_color_space = JCS_EXT_BGRX;
> +        decoder->mjpeg_cinfo.out_color_space = JCS_EXT_BGRX;
>  #else
>  #warning "You should consider building with libjpeg-turbo"
> -    st->mjpeg_cinfo.out_color_space = JCS_RGB;
> +    decoder->mjpeg_cinfo.out_color_space = JCS_RGB;
>  #endif
>  
>  #ifndef SPICE_QUALITY
> -    st->mjpeg_cinfo.dct_method = JDCT_IFAST;
> -    st->mjpeg_cinfo.do_fancy_upsampling = FALSE;
> -    st->mjpeg_cinfo.do_block_smoothing = FALSE;
> -    st->mjpeg_cinfo.dither_mode = JDITHER_ORDERED;
> +    decoder->mjpeg_cinfo.dct_method = JDCT_IFAST;
> +    decoder->mjpeg_cinfo.do_fancy_upsampling = FALSE;
> +    decoder->mjpeg_cinfo.do_block_smoothing = FALSE;
> +    decoder->mjpeg_cinfo.dither_mode = JDITHER_ORDERED;
>  #endif
>      // TODO: in theory should check cinfo.output_height match with our height
> -    jpeg_start_decompress(&st->mjpeg_cinfo);
> +    jpeg_start_decompress(&decoder->mjpeg_cinfo);
>      /* rec_outbuf_height is the recommended size of the output buffer we
>       * pass to libjpeg for optimum performance
>       */
> -    if (st->mjpeg_cinfo.rec_outbuf_height > G_N_ELEMENTS(lines)) {
> -        jpeg_abort_decompress(&st->mjpeg_cinfo);
> -        g_return_if_reached();
> +    if (decoder->mjpeg_cinfo.rec_outbuf_height > G_N_ELEMENTS(lines)) {
> +        jpeg_abort_decompress(&decoder->mjpeg_cinfo);
> +        g_return_val_if_reached(NULL);
>      }
>  
> -    while (st->mjpeg_cinfo.output_scanline < st->mjpeg_cinfo.output_height) {
> +    while (decoder->mjpeg_cinfo.output_scanline < decoder->mjpeg_cinfo.output_height) {
>          /* only used when JCS_EXTENSIONS is undefined */
>          G_GNUC_UNUSED unsigned int lines_read;
>  
> -        for (unsigned int j = 0; j < st->mjpeg_cinfo.rec_outbuf_height; j++) {
> +        for (unsigned int j = 0; j < decoder->mjpeg_cinfo.rec_outbuf_height; j++) {
>              lines[j] = dest;
>  #ifdef JCS_EXTENSIONS
>              dest += 4 * width;
> @@ -118,8 +136,8 @@ void stream_mjpeg_data(display_stream *st)
>              dest += 3 * width;
>  #endif
>          }
> -        lines_read = jpeg_read_scanlines(&st->mjpeg_cinfo, lines,
> -                                st->mjpeg_cinfo.rec_outbuf_height);
> +        lines_read = jpeg_read_scanlines(&decoder->mjpeg_cinfo, lines,
> +                                decoder->mjpeg_cinfo.rec_outbuf_height);
>  #ifndef JCS_EXTENSIONS
>          {
>              uint8_t *s = lines[0];
> @@ -142,15 +160,45 @@ void stream_mjpeg_data(display_stream *st)
>              }
>          }
>  #endif
> -        dest = &st->out_frame[st->mjpeg_cinfo.output_scanline * width * 4];
> +        dest = &(decoder->out_frame[decoder->mjpeg_cinfo.output_scanline * width * 4]);
>      }
> -    jpeg_finish_decompress(&st->mjpeg_cinfo);
> +    jpeg_finish_decompress(&decoder->mjpeg_cinfo);
> +
> +    return decoder->out_frame;
> +}
> +
> +static void mjpeg_decoder_destroy(MJpegDecoder* decoder)
> +{
> +    jpeg_destroy_decompress(&decoder->mjpeg_cinfo);
> +    if (decoder->out_frame) {
> +        free(decoder->out_frame);
> +    }

Can be just free(decoder->out_frame);

> +    free(decoder);
>  }
>  
>  G_GNUC_INTERNAL
> -void stream_mjpeg_cleanup(display_stream *st)
> +MJpegDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream)
>  {
> -    jpeg_destroy_decompress(&st->mjpeg_cinfo);
> -    g_free(st->out_frame);
> -    st->out_frame = NULL;
> +    spice_assert(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG);

g_return_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG, NULL);
would be nicer.

> +
> +    MJpegDecoder *decoder = spice_new0(MJpegDecoder, 1);
> +
> +    decoder->base.destroy = &mjpeg_decoder_destroy;
> +    decoder->base.decode_frame = &mjpeg_decoder_decode_frame;
> +    decoder->base.codec_type = codec_type;
> +    decoder->base.stream = stream;

I'd personally do

VideoDecoder *base_decoder = (VideoDecoder *)decoder;
base_decoder->destroy = ...

but that's me, I'm fine with the way you did it too.

> +
> +    decoder->mjpeg_cinfo.err = jpeg_std_error(&decoder->mjpeg_jerr);
> +    jpeg_create_decompress(&decoder->mjpeg_cinfo);
> +
> +    decoder->mjpeg_src.init_source         = mjpeg_src_init;
> +    decoder->mjpeg_src.fill_input_buffer   = mjpeg_src_fill;
> +    decoder->mjpeg_src.skip_input_data     = mjpeg_src_skip;
> +    decoder->mjpeg_src.resync_to_restart   = jpeg_resync_to_restart;
> +    decoder->mjpeg_src.term_source         = mjpeg_src_term;
> +    decoder->mjpeg_cinfo.src               = &decoder->mjpeg_src;
> +
> +    /* All the other fields are initialized to zero by spice_new0(). */
> +
> +    return decoder;
>  }
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 71f5d17..a01183b 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -34,6 +34,43 @@
>  
>  G_BEGIN_DECLS
>  
> +typedef struct display_stream display_stream;
> +
> +typedef struct VideoDecoder VideoDecoder;
> +#ifndef VIDEO_DECODER_T
> +# define VIDEO_DECODER_T VideoDecoder
> +#endif
> +
> +struct VideoDecoder {
> +    /* Releases the video decoder's resources */
> +    void (*destroy)(VIDEO_DECODER_T *decoder);
> +
> +    /* Decompresses the specified frame.
> +     *
> +     * @decoder:   The video decoder.
> +     * @frame_msg: The Spice message containing the compressed frame.
> +     * @return:    A pointer to the buffer holding the decoded frame. This
> +     *             buffer will be invalidated by the next call to
> +     *             decode_frame().
> +     */
> +    uint8_t* (*decode_frame)(VIDEO_DECODER_T *decoder, SpiceMsgIn *frame_msg);
> +
> +    /* The format of the encoded video. */
> +    int codec_type;
> +
> +    /* The associated display stream. */
> +    display_stream *stream;
> +};
> +
> +
> +/* Instantiates the video decoder for the specified codec.
> + *
> + * @codec_type: The format of the video.
> + * @stream:     The associated video stream.
> + * @return:     A pointer to a structure implementing the VideoDecoder methods.
> + */
> +VIDEO_DECODER_T* create_mjpeg_decoder(int codec_type, display_stream *stream);
> +
>  
>  typedef struct display_surface {
>      guint32                     surface_id;
> @@ -54,7 +91,7 @@ typedef struct drops_sequence_stats {
>      uint32_t duration;
>  } drops_sequence_stats;
>  
> -typedef struct display_stream {
> +struct display_stream {
>      SpiceMsgIn                  *msg_create;
>      SpiceMsgIn                  *msg_clip;
>      SpiceMsgIn                  *msg_data;
> @@ -64,14 +101,9 @@ typedef struct display_stream {
>      SpiceClip                   *clip;
>      QRegion                     region;
>      int                         have_region;
> -    int                         codec;
>  
> -    /* mjpeg decoder */
> -    struct jpeg_source_mgr         mjpeg_src;
> -    struct jpeg_decompress_struct  mjpeg_cinfo;
> -    struct jpeg_error_mgr          mjpeg_jerr;
> +    VideoDecoder                *video_decoder;
>  
> -    uint8_t                     *out_frame;
>      GQueue                      *msgq;
>      guint                       timeout;
>      SpiceChannel                *channel;
> @@ -98,15 +130,11 @@ typedef struct display_stream {
>      uint32_t report_num_frames;
>      uint32_t report_num_drops;
>      uint32_t report_drops_seq_len;
> -} display_stream;
> +};
>  
> -void stream_get_dimensions(display_stream *st, int *width, int *height);
> -uint32_t stream_get_current_frame(display_stream *st, uint8_t **data);
> +void stream_get_dimensions(display_stream *st, SpiceMsgIn *frame_msg, int *width, int *height);
> +uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data);
>  
> -/* channel-display-mjpeg.c */
> -void stream_mjpeg_init(display_stream *st);
> -void stream_mjpeg_data(display_stream *st);
> -void stream_mjpeg_cleanup(display_stream *st);
>  
>  G_END_DECLS
>  
> diff --git a/src/channel-display.c b/src/channel-display.c
> index a65f926..4033b2f 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -996,7 +996,6 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>      st->msg_create = in;
>      spice_msg_in_ref(in);
>      st->clip = &op->clip;
> -    st->codec = op->codec_type;
>      st->surface = find_surface(c, op->surface_id);
>      st->msgq = g_queue_new();
>      st->channel = channel;
> @@ -1005,10 +1004,15 @@ static void display_handle_stream_create(SpiceChannel *channel, SpiceMsgIn *in)
>      region_init(&st->region);
>      display_update_stream_region(st);
>  
> -    switch (st->codec) {
> +    switch (op->codec_type) {
>      case SPICE_VIDEO_CODEC_TYPE_MJPEG:
> -        stream_mjpeg_init(st);
> +        st->video_decoder = create_mjpeg_decoder(op->codec_type, st);
>          break;
> +    default:
> +        st->video_decoder = NULL;
> +    }
> +    if (st->video_decoder == NULL) {
> +        spice_printerr("could not create a video decoder for codec %d", op->codec_type);
>      }
>  }
>  
> @@ -1051,15 +1055,15 @@ static gboolean display_stream_schedule(display_stream *st)
>      return FALSE;
>  }
>  
> -static SpiceRect *stream_get_dest(display_stream *st)
> +static SpiceRect *stream_get_dest(display_stream *st, SpiceMsgIn *frame_msg)
>  {
> -    if (st->msg_data == NULL ||
> -        spice_msg_in_type(st->msg_data) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
> +    if (frame_msg == NULL ||
> +        spice_msg_in_type(frame_msg) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
>          SpiceMsgDisplayStreamCreate *info = spice_msg_in_parsed(st->msg_create);
>  
>          return &info->dest;
>      } else {
> -        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
> +        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
>  
>          return &op->dest;
>     }
> @@ -1074,22 +1078,17 @@ static uint32_t stream_get_flags(display_stream *st)
>  }
>  
>  G_GNUC_INTERNAL
> -uint32_t stream_get_current_frame(display_stream *st, uint8_t **data)
> +uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data)
>  {
> -    if (st->msg_data == NULL) {
> -        *data = NULL;
> -        return 0;
> -    }
> -
> -    if (spice_msg_in_type(st->msg_data) == SPICE_MSG_DISPLAY_STREAM_DATA) {
> -        SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(st->msg_data);
> +    if (spice_msg_in_type(frame_msg) == SPICE_MSG_DISPLAY_STREAM_DATA) {
> +        SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(frame_msg);
>  
>          *data = op->data;
>          return op->data_size;
>      } else {
> -        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
> +        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
>  
> -        g_return_val_if_fail(spice_msg_in_type(st->msg_data) ==
> +        g_return_val_if_fail(spice_msg_in_type(frame_msg) ==
>                               SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, 0);
>          *data = op->data;
>          return op->data_size;
> @@ -1098,19 +1097,19 @@ uint32_t stream_get_current_frame(display_stream *st, uint8_t **data)
>  }
>  
>  G_GNUC_INTERNAL
> -void stream_get_dimensions(display_stream *st, int *width, int *height)
> +void stream_get_dimensions(display_stream *st, SpiceMsgIn *frame_msg, int *width, int *height)
>  {
>      g_return_if_fail(width != NULL);
>      g_return_if_fail(height != NULL);
>  
> -    if (st->msg_data == NULL ||
> -        spice_msg_in_type(st->msg_data) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
> +    if (frame_msg == NULL ||
> +        spice_msg_in_type(frame_msg) != SPICE_MSG_DISPLAY_STREAM_DATA_SIZED) {
>          SpiceMsgDisplayStreamCreate *info = spice_msg_in_parsed(st->msg_create);
>  
>          *width = info->stream_width;
>          *height = info->stream_height;
>      } else {
> -        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
> +        SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
>  
>          *width = op->width;
>          *height = op->height;
> @@ -1128,24 +1127,21 @@ static gboolean display_stream_render(display_stream *st)
>  
>          g_return_val_if_fail(in != NULL, FALSE);
>  
> -        st->msg_data = in;

Not very clear in this commit why you needed to stop storing the message
within the display_stream structure, and makes the diff bigger than it
could, I guess I'll find an explanation when looking at the commits
introducing gstreamer ;)

Looks good otherwise.

Christophe
On Mon, 21 Sep 2015, Christophe Fergeau wrote:
[...]
> > -        st->msg_data = in;
> 
> Not very clear in this commit why you needed to stop storing the message
> within the display_stream structure, and makes the diff bigger than it
> could, I guess I'll find an explanation when looking at the commits
> introducing gstreamer ;)

This patch introduces a proper API for the video decoder. Part of that 
is not having the video decoder depend on the internals of the 
display_stream structure so it is more reusable.

Before this patch channel-display-mjpeg.c has no place to store the 
frame message. Yet, as far as I can see, it cannot pass it to the 
mjpeg_src_init() callback that needs it. So removing the 
display_stream.msg_data field cannot come before this patch. And I don't 
think it's worth introducing a flawed version of the video decoder API 
just to fix it in the next patch.
On Wed, Sep 30, 2015 at 01:40:09PM +0200, Francois Gouget wrote:
> On Mon, 21 Sep 2015, Christophe Fergeau wrote:
> [...]
> > > -        st->msg_data = in;
> > 
> > Not very clear in this commit why you needed to stop storing the message
> > within the display_stream structure, and makes the diff bigger than it
> > could, I guess I'll find an explanation when looking at the commits
> > introducing gstreamer ;)
> 
> This patch introduces a proper API for the video decoder. Part of that 
> is not having the video decoder depend on the internals of the 
> display_stream structure so it is more reusable.
> 
> Before this patch channel-display-mjpeg.c has no place to store the 
> frame message. Yet, as far as I can see, it cannot pass it to the 
> mjpeg_src_init() callback that needs it. So removing the 
> display_stream.msg_data field cannot come before this patch. And I don't 
> think it's worth introducing a flawed version of the video decoder API 
> just to fix it in the next patch.

Sure, I was not necessarily suggesting more code changas, a short
explanation in the commit log might be helpful though.

Christophe