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

[Hans de Goede]

Hi,

On 07/29/2011 05:28 PM, 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 :)

Hmm, I assume you've tested this with odd sized images? The reason
I'm asking is that JPEG compression usually happens by converting
from RGB to YUV color space and in the YUV colorspace the UV
components are sub-sampled 2x2 (iow the UV resolution is half
that of the Y/image resolution).

The usual trick for odd sized images is to duplicate the last row /
column to make them even sized. I could not find anything about
this in the libjpeg documentation, so maybe libjpeg take cares
about this itself?

While on the subject of libjpeg, as this assert shows we should be
defining our own jpeg error_exit handler, so that in case of
an error we can simply drop the image update rather then kill
the entire vm. This can be done by losing longjmp (ugly I know),
see the init_libjpeg_cinfo function in:
http://git.linuxtv.org/v4l-utils.git/blob/HEAD:/lib/libv4lconvert/jpeg.c

Also I think there are several opportunities for more optimization
with our libjpeg code. The first is to not include and parse the headers
each frame, this will save both bandwidth and CPU for parsing the
huffman headers and generating lookup tables, see the
"Abbreviated datastreams and multiple images" section in
/usr/share/doc/libjpeg-turbo-devel-1.1.0/libjpeg.txt

The second is to define a new jpeg-rgb format where we use jpeg
compression straight on rgb, rather then first converting it
to yuv colorspace (this happens inside libjpeg, but can be
overridden) this will lead to a slight increase in bandwidth
usage, but also a great drop in cpu usage.

Regards,

Hans