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@xxxxxxxxxxxxxxx> > --- > > 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
Attachment:
pgpVp8mTJ54yj.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel