Recent Bootvid Commit

[Ros Arm]

Hello!

While the state of the original code is horrendous, and removing it was a step in the good direction, this simplified implementation omits many of the optimizations and specific VGA tricks that the original "author" of the code had intended to duplicate.

If you'd like, I would be open to talking to eVb (we both have some experience with VGA programming) to create an efficient 4BitBlt routine similar to the original one that has been removed, but one that actually has basis in actual fact and isn't of the nature this original implementation was.

I can see the original code was converting packed data into per-plane (planar) data and had special edge handling, but it looks like the original author did not realize this, and most of the modulo-and-shifts appear as bitwise ANDs (compiler optimizations), which is just plain confusing.

Since the rest of bootvid is probably the same "quality", we could take a good look at it, both from a coding and legal standpoint, if you'd like.

-r

Author: gschneider
Date: Sun May 30 01:54:47 2010
New Revision: 47431

URL: http://svn.reactos.org/svn/reactos?rev=47431&view=rev
Log:
[BOOTVID] Dramatically simplify 4bpp blitting routine
See issue #5103 for more details.

Modified:
   trunk/reactos/drivers/base/bootvid/i386/vga.c

Modified: trunk/reactos/drivers/base/bootvid/i386/vga.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/base/bootvid/i386/v...
==============================================================================
--- trunk/reactos/drivers/base/bootvid/i386/vga.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/base/bootvid/i386/vga.c [iso-8859-1] Sun May 30 01:54:47 2010
@@ -402,33 +402,20 @@
       IN ULONG BitsPerPixel,
       IN ULONG Delta)
{
-    ULONG LeftAnd, LeftShifted, LeftPlusOne, LeftPos;
-    ULONG lMask, rMask;
-    UCHAR NotlMask;
-    ULONG Distance;
-    ULONG DistanceMinusLeftBpp;
-    ULONG SomeYesNoFlag, SomeYesNoFlag2;
-    PUCHAR PixelPosition, m;
-    PUCHAR i, k;
-    ULONG j;
-    ULONG x;
-    ULONG Plane;
-    UCHAR LeftArray[84];
-    PUCHAR CurrentLeft;
-    PUCHAR l;
-    ULONG LoopCount;
-    UCHAR pMask, PlaneShift;
-    BOOLEAN Odd;
-    UCHAR Value;
+    ULONG sx, dx, dy;
+    UCHAR color;
+    ULONG offset = 0;
+    const ULONG Bottom = Top + Height;
+    const ULONG Right = Left + Width;

/* Check if the buffer isn't 4bpp */
    if (BitsPerPixel != 4)
    {
        /* FIXME: TODO */
        DbgPrint("Unhandled BitBlt\n"
-                 "%lxx%lx @ (%lx,%lx)\n"
-                 "Bits Per Pixel %lx\n"
-                 "Buffer: %p. Delta: %lx\n",
+                 "%lux%lu @ (%lu|%lu)\n"
+                 "Bits Per Pixel %lu\n"
+                 "Buffer: %p. Delta: %lu\n",
                 Width,
                 Height,
                 Left,
@@ -439,181 +426,28 @@
        return;
    }

-    /* Get the masks and other values */
-    LeftAnd = Left & 0x7;
-    lMask = lMaskTable[LeftAnd];
-    Distance = Width + Left;
-    rMask = rMaskTable[(Distance - 1) & 0x7];
-    Left >>= 3;
-
-    /* Set some values */
-    SomeYesNoFlag = FALSE;
-    SomeYesNoFlag2 = FALSE;
-    Distance = (Distance - 1) >> 3;
-    DistanceMinusLeftBpp = Distance - Left;
-
-    /* Check if the distance is equal to the left position and add the masks */
-    if (Left == Distance) lMask += rMask;
-
-    /* Check if there's no distance offset */
-    if (DistanceMinusLeftBpp)
-    {
-        /* Set the first flag on */
-        SomeYesNoFlag = TRUE;
-
-        /* Decrease offset and check if we still have one */
-        if (--DistanceMinusLeftBpp)
-        {
-            /* Still have a distance offset */
-            SomeYesNoFlag2 = TRUE;
-        }
-    }
-
-    /* Calculate initial pixel position */
-    PixelPosition = (PUCHAR)VgaBase + (Top * 80) + Left;
-
-    /* Set loop buffer variable */
-    i = Buffer;
-
-    /* Switch to mode 0 */
-    ReadWriteMode(0);
-
-    /* Leave now if the height is 0 */
-    if (Height <= 0) return;
-
-    /* Set more weird values */
-    CurrentLeft = &LeftArray[Left];
-    NotlMask = ~(UCHAR)lMask;
-    LeftPlusOne = Left + 1;
-    LeftShifted = (lMask << 8) | 8;
-    j = Height;
-
-    /* Start the height loop */
+    /* 4bpp blitting */
+    dy = Top;
    do
    {
-        /* Start the plane loop */
-        Plane = 0;
+        sx = 0;
        do
        {
-            /* Clear the current value */
-            *CurrentLeft = 0;
-            LoopCount = 0;
-
-            /* Set the buffer loop variable for this loop */
-            k = i;
-
-            /* Calculate plane shift and pixel mask */
-            PlaneShift = 1 << Plane;
-            pMask = PixelMask[LeftAnd];
-
-            /* Check if we have a width */
-            if (Width > 0)
-            {
-                /* Loop it */
-                l = CurrentLeft;
-                x = Width;
-                do
-                {
-                    /* Check if we're odd and increase the loop count */
-                    Odd = LoopCount & 1 ? TRUE : FALSE;
-                    LoopCount++;
-                    if (Odd)
-                    {
-                        /* Check for the plane shift */
-                        if (*k & PlaneShift)
-                        {
-                            /* Write the pixel mask */
-                            *l |= pMask;
-                        }
-
-                        /* Increase buffer position */
-                        k++;
-                    }
-                    else
-                    {
-                        /* Check for plane shift */
-                        if ((*k >> 4) & PlaneShift)
-                        {
-                            /* Write the pixel mask */
-                            *l |= pMask;
-                        }
-                    }
-
-                    /* Shift the pixel mask */
-                    pMask >>= 1;
-                    if (!pMask)
-                    {
-                        /* Move to the next current left position and clear it */
-                        l++;
-                        *l = 0;
-
-                        /* Set the pixel mask to 0x80 */
-                        pMask = 0x80;
-                    }
-                } while (--x);
-            }
-
-            /* Set the plane value */
-            __outpw(0x3C4, (1 << (Plane + 8) | 2));
-
-            /* Select the bitmask register and write the mask */
-            __outpw(0x3CE, (USHORT)LeftShifted);
-
-            /* Read the current Pixel value */
-            Value = READ_REGISTER_UCHAR(PixelPosition);
-
-            /* Add our mask */
-            Value = (Value & NotlMask) | *CurrentLeft;
-
-            /* Set current left for the loop, and write new pixel value */
-            LeftPos = LeftPlusOne;
-            WRITE_REGISTER_UCHAR(PixelPosition, Value);
-
-            /* Set loop pixel position and check if we should loop */
-            m = PixelPosition + 1;
-            if (SomeYesNoFlag2)
-            {
-                /* Set the bitmask to 0xFF for all 4 planes */
-                __outpw(0x3CE, 0xFF08);
-
-                /* Check if we have any distance left */
-                if (DistanceMinusLeftBpp > 0)
-                {
-                    /* Start looping it */
-                    x = DistanceMinusLeftBpp;
-                    do
-                    {
-                        /* Write the value */
-                        WRITE_REGISTER_UCHAR(m, LeftArray[LeftPos]);
-
-                        /* Go to the next position */
-                        m++;
-                        LeftPos++;
-                    } while (--x);
-                }
-            }
-
-            /* Check if the first flag is on */
-            if (SomeYesNoFlag)
-            {
-                /* Set the mask value */
-                __outpw(0x3CE, (rMask << 8) | 8);
-
-                /* Read the current Pixel value */
-                Value = READ_REGISTER_UCHAR(m);
-
-                /* Add our mask */
-                Value = (Value & ~(UCHAR)rMask) | LeftArray[LeftPos];
-
-                /* Set current left for the loop, and write new pixel value */
-                WRITE_REGISTER_UCHAR(m, Value);
-            }
-        } while (++Plane < 4);
-
-        /* Update pixel position, buffer and height */
-        PixelPosition += 80;
-        i += Delta;
-    } while (--j);
+            /* Extract color */
+            color = Buffer[offset + sx];
+
+            /* Calc destination x */
+            dx = Left + (sx << 1);
+
+            /* Set two pixels */
+            SetPixel(dx, dy, color >> 4);
+            SetPixel(dx + 1, dy, color & 0x0F);
+
+            sx++;
+        } while (dx < Right);
+        offset += Delta;
+        dy++;
+    } while (dy < Bottom);
}

VOID

[Gregor Schneider]

Hey Ros Arm,

if you feel like extending and optimizing that code, please feel free to do
so.
> From the old code and the comments I could only guess what it was trying to

do except the standard blit. Most parts of it worked by chance, so I added a
very basic (and common sense) implementation and tested it with plain
non-RLE bitmaps by converting those in ntoskrnl\inbv\logo\ for testing.

At the moment this routine is not even used as it seems, the ntoskrnl
bitmaps are RLE coded by default. Those are processed by the RLEBitBlt
function in the same file. This functions looks a bit better, but could
still require some changes here and there.

Best regards,
Gregor

2010/5/30 Ros Arm [e-mail]
> Hello!
>
> While the state of the original code is horrendous, and removing it was a
> step in the good direction, this simplified implementation omits many of the
> optimizations and specific VGA tricks that the original "author" of the code
> had intended to duplicate.
>
> If you'd like, I would be open to talking to eVb (we both have some
> experience with VGA programming) to create an efficient 4BitBlt routine
> similar to the original one that has been removed, but one that actually has
> basis in actual fact and isn't of the nature this original implementation
> was.
>
> I can see the original code was converting packed data into per-plane
> (planar) data and had special edge handling, but it looks like the original
> author did not realize this, and most of the modulo-and-shifts appear as
> bitwise ANDs (compiler optimizations), which is just plain confusing.
>
> Since the rest of bootvid is probably the same "quality", we could take a
> good look at it, both from a coding and legal standpoint, if you'd like.
>
> -r
>
> Author: gschneider
> Date: Sun May 30 01:54:47 2010
> New Revision: 47431
>
> URL: http://svn.reactos.org/svn/reactos?rev=47431&view=rev
> Log:
> [BOOTVID] Dramatically simplify 4bpp blitting routine
> See issue #5103 for more details.
>
> Modified:
>   trunk/reactos/drivers/base/bootvid/i386/vga.c
>
> Modified: trunk/reactos/drivers/base/bootvid/i386/vga.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/base/bootvid/i386/v...
>
> ==============================================================================
> --- trunk/reactos/drivers/base/bootvid/i386/vga.c [iso-8859-1] (original)
> +++ trunk/reactos/drivers/base/bootvid/i386/vga.c [iso-8859-1] Sun May 30
> 01:54:47 2010
> @@ -402,33 +402,20 @@
>       IN ULONG BitsPerPixel,
>       IN ULONG Delta)
> {
> ULONG LeftAnd, LeftShifted, LeftPlusOne, LeftPos;
> ULONG lMask, rMask;
> UCHAR NotlMask;
> ULONG Distance;
> ULONG DistanceMinusLeftBpp;
> ULONG SomeYesNoFlag, SomeYesNoFlag2;
> PUCHAR PixelPosition, m;
> PUCHAR i, k;
> ULONG j;
> ULONG x;
> ULONG Plane;
> UCHAR LeftArray[84];
> PUCHAR CurrentLeft;
> PUCHAR l;
> ULONG LoopCount;
> UCHAR pMask, PlaneShift;
> BOOLEAN Odd;
> UCHAR Value;
>
> ULONG sx, dx, dy;
>
> UCHAR color;
>
> ULONG offset = 0;
>
> const ULONG Bottom = Top + Height;
>
> const ULONG Right = Left + Width;
>
> /* Check if the buffer isn't 4bpp */
> if (BitsPerPixel != 4)
> {
>   /* FIXME: TODO */
>   DbgPrint("Unhandled BitBlt\n"
>             "%lxx%lx @ (%lx,%lx)\n"
>             "Bits Per Pixel %lx\n"
>             "Buffer: %p. Delta: %lx\n",
>             "%lux%lu @ (%lu|%lu)\n"
>             "Bits Per Pixel %lu\n"
>             "Buffer: %p. Delta: %lu\n",
>           Width,
>           Height,
>           Left,
>
> @@ -439,181 +426,28 @@
>        return;
>    }
> /* Get the masks and other values */
> LeftAnd = Left & 0x7;
> lMask = lMaskTable[LeftAnd];
> Distance = Width + Left;
> rMask = rMaskTable[(Distance - 1) & 0x7];
> Left >>= 3;
> /* Set some values */
> SomeYesNoFlag = FALSE;
> SomeYesNoFlag2 = FALSE;
> Distance = (Distance - 1) >> 3;
> DistanceMinusLeftBpp = Distance - Left;
> /* Check if the distance is equal to the left position and add the
>
> masks */
> if (Left == Distance) lMask += rMask;
> /* Check if there's no distance offset */
> if (DistanceMinusLeftBpp)
> {
>    /* Set the first flag on */
>    SomeYesNoFlag = TRUE;
>    /* Decrease offset and check if we still have one */
>    if (--DistanceMinusLeftBpp)
>    {
>        /* Still have a distance offset */
>        SomeYesNoFlag2 = TRUE;
>    }
> }
> /* Calculate initial pixel position */
> PixelPosition = (PUCHAR)VgaBase + (Top * 80) + Left;
> /* Set loop buffer variable */
> i = Buffer;
> /* Switch to mode 0 */
> ReadWriteMode(0);
> /* Leave now if the height is 0 */
> if (Height <= 0) return;
> /* Set more weird values */
> CurrentLeft = &LeftArray[Left];
> NotlMask = ~(UCHAR)lMask;
> LeftPlusOne = Left + 1;
> LeftShifted = (lMask << 8) | 8;
> j = Height;
> /* Start the height loop */
> /* 4bpp blitting */
> dy = Top;
> do
> {
>    /* Start the plane loop */
>    Plane = 0;
>    sx = 0;
>  do
>  {
>        /* Clear the current value */
>        *CurrentLeft = 0;
>        LoopCount = 0;
>        /* Set the buffer loop variable for this loop */
>        k = i;
>        /* Calculate plane shift and pixel mask */
>        PlaneShift = 1 << Plane;
>        pMask = PixelMask[LeftAnd];
>        /* Check if we have a width */
>        if (Width > 0)
>        {
>            /* Loop it */
>            l = CurrentLeft;
>            x = Width;
>            do
>            {
>                /* Check if we're odd and increase the loop count */
>                Odd = LoopCount & 1 ? TRUE : FALSE;
>                LoopCount++;
>                if (Odd)
>                {
>                    /* Check for the plane shift */
>                    if (*k & PlaneShift)
>                    {
>                        /* Write the pixel mask */
>                        *l |= pMask;
>                    }
>                    /* Increase buffer position */
>                    k++;
>                }
>                else
>                {
>                    /* Check for plane shift */
>                    if ((*k >> 4) & PlaneShift)
>                    {
>                        /* Write the pixel mask */
>                        *l |= pMask;
>                    }
>                }
>                /* Shift the pixel mask */
>                pMask >>= 1;
>                if (!pMask)
>                {
>                    /* Move to the next current left position and
>
> clear it */
>                    l++;
>                    *l = 0;
>                    /* Set the pixel mask to 0x80 */
>                    pMask = 0x80;
>                }
>            } while (--x);
>        }
>        /* Set the plane value */
>        __outpw(0x3C4, (1 << (Plane + 8) | 2));
>        /* Select the bitmask register and write the mask */
>        __outpw(0x3CE, (USHORT)LeftShifted);
>        /* Read the current Pixel value */
>        Value = READ_REGISTER_UCHAR(PixelPosition);
>        /* Add our mask */
>        Value = (Value & NotlMask) | *CurrentLeft;
>        /* Set current left for the loop, and write new pixel value */
>        LeftPos = LeftPlusOne;
>        WRITE_REGISTER_UCHAR(PixelPosition, Value);
>        /* Set loop pixel position and check if we should loop */
>        m = PixelPosition + 1;
>        if (SomeYesNoFlag2)
>        {
>            /* Set the bitmask to 0xFF for all 4 planes */
>            __outpw(0x3CE, 0xFF08);
>            /* Check if we have any distance left */
>            if (DistanceMinusLeftBpp > 0)
>            {
>                /* Start looping it */
>                x = DistanceMinusLeftBpp;
>                do
>                {
>                    /* Write the value */
>                    WRITE_REGISTER_UCHAR(m, LeftArray[LeftPos]);
>                    /* Go to the next position */
>                    m++;
>                    LeftPos++;
>                } while (--x);
>            }
>        }
>        /* Check if the first flag is on */
>        if (SomeYesNoFlag)
>        {
>            /* Set the mask value */
>            __outpw(0x3CE, (rMask << 8) | 8);
>            /* Read the current Pixel value */
>            Value = READ_REGISTER_UCHAR(m);
>            /* Add our mask */
>            Value = (Value & ~(UCHAR)rMask) | LeftArray[LeftPos];
>            /* Set current left for the loop, and write new pixel
>
> value */
>            WRITE_REGISTER_UCHAR(m, Value);
>        }
>    } while (++Plane < 4);
>    /* Update pixel position, buffer and height */
>    PixelPosition += 80;
>    i += Delta;
> } while (--j);
>        /* Extract color */
>        color = Buffer[offset + sx];
>        /* Calc destination x */
>        dx = Left + (sx << 1);
>        /* Set two pixels */
>        SetPixel(dx, dy, color >> 4);
>        SetPixel(dx + 1, dy, color & 0x0F);
>        sx++;
>    } while (dx < Right);
>    offset += Delta;
>    dy++;
> } while (dy < Bottom);
>
> }
>
> VOID
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev