Hi, the display of LSS16 files from a DISPLAY file (as documented in http://www.syslinux.org/wiki/index.php/SYSLINUX#Display_graphic_from_filename:) seems heavily broken since syslinux has been converted from assembler to C. I already discovered one bug in core/include/graphics.h (and core/graphics.c): the pointer VGAFilePtr is of type uint16_t*, but should be plain char*. This bug causes the file name to be assembled incorrectly in com32/elflink/ldlinux/msg.c (msg_filename(), line 160): there the pointer is autoincremented, and if it is a uint16_t*, then it will autoincrement by two bytes. Therefore there will be a null char every second byte, which makes everybody think the file name has only one single character. After fixing this, the file name is assembled correctly, and msg_viewimage() seems to open the file successfully. The display now switches to graphic mode, but still the graphic file is not displayed. I am stuck here. The very same configuration works fine with syslinux 4.05. Adrian
> Hi, > > the display of LSS16 files from a DISPLAY file (as documented in > http://www.syslinux.org/wiki/index.php/SYSLINUX#Display_graphic_from_filename:) > seems heavily broken since syslinux has been converted from assembler to > C. I already discovered one bug in core/include/graphics.h (and > core/graphics.c): the pointer VGAFilePtr is of type uint16_t*, but > should be plain char*. This bug causes the file name to be assembled > incorrectly in com32/elflink/ldlinux/msg.c (msg_filename(), line 160): > there the pointer is autoincremented, and if it is a uint16_t*, then it > will autoincrement by two bytes. Therefore there will be a null char > every second byte, which makes everybody think the file name has only > one single character. > > After fixing this, the file name is assembled correctly, and > msg_viewimage() seems to open the file successfully. The display now > switches to graphic mode, but still the graphic file is not displayed. I > am stuck here. The very same configuration works fine with syslinux 4.05. > > Adrian > _______________________________________________ > Syslinux mailing list > Submissions to Syslinux at zytor.com > Unsubscribe or set options at: > http://www.zytor.com/mailman/listinfo/syslinux >Yes, the lss16 support for DISPLAY files is broken since Syslinux 5+. This has been reported in this Syslinux Mailing List several times already, and there is also a bug report for it http://bugzilla.syslinux.org/show_bug.cgi?id=10 . FWIW, version 5.xx has many other problems, part of them resolved in 6.03 (but the lss16 issue is not yet resolved). 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). Any developers willing to look at this issue and actually solve it? TIA, Ady.
On 10/30/2014 07:25 AM, Ady wrote:> > Yes, the lss16 support for DISPLAY files is broken since Syslinux 5+. > This has been reported in this Syslinux Mailing List several times > already, and there is also a bug report for it > http://bugzilla.syslinux.org/show_bug.cgi?id=10 . > > FWIW, version 5.xx has many other problems, part of them resolved in > 6.03 (but the lss16 issue is not yet resolved). > > 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). > > Any developers willing to look at this issue and actually solve it? >Just a huge lack of time. We should support all image formats for this. -hpa
Adrian Weiler
2014-Nov-13 18:34 UTC
[syslinux] Display graphic from filename broken? - work in progress
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