On Fri, 16 Oct 2015, Victor Toso wrote:
[...]
> > - 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
Actually it didn't quite.
It set data to NULL in case msg_data was NULL. But I consider not
passing a NULL pointer to spice_msg_in_frame_data() to be the
responsibility of the caller since it would not make sense. In any case
the g_return_val_if_fail(in != NULL, FALSE) check in
display_stream_render() makes this case impossible, probably more
clearly now that we're not using a stream member variable that almost
anyone can access.
But in the other error case stream_get_current_frame() was not
initializing data. So I propose the attached two-patch set.
p21a.diff makes sure stream_get_current_frame() always initializes data.
p21b.diff is the updated patch preserving this behavior in
spice_msg_in_frame_data().
As far as I can tell the remainder of the patches should still apply
fine.
--
Francois Gouget <fgouget@xxxxxxxxxxxxxxx>commit b1afb23a53489a5dd3b25046d73bdcbade2f79d1
Author: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
Date: Fri Oct 16 16:06:49 2015 +0200
spice-gtk: Fix error handling in stream_get_current_frame()
diff --git a/src/channel-display.c b/src/channel-display.c
index 46e9829..9e42dd9 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -1081,20 +1081,21 @@ uint32_t stream_get_current_frame(display_stream *st, uint8_t **data)
return 0;
}
- if (spice_msg_in_type(st->msg_data) == SPICE_MSG_DISPLAY_STREAM_DATA) {
+ switch (spice_msg_in_type(st->msg_data)) {
+ case SPICE_MSG_DISPLAY_STREAM_DATA: {
SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(st->msg_data);
-
*data = op->data;
return op->data_size;
- } else {
+ }
+ case SPICE_MSG_DISPLAY_STREAM_DATA_SIZED: {
SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
-
- g_return_val_if_fail(spice_msg_in_type(st->msg_data) ==
- SPICE_MSG_DISPLAY_STREAM_DATA_SIZED, 0);
*data = op->data;
return op->data_size;
- }
-
+ }
+ default:
+ *data = NULL;
+ g_return_val_if_reached(0);
+ }
}
G_GNUC_INTERNAL
commit 72d4800c10801370bb95bf652d6d3d9943c02bcb
Author: Francois Gouget <fgouget@xxxxxxxxxxxxxxx>
Date: Wed Jul 29 12:43:42 2015 +0200
spice-gtk: Enable adding alternative video decoders
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>
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;
}
@@ -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 9e42dd9..5c916cc 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,21 +1078,16 @@ 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;
- }
-
- switch (spice_msg_in_type(st->msg_data)) {
+ switch (spice_msg_in_type(frame_msg)) {
case SPICE_MSG_DISPLAY_STREAM_DATA: {
- SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(st->msg_data);
+ SpiceMsgDisplayStreamData *op = spice_msg_in_parsed(frame_msg);
*data = op->data;
return op->data_size;
}
case SPICE_MSG_DISPLAY_STREAM_DATA_SIZED: {
- SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(st->msg_data);
+ SpiceMsgDisplayStreamDataSized *op = spice_msg_in_parsed(frame_msg);
*data = op->data;
return op->data_size;
}
@@ -1099,19 +1098,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;
@@ -1129,24 +1128,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);
@@ -1169,7 +1165,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);
@@ -1478,10 +1473,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)
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel