[Spice-devel] [PATCH 1/2] server: Remove the width and height parameters of encode_frame()

[Victor Toso]

Hi,

On Wed, Feb 24, 2016 at 06:30:34PM +0100, Francois Gouget wrote:
> They always match the size of the source bitmap area:
> * because for sized frames they are set from the source area,
> * and because otherwise the initial stream's size is that of regular
>   frames by definition.
> Note also that we do not lose any flexibility since the source area
> parameter can be used to encode a specific bitmap area.
>
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
>  server/dcc-send.c      | 24 +++++++-----------------
>  server/mjpeg-encoder.c |  8 +++++---
>  server/mjpeg-encoder.h |  2 +-
>  3 files changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/server/dcc-send.c b/server/dcc-send.c
> index 4180cc1..e280383 100644
> --- a/server/dcc-send.c
> +++ b/server/dcc-send.c
> @@ -1658,7 +1658,6 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>      SpiceImage *image;
>      uint32_t frame_mm_time;
>      int n;
> -    int width, height;
>      int ret;
>
>      if (!stream) {
> @@ -1673,21 +1672,13 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>          return FALSE;
>      }
>
> -    if (drawable->sized_stream) {
> -        if (red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM)) {
> -            SpiceRect *src_rect = &drawable->red_drawable->u.copy.src_area;
> -
> -            width = src_rect->right - src_rect->left;
> -            height = src_rect->bottom - src_rect->top;
This.

I think it makes sense and could actually be the proper fix for 1274575

You can see on that mjpeg_encoder_encode_frame() uses this width and
heigh for jpeg_start_compress() but on encode_frame() it uses the
drawable->red_drawable->u.copy.src_area directly.

For some reason, it was entering in the else below and using
stream->width and stream->height directly.

I recall doing tests here but I'll try this tomorrow.

Thank you for this patches, it does simplify quite a bit.

Cheers,
  toso

> -        } else {
> -            return FALSE;
> -        }
> -    } else {
> -        width = stream->width;
> -        height = stream->height;
> +    if (drawable->sized_stream &&
> +        !red_channel_client_test_remote_cap(rcc, SPICE_DISPLAY_CAP_SIZED_STREAM)) {
> +        return FALSE;
>      }
>
>      StreamAgent *agent = &dcc->stream_agents[get_stream_id(display, stream)];
> +    const SpiceRect *src_area = &drawable->red_drawable->u.copy.src_area;
>      uint64_t time_now = spice_get_monotonic_time_ns();
>      size_t outbuf_size;
>
> @@ -1707,8 +1698,7 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>                          reds_get_mm_time();
>      outbuf_size = dcc->send_data.stream_outbuf_size;
>      ret = mjpeg_encoder_encode_frame(agent->mjpeg_encoder,
> -                                     &image->u.bitmap, width, height,
> -                                     &drawable->red_drawable->u.copy.src_area,
> +                                     &image->u.bitmap, src_area,
>                                       stream->top_down, frame_mm_time,
>                                      &dcc->send_data.stream_outbuf,
>                                       &outbuf_size, &n);
> @@ -1747,8 +1737,8 @@ static int red_marshall_stream_data(RedChannelClient *rcc,
>          stream_data.base.id = get_stream_id(display, stream);
>          stream_data.base.multi_media_time = frame_mm_time;
>          stream_data.data_size = n;
> -        stream_data.width = width;
> -        stream_data.height = height;
> +        stream_data.width = src_area->right - src_area->left;
> +        stream_data.height = src_area->bottom - src_area->top;
>          stream_data.dest = drawable->red_drawable->bbox;
>  
>          spice_debug("stream %d: sized frame: dest ==> ", stream_data.base.id);
> diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> index 3bd1a04..1086b53 100644
> --- a/server/mjpeg-encoder.c
> +++ b/server/mjpeg-encoder.c
> @@ -925,15 +925,17 @@ static int encode_frame(MJpegEncoder *encoder, const SpiceRect *src,
>  }
>  
>  int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
> -                               const SpiceBitmap *bitmap, int width, int height,
> +                               const SpiceBitmap *bitmap,
>                                 const SpiceRect *src,
>                                 int top_down, uint32_t frame_mm_time,
>                                 uint8_t **outbuf, size_t *outbuf_size,
>                                 int *data_size)
>  {
>      int ret = mjpeg_encoder_start_frame(encoder, bitmap->format,
> -                                    width, height, outbuf, outbuf_size,
> -                                    frame_mm_time);
> +                                        src->right - src->left,
> +                                        src->bottom - src->top,
> +                                        outbuf, outbuf_size,
> +                                        frame_mm_time);
>      if (ret != MJPEG_ENCODER_FRAME_ENCODE_DONE) {
>          return ret;
>      }
> diff --git a/server/mjpeg-encoder.h b/server/mjpeg-encoder.h
> index 8223a7f..31e7cb3 100644
> --- a/server/mjpeg-encoder.h
> +++ b/server/mjpeg-encoder.h
> @@ -54,7 +54,7 @@ MJpegEncoder *mjpeg_encoder_new(uint64_t starting_bit_rate,
>  void mjpeg_encoder_destroy(MJpegEncoder *encoder);
>  
>  int mjpeg_encoder_encode_frame(MJpegEncoder *encoder,
> -                               const SpiceBitmap *bitmap, int width, int height,
> +                               const SpiceBitmap *bitmap,
>                                 const SpiceRect *src,
>                                 int top_down, uint32_t frame_mm_time,
>                                 uint8_t **outbuf, size_t *outbuf_size,
> -- 
> 2.7.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel