[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