[Spice-devel] [PATCH] mjpeg: fix libjpeg assertion

[Christophe Fergeau]

Hi,

I've pushed this patch since it was causing much grief to people running
from git :) I also pushed the jpeg patches to spice-gtk master.
However, we'll have to be careful when we do our next releases, ideally a
spice-gtk with these fixes should be released some time before we make the
next spice-server release to avoid spice-gtk starting to mysteriously
crash.

Any thoughts?

Christophe

On Fri, Jul 29, 2011 at 05:28:35PM +0200, Christophe Fergeau wrote:
> After the changes to add libjpeg-turbo support to spice-server mjpeg
> compression code, it's relatively easy to hit an assertion from
> libjpeg in spice-server about "too few scanlines transferred" when
> the mjpeg streaming code triggers. This assertion brings down qemu,
> which is bad :)
> 
> This is because when we first initialize the mjpeg encoder, we do:
> 
> stream_width = SPICE_ALIGN(src_rect->right - src_rect->left, 2);
> stream_height = SPICE_ALIGN(src_rect->bottom - src_rect->top, 2);
> 
> stream->mjpeg_encoder = mjpeg_encoder_new(stream_width, stream_height);
> 
> and then when we iterate over the image scanlines to feed them to
> libjpeg, we do:
> 
> const int image_height = src->bottom - src->top;
> const int image_width = src->right - src->left;
> 
> for (i = 0; i < image_height; i++) {
>     mjpeg_encoder_encode_scanline(...);
> }
> mjpeg_encoder_end_frame(...);
> 
> When stream_height is odd, the mjpeg_encoder will be created with
> an height that is 1 more than the number of lines we encode. Then
> libjpeg asserts when we tell it we're done with the compression
> while it's still waiting for one more scanline.
> 
> Looking through git history, this rounding seems to be an artifact
> from when we were using ffmpeg for the mjpeg encoding. Since
> spicec and spicy (the latter needs a few fixes) can handle streams
> with odd height/width, the best way to solve this issue is to stop
> rounding up the height and width of the streams we create. This
> even saves some bandwidth :)
> ---
>  server/red_worker.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 6737940..efedc19 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -2417,8 +2417,8 @@ static void red_create_stream(RedWorker *worker, Drawable *drawable)
>  
>      ASSERT(drawable->red_drawable->type == QXL_DRAW_COPY);
>      src_rect = &drawable->red_drawable->u.copy.src_area;
> -    stream_width = SPICE_ALIGN(src_rect->right - src_rect->left, 2);
> -    stream_height = SPICE_ALIGN(src_rect->bottom - src_rect->top, 2);
> +    stream_width = src_rect->right - src_rect->left;
> +    stream_height = src_rect->bottom - src_rect->top;
>  
>      stream->mjpeg_encoder = mjpeg_encoder_new(stream_width, stream_height);
>  
> @@ -7329,10 +7329,10 @@ static int encode_frame (RedWorker *worker, const SpiceRect *src,
>          red_get_image_line(worker, chunks, &offset, &chunk, image_stride);
>      }
>  
> -    const int image_height = src->bottom - src->top;
> -    const int image_width = src->right - src->left;
> +    const unsigned int stream_height = src->bottom - src->top;
> +    const unsigned int stream_width = src->right - src->left;
>  
> -    for (i = 0; i < image_height; i++) {
> +    for (i = 0; i < stream_height; i++) {
>          uint8_t *src_line =
>              (uint8_t *)red_get_image_line(worker, chunks, &offset, &chunk, image_stride);
>  
> @@ -7341,7 +7341,7 @@ static int encode_frame (RedWorker *worker, const SpiceRect *src,
>          }
>  
>          src_line += src->left * mjpeg_encoder_get_bytes_per_pixel(stream->mjpeg_encoder);
> -        if (mjpeg_encoder_encode_scanline(stream->mjpeg_encoder, src_line, image_width) == 0)
> +        if (mjpeg_encoder_encode_scanline(stream->mjpeg_encoder, src_line, stream_width) == 0)
>              return FALSE;
>      }
>  
> @@ -7705,8 +7705,8 @@ static void red_display_marshall_stream_start(DisplayChannel *display_channel,
>  
>      stream_create.src_width = stream->width;
>      stream_create.src_height = stream->height;
> -    stream_create.stream_width = SPICE_ALIGN(stream_create.src_width, 2);
> -    stream_create.stream_height = SPICE_ALIGN(stream_create.src_height, 2);
> +    stream_create.stream_width = stream_create.src_width;
> +    stream_create.stream_height = stream_create.src_height;
>      stream_create.dest = stream->dest_area;
>  
>      if (stream->current) {
> -- 
> 1.7.6
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110801/6265239e/attachment.pgp>