[syslinux] Display graphic from filename broken? - work in progress

[Adrian Weiler]

On 30.10.2014 15:25 Ady wrote:
> Yes, the lss16 support for DISPLAY files is broken since Syslinux 5+.
> [...]
> I hope your initial scrutiny of the source code can bring up an actual
> solution / patches for both, Syslinux CLI and menu parsers (and if
> possible for both, BIOS and UEFI). 
I made some further investigations and fixed a number of bugs in
graphics.c. This includes
- file name properly extracted from DISPLAY file [pointer type problem]
- fixed reading of header in vgadisplayfile() [pointer was not
incremented and EOF was compared against a byte]
- fixed getnybble() [completely broken]
- fixed rledecode() [when adding 16 to a byte, a bigger data type must
be used]
- fixed packedpixel2vga() [input pointer was incremented in wrong loop]

Unfortunately, there is still no graphic. As far as I can tell, this
portion of the code is now faithfully translated from assembler to C.
Diagnosis: outputvga() gets called, but the memcpy() to the VGA memory
at 0xA0000 does not seem to have any effect. I even tried to replace the
memcpy() with a memset() and set the VGA memory to various (constant and
repeated) values - with no visible result.

Here I am out of ideas. Also, I don't have any experience with VGA
hardware at that level.  May be someone with VGA knowledge (HPA?) could
find out what is probably the last problem of lss16 in DISPLAY files.

And yes, I understand that is BIOS only and thus not the preferred
solution. But it might be easier to make the code work first and
refactor later.

These are my patches so far (based on syslinux-6.03).
-----------------8<---------------------8<----------------
--- a/core/include/graphics.h    2014-10-30 14:02:34.904029005 +0100
+++ b/core/include/graphics.h    2014-11-13 18:44:04.101257141 +0100
@@ -41,7 +41,7 @@
 
 extern uint8_t UsingVGA;
 extern uint16_t VGAPos;
-extern uint16_t *VGAFilePtr;
+extern char *VGAFilePtr;
 extern char VGAFileBuf[VGA_FILE_BUF_SIZE];
 extern char VGAFileMBuf[];
 extern uint16_t VGAFontSize;
--- a/core/graphics.c    2014-10-30 14:02:34.904029005 +0100
+++ b/core/graphics.c    2014-11-13 18:54:17.929261242 +0100
@@ -28,7 +28,7 @@
 
 __export uint8_t UsingVGA = 0;
 uint16_t VGAPos;        /* Pointer into VGA memory */
-__export uint16_t *VGAFilePtr;    /* Pointer into VGAFileBuf */
+__export char *VGAFilePtr;    /* Pointer into VGAFileBuf */
 __export uint16_t VGAFontSize = 16;    /* Defaults to 16 byte font */
 
 __export char VGAFileBuf[VGA_FILE_BUF_SIZE];    /* Unmangled VGA image
name */
@@ -44,6 +44,8 @@ extern uint16_t GXPixRows;
 static uint8_t linear_color[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8,
                   9, 10, 11, 12, 13, 14, 15, 0 };
 
+static char last_nibble;
+
 static FILE *fd;
 
 typedef struct {
@@ -66,7 +68,7 @@ static int vgasetmode(void)
 {
     com32sys_t ireg, oreg;
 
-    if (UsingVGA)
+    if (UsingVGA == 1)
         return 0;        /* Nothing to do... */
 
     memset(&ireg, 0, sizeof(ireg));
@@ -99,7 +101,8 @@ static int vgasetmode(void)
     __intcall(0x10, &ireg, &oreg);
 
     memset(&ireg, 0, sizeof(ireg));
-    ireg.edx.w[0] = (uint32_t)linear_color;
+    ireg.edx.w[0] = OFFS(linear_color);
+    ireg.es       = SEG(linear_color);
     ireg.eax.w[0] = 0x1002;    /* Write color registers */
     __intcall(0x10, &ireg, &oreg);
 
@@ -117,14 +120,15 @@ static int vgasetmode(void)
 
 static inline char getnybble(void)
 {
-    char data = getc(fd);
+    char data;
 
-    if (data & 0x10) {
-        data &= 0x0F;
-        return data;
+    if (last_nibble & 0x10) {
+        last_nibble &= 0x0F;
+        return last_nibble;
     }
 
     data = getc(fd);
+    last_nibble = (data >> 4) | 0x10;  /* Flag nibble already read */
     return (data & 0x0F);
 }
 
@@ -139,12 +143,12 @@ static inline char getnybble(void)
 static void rledecode(uint8_t *out, size_t count)
 {
     uint8_t prev_pixel = 0;
-    size_t size = count;
-    uint8_t data;
-    int i;
+    uint16_t data;
+    uint16_t i;
 
+    last_nibble = 0;
 again:
-    for (i = 0; i < size; i++) {
+    for (; count; --count) {
 
         data = getnybble();
         if (data == prev_pixel)
@@ -154,8 +158,7 @@ again:
         prev_pixel = data;
     }
 
-    size -= i;
-    if (!size)
+    if (!count)
         return;
 
     /* Start of run sequence */
@@ -171,12 +174,17 @@ again:
         data += 16;
     }
 
+    /* sanity check */
+    if (data > count)
+        data = count;
+
     /* dorun */
     for (i = 0; i < data; i++)
         *out++ = prev_pixel;
 
-    size -= i;
-    if (size)
+    count -= data;
+
+    if (count)
         goto again;
 }
 
@@ -197,9 +205,9 @@ static void packedpixel2vga(const uint8_
 
         for (j = 0; j < 640/8; j++) {
             uint8_t ob = 0;
+            uint8_t px = *ip++;
 
             for (k = 0; k < 8; k++) {
-                uint8_t px = *ip++;
                 ob = (ob << 1) | ((px >> i) & 1);
             }
 
@@ -236,6 +244,7 @@ static void outputvga(const void *in, vo
 __export void vgadisplayfile(FILE *_fd)
 {
     char *p;
+    int ch;
     int size;
 
     fd = _fd;
@@ -252,9 +261,12 @@ __export void vgadisplayfile(FILE *_fd)
 
     /* Load the header */
     while (size--)
-        *p = getc(fd);
+    {   
+        ch = getc(fd);
+        *p++ = (char) ch;
+    }
 
-    if (*p != EOF) {
+    if (ch != EOF) {
         com32sys_t ireg, oreg;
         uint16_t rows;
         int i;
@@ -266,7 +278,8 @@ __export void vgadisplayfile(FILE *_fd)
         memset(&ireg, 0, sizeof(ireg));
 
         /* Color map offset */
-        ireg.edx.w[0] = offsetof(lssheader_t, GraphColorMap);
+        ireg.edx.w[0] = OFFS(LSSHeader.GraphColorMap);
+        ireg.es       = SEG(LSSHeader.GraphColorMap);
 
         ireg.eax.w[0] = 0x1012;           /* Set RGB registers */
         ireg.ebx.w[0] = 0;           /* First register number */
-----------------8<---------------------8<----------------

Adrian