[Spice-devel] [PATCH v5 16/20] spice-gtk: Refactor the video decoding to use a more object oriented design.
Christophe Fergeau
cfergeau at redhat.com
Mon Sep 21 08:07:33 PDT 2015
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150921/0d007218/attachment.sig>
More information about the Spice-devel
mailing list