[PATCH] windowscodecs: Move JPEG frame image data initialization from Frame::CopyPixels to Decoder::Initialize.

[Dmitry Timoshkov]

This is how PNG decoder does things, and it avoids image data corruption
in some cases (presumably when libjpeg reuses existing scanline data).

Signed-off-by: Dmitry Timoshkov 
---
 dlls/windowscodecs/jpegformat.c | 152 ++++++++++++--------------------
 1 file changed, 56 insertions(+), 96 deletions(-)

diff --git a/dlls/windowscodecs/jpegformat.c b/dlls/windowscodecs/jpegformat.c
index 54f77a7220..1df898f28c 100644
--- a/dlls/windowscodecs/jpegformat.c
+++ b/dlls/windowscodecs/jpegformat.c
@@ -50,6 +50,7 @@

#include "wincodecs_private.h"

+#include "wine/heap.h"
 #include "wine/debug.h"
 #include "wine/library.h"

@@ -155,6 +156,7 @@ typedef struct {
     struct jpeg_error_mgr jerr;
     struct jpeg_source_mgr source_mgr;
     BYTE source_buffer[1024];
+    UINT bpp, stride;
     BYTE *image_data;
     CRITICAL_SECTION lock;
 } JpegDecoder;
@@ -303,6 +305,8 @@ static HRESULT WINAPI JpegDecoder_Initialize(IWICBitmapDecoder *iface, IStream *
     int ret;
     LARGE_INTEGER seek;
     jmp_buf jmpbuf;
+    UINT data_size, i;
+
     TRACE("(%p,%p,%u)\n", iface, pIStream, cacheOptions);

EnterCriticalSection(&This->lock);
@@ -381,6 +385,55 @@ static HRESULT WINAPI JpegDecoder_Initialize(IWICBitmapDecoder *iface, IStream *
         return E_FAIL;
     }

+    if (This->cinfo.out_color_space == JCS_GRAYSCALE) This->bpp = 8;
+    else if (This->cinfo.out_color_space == JCS_CMYK) This->bpp = 32;
+    else This->bpp = 24;
+
+    This->stride = (This->bpp * This->cinfo.output_width + 7) / 8;
+    data_size = This->stride * This->cinfo.output_height;
+
+    This->image_data = heap_alloc(data_size);
+    if (!This->image_data)
+    {
+        LeaveCriticalSection(&This->lock);
+        return E_OUTOFMEMORY;
+    }
+
+    while (This->cinfo.output_scanline < This->cinfo.output_height)
+    {
+        UINT first_scanline = This->cinfo.output_scanline;
+        UINT max_rows;
+        JSAMPROW out_rows[4];
+        JDIMENSION ret;
+
+        max_rows = min(This->cinfo.output_height-first_scanline, 4);
+        for (i=0; i<max_rows; i++)
+            out_rows[i] = This->image_data + This->stride * (first_scanline+i);
+
+        ret = pjpeg_read_scanlines(&This->cinfo, out_rows, max_rows);
+        if (ret == 0)
+        {
+            ERR("read_scanlines failed\n");
+            LeaveCriticalSection(&This->lock);
+            return E_FAIL;
+        }
+    }
+
+    if (This->bpp == 24)
+    {
+        /* libjpeg gives us RGB data and we want BGR, so byteswap the data */
+        reverse_bgr8(3, This->image_data,
+            This->cinfo.output_width, This->cinfo.output_height,
+            This->stride);
+    }
+
+    if (This->cinfo.out_color_space == JCS_CMYK && This->cinfo.saw_Adobe_marker)
+    {
+        /* Adobe JPEG's have inverted CMYK data. */
+        for (i=0; i<data_size; i++)
+            This->image_data[i] ^= 0xff;
+    }
+
     This->initialized = TRUE;

LeaveCriticalSection(&This->lock);
@@ -590,104 +643,11 @@ static HRESULT WINAPI JpegDecoder_Frame_CopyPixels(IWICBitmapFrameDecode *iface,
     const WICRect *prc, UINT cbStride, UINT cbBufferSize, BYTE *pbBuffer)
 {
     JpegDecoder *This = impl_from_IWICBitmapFrameDecode(iface);
-    UINT bpp;
-    UINT stride;
-    UINT data_size;
-    UINT max_row_needed;
-    jmp_buf jmpbuf;
-    WICRect rect;
-    TRACE("(%p,%s,%u,%u,%p)\n", iface, debug_wic_rect(prc), cbStride, cbBufferSize, pbBuffer);
-
-    if (!prc)
-    {
-        rect.X = 0;
-        rect.Y = 0;
-        rect.Width = This->cinfo.output_width;
-        rect.Height = This->cinfo.output_height;
-        prc = &rect;
-    }
-    else
-    {
-        if (prc->X < 0 || prc->Y < 0 || prc->X+prc->Width > This->cinfo.output_width ||
-            prc->Y+prc->Height > This->cinfo.output_height)
-            return E_INVALIDARG;
-    }
-
-    if (This->cinfo.out_color_space == JCS_GRAYSCALE) bpp = 8;
-    else if (This->cinfo.out_color_space == JCS_CMYK) bpp = 32;
-    else bpp = 24;
-
-    stride = (bpp * This->cinfo.output_width + 7) / 8;
-    data_size = stride * This->cinfo.output_height;
-
-    max_row_needed = prc->Y + prc->Height;
-    if (max_row_needed > This->cinfo.output_height) return E_INVALIDARG;
-
-    EnterCriticalSection(&This->lock);

-    if (!This->image_data)
-    {
-        This->image_data = HeapAlloc(GetProcessHeap(), 0, data_size);
-        if (!This->image_data)
-        {
-            LeaveCriticalSection(&This->lock);
-            return E_OUTOFMEMORY;
-        }
-    }
-
-    This->cinfo.client_data = jmpbuf;
-
-    if (setjmp(jmpbuf))
-    {
-        LeaveCriticalSection(&This->lock);
-        return E_FAIL;
-    }
-
-    while (max_row_needed > This->cinfo.output_scanline)
-    {
-        UINT first_scanline = This->cinfo.output_scanline;
-        UINT max_rows;
-        JSAMPROW out_rows[4];
-        UINT i;
-        JDIMENSION ret;
-
-        max_rows = min(This->cinfo.output_height-first_scanline, 4);
-        for (i=0; i<max_rows; i++)
-            out_rows[i] = This->image_data + stride * (first_scanline+i);
-
-        ret = pjpeg_read_scanlines(&This->cinfo, out_rows, max_rows);
-
-        if (ret == 0)
-        {
-            ERR("read_scanlines failed\n");
-            LeaveCriticalSection(&This->lock);
-            return E_FAIL;
-        }
-
-        if (bpp == 24)
-        {
-            /* libjpeg gives us RGB data and we want BGR, so byteswap the data */
-            reverse_bgr8(3, This->image_data + stride * first_scanline,
-                This->cinfo.output_width, This->cinfo.output_scanline - first_scanline,
-                stride);
-        }
-
-        if (This->cinfo.out_color_space == JCS_CMYK && This->cinfo.saw_Adobe_marker)
-        {
-            DWORD *pDwordData = (DWORD*) (This->image_data + stride * first_scanline);
-            DWORD *pDwordDataEnd = (DWORD*) (This->image_data + This->cinfo.output_scanline * stride);
-
-            /* Adobe JPEG's have inverted CMYK data. */
-            while(pDwordData < pDwordDataEnd)
-                *pDwordData++ ^= 0xffffffff;
-        }
-
-    }
-
-    LeaveCriticalSection(&This->lock);
+    TRACE("(%p,%s,%u,%u,%p)\n", iface, debug_wic_rect(prc), cbStride, cbBufferSize, pbBuffer);

-    return copy_pixels(bpp, This->image_data,
-        This->cinfo.output_width, This->cinfo.output_height, stride,
+    return copy_pixels(This->bpp, This->image_data,
+        This->cinfo.output_width, This->cinfo.output_height, This->stride,
         prc, cbStride, cbBufferSize, pbBuffer);
 }

[Vincent Povirk]

I'm not sure about this. According to MSDN, decoders are supposed to
do the decoding incrementally in CopyPixels, assuming the user reads
rows from the top. In theory, the other decoders that do all the work
in Initialize should be changed to do it this way.

Can you explain why this change fixes the problem you observed? Is
libjpeg broken? Are we using the API incorrectly? Could there be a
memory bug somewhere else causing strange behavior?

[Dmitry Timoshkov]

Hi Vincent,

Vincent Povirk  wrote:
> I'm not sure about this. According to MSDN, decoders are supposed to
> do the decoding incrementally in CopyPixels, assuming the user reads
> rows from the top. In theory, the other decoders that do all the work
> in Initialize should be changed to do it this way.
>
> Can you explain why this change fixes the problem you observed? Is
> libjpeg broken? Are we using the API incorrectly? Could there be a
> memory bug somewhere else causing strange behavior?

It was quite a bit of time ago when I was looking into this, so I may not
recall all the details. This patch fixed several problems: first one is
with decoding the JPEG data, and that was a real bug causing corrupted
images being produced, and I haven't digged into the source of it inside
of libjpeg; another one is that Initialize() doesn't really try to decode
the image data, and that leads to CopyPixels() failures later on when it's
not really expected (e.g. for broken images), and that also makes QueryCapability()
report success while subsequent CopyPixels() fails. I have a test program
that converts any WIC image to other WIC formats to test converting images
from many different sources (mainly from libtiff/libjpeg samples) and that
was one of the problems found during my testing.

[Vincent Povirk]

Signed-off-by: Vincent Povirk