[Spice-devel] [client 2/2] streaming: Use the frame dimensions reported by the video decoder
Pavel Grunt
pgrunt at redhat.com
Wed May 25 15:06:50 UTC 2016
Hi Francois,
On Tue, 2016-05-24 at 11:46 +0200, Francois Gouget wrote:
> The dimensions sent by the remote end are redundant and should not be
> trusted.
>
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
> src/channel-display-gst.c | 40 +++++++++++++++++++++++++++++++---------
> src/channel-display-mjpeg.c | 25 +++++++++++++------------
> src/channel-display-priv.h | 3 +--
> src/channel-display.c | 24 +-----------------------
> 4 files changed, 46 insertions(+), 46 deletions(-)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 46a85ea..f8a460f 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -93,18 +93,40 @@ static gboolean display_frame(gpointer video_decoder)
> g_mutex_lock(&decoder->queues_mutex);
> SpiceFrame *frame = g_queue_pop_head(decoder->display_queue);
> g_mutex_unlock(&decoder->queues_mutex);
> + /* If the queue is empty we don't even need to reschedule */
> g_return_val_if_fail(frame, G_SOURCE_REMOVE);
>
> - GstBuffer *buffer = frame->sample ? gst_sample_get_buffer(frame->sample)
> : NULL;
> - GstMapInfo mapinfo;
> - if (!frame->sample) {
> - spice_warning("got a frame without a sample!");
> - } else if (gst_buffer_map(buffer, &mapinfo, GST_MAP_READ)) {
> - stream_display_frame(decoder->base.stream, frame->msg, mapinfo.data);
> + do {
it is preferred to declare variables at the beginning of the block. imo a 'goto'
would be more readable than do {} while(FALSE)
Regards,
Pavel
> + if (!frame->sample) {
> + spice_warning("got a frame without a sample!");
> + break;
> + }
> +
> + GstCaps *caps = gst_sample_get_caps(frame->sample);
> + if (!caps) {
> + spice_warning("GStreamer error: could not get the caps of the
> sample");
> + break;
> + }
> +
> + gint width, height;
> + GstStructure *s = gst_caps_get_structure(caps, 0);
> + if (!gst_structure_get_int(s, "width", &width) ||
> + !gst_structure_get_int(s, "height", &height)) {
> + spice_warning("GStreamer error: could not get the size of the
> frame");
> + break;
> + }
> +
> + GstBuffer *buffer = gst_sample_get_buffer(frame->sample);
> + GstMapInfo mapinfo;
> + if (!gst_buffer_map(buffer, &mapinfo, GST_MAP_READ)) {
> + spice_warning("GStreamer error: could not map the buffer");
> + break;
> + }
> +
> + stream_display_frame(decoder->base.stream, frame->msg,
> + width, height, mapinfo.data);
> gst_buffer_unmap(buffer, &mapinfo);
> - } else {
> - spice_warning("GStreamer error: could not map the buffer");
> - }
> + } while (FALSE);
> free_frame(frame);
>
> schedule_frame(decoder);
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 1238b41..3327a6b 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -86,20 +86,18 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> {
> 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(decoder->base.stream, decoder->cur_frame_msg,
> &width, &height);
> - if (decoder->out_size < width * height * 4) {
> + jpeg_read_header(&decoder->mjpeg_cinfo, 1);
> + uint32_t img_size = decoder->mjpeg_cinfo.image_width * decoder-
> >mjpeg_cinfo.image_height * 4;
> + if (decoder->out_size < img_size) {
> g_free(decoder->out_frame);
> - decoder->out_size = width * height * 4;
> + decoder->out_size = img_size;
> decoder->out_frame = g_malloc(decoder->out_size);
> }
> dest = decoder->out_frame;
>
> - jpeg_read_header(&decoder->mjpeg_cinfo, 1);
> #ifdef JCS_EXTENSIONS
> // requires jpeg-turbo
> if (back_compat)
> @@ -134,9 +132,9 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> for (unsigned int j = 0; j < decoder->mjpeg_cinfo.rec_outbuf_height;
> j++) {
> lines[j] = dest;
> #ifdef JCS_EXTENSIONS
> - dest += 4 * width;
> + dest += 4 * decoder->mjpeg_cinfo.image_width;
> #else
> - dest += 3 * width;
> + dest += 3 * decoder->mjpeg_cinfo.image_width;
> #endif
> }
> lines_read = jpeg_read_scanlines(&decoder->mjpeg_cinfo, lines,
> @@ -147,14 +145,14 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> uint32_t *d = (uint32_t *)s;
>
> if (back_compat) {
> - for (unsigned int j = lines_read * width; j > 0; ) {
> + for (unsigned int j = lines_read * decoder-
> >mjpeg_cinfo.image_width; j > 0; ) {
> j -= 1; // reverse order, bad for cache?
> d[j] = s[j * 3 + 0] |
> s[j * 3 + 1] << 8 |
> s[j * 3 + 2] << 16;
> }
> } else {
> - for (unsigned int j = lines_read * width; j > 0; ) {
> + for (unsigned int j = lines_read * decoder-
> >mjpeg_cinfo.image_width; j > 0; ) {
> j -= 1; // reverse order, bad for cache?
> d[j] = s[j * 3 + 0] << 16 |
> s[j * 3 + 1] << 8 |
> @@ -163,12 +161,15 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> }
> }
> #endif
> - dest = &(decoder->out_frame[decoder->mjpeg_cinfo.output_scanline *
> width * 4]);
> + dest = &(decoder->out_frame[decoder->mjpeg_cinfo.output_scanline *
> decoder->mjpeg_cinfo.image_width * 4]);
> }
> jpeg_finish_decompress(&decoder->mjpeg_cinfo);
>
> /* Display the frame and dispose of it */
> - stream_display_frame(decoder->base.stream, decoder->cur_frame_msg,
> decoder->out_frame);
> + stream_display_frame(decoder->base.stream, decoder->cur_frame_msg,
> + decoder->mjpeg_cinfo.image_width,
> + decoder->mjpeg_cinfo.image_height,
> + decoder->out_frame);
> spice_msg_in_unref(decoder->cur_frame_msg);
> decoder->cur_frame_msg = NULL;
> decoder->timer_id = 0;
> diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
> index 3155015..3fcf2e2 100644
> --- a/src/channel-display-priv.h
> +++ b/src/channel-display-priv.h
> @@ -135,10 +135,9 @@ struct display_stream {
> uint32_t report_drops_seq_len;
> };
>
> -void stream_get_dimensions(display_stream *st, SpiceMsgIn *frame_msg, int
> *width, int *height);
> guint32 stream_get_time(display_stream *st);
> void stream_dropped_frame_on_playback(display_stream *st);
> -void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg, uint8_t*
> data);
> +void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg, uint32_t
> width, uint32_t height, uint8_t *data);
> uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg, uint8_t **data);
>
>
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 2b8ab4e..31ba574 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1174,26 +1174,6 @@ uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame_msg,
> uint8_t **data)
> }
>
> G_GNUC_INTERNAL
> -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 (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(frame_msg);
> -
> - *width = op->width;
> - *height = op->height;
> - }
> -}
> -
> -G_GNUC_INTERNAL
> guint32 stream_get_time(display_stream *st)
> {
> SpiceSession *session = spice_channel_get_session(st->channel);
> @@ -1210,13 +1190,11 @@ void stream_dropped_frame_on_playback(display_stream
> *st)
> /* main context */
> G_GNUC_INTERNAL
> void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg,
> - uint8_t* data)
> + uint32_t width, uint32_t height, uint8_t *data)
> {
> - int width, height;
> const SpiceRect *dest;
> int stride;
>
> - stream_get_dimensions(st, frame_msg, &width, &height);
> dest = stream_get_dest(st, frame_msg);
>
> stride = width * sizeof(uint32_t);
More information about the Spice-devel
mailing list