Hi,
On Wed, Oct 14, 2015 at 05:34:17PM +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
> they will have more flexibility for their implementation.
>
> Signed-off-by: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
> ---
> src/channel-display-mjpeg.c | 131 ++++++++++++++++++++++++++++----------------
> src/channel-display-priv.h | 53 +++++++++++++-----
> src/channel-display.c | 65 ++++++++++------------
> 3 files changed, 152 insertions(+), 97 deletions(-)
>
>
> Changes since the previous version:
> * Removed the VIDEO_DECODER_T magic.
> * Removed NULL checks before free().
> * Used g_return_if_fail() in stream_mjpeg_cleanup().
>
>
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 95d5b33..78e3d5a 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -23,12 +23,33 @@
>
> #include "channel-display-priv.h"
>
> +
> +/* MJpeg decoder implementation */
> +
> +typedef 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;
> +} MJpegDecoder;
> +
> +
> +/* ---------- 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;
data could be not initialized here in case of error and
spice_msg_in_frame_data does not set it to NULL.
Either one should do it.
** stream_get_current_frame was doing it before
> }
>
> @@ -49,68 +70,57 @@ 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(VideoDecoder *video_decoder,
> + SpiceMsgIn *frame_msg)
> {
> - gboolean back_compat = st->channel->priv->peer_hdr.major_version == 1;
> + MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
> + 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);
> + decoder->frame_msg = frame_msg;
> + stream_get_dimensions(decoder->base.stream, frame_msg, &width, &height);
> + g_free(decoder->out_frame);
> + dest = decoder->out_frame = g_malloc0(width * height * 4);
>
> - g_free(st->out_frame);
> - st->out_frame = dest;
> -
> - 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 +128,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 +152,44 @@ 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(VideoDecoder* video_decoder)
> +{
> + MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
> + jpeg_destroy_decompress(&decoder->mjpeg_cinfo);
> + g_free(decoder->out_frame);
> + free(decoder);
> }
>
> G_GNUC_INTERNAL
> -void stream_mjpeg_cleanup(display_stream *st)
> +VideoDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream)
> {
> - jpeg_destroy_decompress(&st->mjpeg_cinfo);
> - g_free(st->out_frame);
> - st->out_frame = NULL;
> + g_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG, NULL);
> +
> + 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 (VideoDecoder*)decoder;
> }
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 71f5d17..4ca80b6 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -34,6 +34,39 @@
>
> G_BEGIN_DECLS
>
> +typedef struct display_stream display_stream;
> +
> +typedef struct VideoDecoder VideoDecoder;
> +struct VideoDecoder {
> + /* Releases the video decoder's resources */
> + void (*destroy)(VideoDecoder *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)(VideoDecoder *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.
> + */
> +VideoDecoder* create_mjpeg_decoder(int codec_type, display_stream *stream);
> +
>
> typedef struct display_surface {
> guint32 surface_id;
> @@ -54,24 +87,18 @@ 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;
>
> /* from messages */
> display_surface *surface;
> 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 +125,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 46e9829..9dd51fe 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;
^ mentioned above.
Patch looks good to me.
toso
> - 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)
> --
> 2.6.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel