[ros-dev] Need to speak out
Kamil Hornicek
kamil.hornicek at reactos.org
Sat May 8 21:49:01 CEST 2010
Hi,
the only way we can support RLE compressed bitmaps right now is by
decompressing them and use those decompressed bits to create a regular
8bpp/4bpp bitmap.
Your changes to EngCreateBitmap introduced a serious regressions which you
didn't bother to fix for several weeks (+ the font rendering problem etc).
You can't just change some code and then walk away and "let other people
deal with it". a) do it all by yourself properly b) rely on other people's
(incorrect) changes
Maybe you should follow the development more closely so we can avoid these
things.
Regards,
Kamil
----- Original Message -----
From: "Ros Arm" <ros.arm at reactos.org>
To: <ros-dev at reactos.org>
Sent: Saturday, May 08, 2010 9:02 PM
Subject: [ros-dev] Need to speak out
> Hello,
>
> This is the second time that work that someone on the ARM Team has worked
> on has been mostly reverted without any communication with us, and
> incorrect changes have been added to parts of our work.
>
> The Eng function worked on even clearly stated comments such as
> "Compressed surfaces don't have scanlines!", yet this new patch restores
> the old incorrect functionality.
>
> lDelta cannot be anything but != 0 for compressed data such as RLE or
> JPG/PNG! And creating the DIB should not decompress the bits.
>
> Please read
> http://www.tech-archive.net/Archive/Development/microsoft.public.development.device.drivers/2009-05/msg00165.html
> for example.
>
> I do not understand how you can believe that "RLE" has a scan-line. I am
> not a graphics expert by no means, but even I understand this fact: by
> definition scan-lines in an RLE are dynamic, and a scan-line offset table
> contains information about that.
>
> You do not have to take my word for it, as a simple driver test case will
> also show that Windows does not decompress RLE data or set lDelta to any
> other value than 0 with an RLE bitmap. You can find more information on
> RLE at http://www.fileformat.info/mirror/egff/ch09_03.htm.
>
> If you are wondering why there have not been any commits coming lately,
> recent actions such as these as the cause.
>
> -r
>
> Author: khornicek
> Date: Sat May 8 20:09:45 2010
> New Revision: 47134
>
> URL: http://svn.reactos.org/svn/reactos?rev=47134&view=rev
> Log:
> [WIN32K]
> - Bring back support for RLE compressed bitmaps.
> - Merge the decompress functions for 4bb and 8bpp bitmaps to one generic
> function.
> - Simplify SURFMEM_bCreateDib a bit by not allowing PNG/JPEG compression
> at all.
> See issue #5276 for more details.
>
> Modified:
> trunk/reactos/subsystems/win32/win32k/eng/surface.c
>
> Modified: trunk/reactos/subsystems/win32/win32k/eng/surface.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/eng/surface.c?rev=47134&r1=47133&r2=47134&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/eng/surface.c [iso-8859-1]
> (original)
> +++ trunk/reactos/subsystems/win32/win32k/eng/surface.c [iso-8859-1] Sat
> May 8 20:09:45 2010
> @@ -204,48 +204,59 @@
> return NewBitmap;
> }
>
> -VOID Decompress4bpp(SIZEL Size, BYTE *CompressedBits, BYTE
> *UncompressedBits, LONG Delta)
> -{
> - int x = 0;
> - int y = Size.cy - 1;
> - int c;
> - int length;
> - int width = ((Size.cx+1)/2);
> - int height = Size.cy - 1;
> +BOOL DecompressBitmap(SIZEL Size, BYTE *CompressedBits, BYTE
> *UncompressedBits, LONG Delta, ULONG Format)
> +{
> + INT x = 0;
> + INT y = Size.cy - 1;
> + INT c;
> + INT length;
> + INT width;
> + INT height = Size.cy - 1;
> BYTE *begin = CompressedBits;
> BYTE *bits = CompressedBits;
> BYTE *temp;
> - while (y >= 0)
> - {
> - length = *bits++ / 2;
> - if (length)
> - {
> - c = *bits++;
> - while (length--)
> - {
> - if (x >= width) break;
> - temp = UncompressedBits + (((height - y) * Delta) + x);
> - x++;
> - *temp = c;
> - }
> - }
> - else
> - {
> - length = *bits++;
> - switch (length)
> - {
> + INT shift = 0;
> +
> + if (Format == BMF_4RLE)
> + shift = 1;
> + else if(Format != BMF_8RLE)
> + return FALSE;
> +
> + width = ((Size.cx + shift) >> shift);
> +
> + _SEH2_TRY
> + {
> + while (y >= 0)
> + {
> + length = (*bits++) >> shift;
> + if (length)
> + {
> + c = *bits++;
> + while (length--)
> + {
> + if (x >= width) break;
> + temp = UncompressedBits + (((height - y) * Delta) +
> x);
> + x++;
> + *temp = c;
> + }
> + }
> + else
> + {
> + length = *bits++;
> + switch (length)
> + {
> case RLE_EOL:
> x = 0;
> y--;
> break;
> case RLE_END:
> - return;
> + _SEH2_YIELD(return TRUE);
> case RLE_DELTA:
> - x += (*bits++)/2;
> - y -= (*bits++)/2;
> + x += (*bits++) >> shift;
> + y -= (*bits++) >> shift;
> break;
> default:
> - length /= 2;
> + length = length >> shift;
> while (length--)
> {
> c = *bits++;
> @@ -260,69 +271,18 @@
> {
> bits++;
> }
> - }
> - }
> - }
> -}
> -
> -VOID Decompress8bpp(SIZEL Size, BYTE *CompressedBits, BYTE
> *UncompressedBits, LONG Delta)
> -{
> - int x = 0;
> - int y = Size.cy - 1;
> - int c;
> - int length;
> - int width = Size.cx;
> - int height = Size.cy - 1;
> - BYTE *begin = CompressedBits;
> - BYTE *bits = CompressedBits;
> - BYTE *temp;
> - while (y >= 0)
> - {
> - length = *bits++;
> - if (length)
> - {
> - c = *bits++;
> - while (length--)
> - {
> - if (x >= width) break;
> - temp = UncompressedBits + (((height - y) * Delta) + x);
> - x++;
> - *temp = c;
> - }
> - }
> - else
> - {
> - length = *bits++;
> - switch (length)
> - {
> - case RLE_EOL:
> - x = 0;
> - y--;
> - break;
> - case RLE_END:
> - return;
> - case RLE_DELTA:
> - x += *bits++;
> - y -= *bits++;
> - break;
> - default:
> - while (length--)
> - {
> - c = *bits++;
> - if (x < width)
> - {
> - temp = UncompressedBits + (((height - y) *
> Delta) + x);
> - x++;
> - *temp = c;
> - }
> - }
> - if ((bits - begin) & 1)
> - {
> - bits++;
> - }
> - }
> - }
> - }
> + }
> + }
> + }
> + }
> + _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
> + {
> + DPRINT1("Decoding error\n");
> + _SEH2_YIELD(return FALSE);
> + }
> + _SEH2_END;
> +
> + return TRUE;
> }
>
> HBITMAP FASTCALL
> @@ -362,7 +322,7 @@
> pso->cjBits = pso->lDelta * Size.cy;
> UncompressedFormat = BMF_4BPP;
> UncompressedBits = EngAllocMem(FL_ZERO_MEMORY, pso->cjBits,
> TAG_DIB);
> - Decompress4bpp(Size, (BYTE *)Bits, (BYTE *)UncompressedBits,
> pso->lDelta);
> + DecompressBitmap(Size, (BYTE *)Bits, (BYTE *)UncompressedBits,
> pso->lDelta, Format);
> }
> else if (Format == BMF_8RLE)
> {
> @@ -370,7 +330,7 @@
> pso->cjBits = pso->lDelta * Size.cy;
> UncompressedFormat = BMF_8BPP;
> UncompressedBits = EngAllocMem(FL_ZERO_MEMORY, pso->cjBits,
> TAG_DIB);
> - Decompress8bpp(Size, (BYTE *)Bits, (BYTE *)UncompressedBits,
> pso->lDelta);
> + DecompressBitmap(Size, (BYTE *)Bits, (BYTE *)UncompressedBits,
> pso->lDelta, Format);
> }
> else
> {
> @@ -467,6 +427,7 @@
> PSURFACE psurf;
> SIZEL LocalSize;
> BOOLEAN AllocatedLocally = FALSE;
> + PVOID DecompressedBits = NULL;
>
> /*
> * First, check the format so we can get the aligned scanline width.
> @@ -500,17 +461,29 @@
> break;
>
> case BMF_8RLE:
> + ScanLine = (BitmapInfo->Width + 3) & ~3;
> + Compressed = TRUE;
> + break;
> case BMF_4RLE:
> + ScanLine = ((BitmapInfo->Width + 7) & ~7) >> 1;
> + Compressed = TRUE;
> + break;
> +
> case BMF_JPEG:
> case BMF_PNG:
> - Compressed = TRUE;
> - break;
> + ASSERT(FALSE); // ENGDDI shouldn't be creating PNGs for
> drivers ;-)
> + DPRINT1("No support for JPEG and PNG formats\n");
> + return NULL;
>
> default:
> DPRINT1("Invalid bitmap format\n");
> return NULL;
> }
>
> + /* Save local bitmap size */
> + LocalSize.cy = BitmapInfo->Height;
> + LocalSize.cx = BitmapInfo->Width;
> +
> /* Does the device manage its own surface? */
> if (!Bits)
> {
> @@ -519,7 +492,8 @@
> {
> /* Note: we should not be seeing this scenario from ENGDDI */
> ASSERT(FALSE);
> - Size = BitmapInfo->Size;
> + DPRINT1("RLE compressed bitmap requested with no valid bitmap
> bits\n");
> + return NULL;
> }
> else
> {
> @@ -551,6 +525,22 @@
> {
> /* Should not have asked for user memory */
> ASSERT((BitmapInfo->Flags & BMF_USERMEM) == 0);
> +
> + if (Compressed)
> + {
> + DecompressedBits = EngAllocMem(FL_ZERO_MEMORY,
> BitmapInfo->Height * ScanLine, TAG_DIB);
> +
> + if(!DecompressedBits)
> + return NULL;
> +
> + if(!DecompressBitmap(LocalSize, (BYTE *)Bits, (BYTE
> *)DecompressedBits, ScanLine, BitmapInfo->Format))
> + {
> + EngFreeMem(DecompressedBits);
> + return NULL;
> + }
> +
> + BitmapInfo->Format = (BitmapInfo->Format == BMF_4RLE) ?
> BMF_4BPP : BMF_8BPP;
> + }
> }
>
> /* Allocate the actual surface object structure */
> @@ -564,6 +554,8 @@
> else
> EngFreeMem(Bits);
> }
> + if (DecompressedBits)
> + EngFreeMem(DecompressedBits);
> return NULL;
> }
>
> @@ -584,11 +576,9 @@
> pso->fjBitmap = BitmapInfo->Flags & (BMF_TOPDOWN | BMF_UMPDMEM |
> BMF_USERMEM);
>
> /* Save size and type */
> - LocalSize.cy = BitmapInfo->Height;
> - LocalSize.cx = BitmapInfo->Width;
> pso->sizlBitmap = LocalSize;
> pso->iType = STYPE_BITMAP;
> -
> +
> /* Device-managed surface, no flags or dimension */
> pso->dhsurf = 0;
> pso->dhpdev = NULL;
> @@ -599,48 +589,28 @@
> psurf->hSecure = NULL;
> psurf->hDIBSection = NULL;
> psurf->flHooks = 0;
> -
> +
> /* Set bits */
> - pso->pvBits = Bits;
> -
> - /* Check for bitmap type */
> - if (!Compressed)
> - {
> - /* Number of bits is based on the height times the scanline */
> - pso->cjBits = BitmapInfo->Height * ScanLine;
> - if (BitmapInfo->Flags & BMF_TOPDOWN)
> - {
> - /* For topdown, the base address starts with the bits */
> - pso->pvScan0 = pso->pvBits;
> - pso->lDelta = ScanLine;
> - }
> - else
> - {
> - /* Otherwise we start with the end and go up */
> - pso->pvScan0 = (PVOID)((ULONG_PTR)pso->pvBits + pso->cjBits -
> ScanLine);
> - pso->lDelta = -ScanLine;
> - }
> + if(Compressed)
> + pso->pvBits = DecompressedBits;
> + else
> + pso->pvBits = Bits;
> +
> + /* Number of bits is based on the height times the scanline */
> + pso->cjBits = BitmapInfo->Height * ScanLine;
> + if (BitmapInfo->Flags & BMF_TOPDOWN)
> + {
> + /* For topdown, the base address starts with the bits */
> + pso->pvScan0 = pso->pvBits;
> + pso->lDelta = ScanLine;
> }
> else
> {
> - /* Compressed surfaces don't have scanlines! */
> - pso->lDelta = 0;
> - pso->cjBits = BitmapInfo->Size;
> -
> - /* Check for JPG or PNG */
> - if ((BitmapInfo->Format != BMF_JPEG) && (BitmapInfo->Format !=
> BMF_PNG))
> - {
> - /* Wherever the bit data is */
> - pso->pvScan0 = pso->pvBits;
> - }
> - else
> - {
> - /* Fancy formats don't use a base address */
> - pso->pvScan0 = NULL;
> - ASSERT(FALSE); // ENGDDI shouldn't be creating PNGs for
> drivers ;-)
> - }
> - }
> -
> + /* Otherwise we start with the end and go up */
> + pso->pvScan0 = (PVOID)((ULONG_PTR)pso->pvBits + pso->cjBits -
> ScanLine);
> + pso->lDelta = -ScanLine;
> + }
> +
> /* Finally set the handle and uniq */
> pso->hsurf = (HSURF)psurf->BaseObject.hHmgr;
> pso->iUniq = 0;
>
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
More information about the Ros-dev
mailing list