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

[Christophe Fergeau]

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