[PATCH] File readahead buffering

[PATCH] File readahead buffering

[Colin D Bennett]

[-- Attachment #1.1: Type: text/plain, Size: 1555 bytes --]

This patch speeds up loading a TGA image on my test system from 29
seconds to approximately 1 second.

I noticed that on my 1 GHz test system running from an IDE CompactFlash
drive, loading a certain TGA image in GRUB takes about 29 seconds.
This seems pretty outrageous, so after looking at the code I hacked the
TGA reader code to read the entire TGA image file into memory with a
single grub_file_read() call.  This made it much faster.  Therefore, I
concluded that the problem was that each of the one- to four-byte
reads done by the TGA reader was causing an actual device read.  It
seems that it is the file subsystem's responsibility to do file
buffering, so I implemented it in `kern/file.c'.

It turns out that when booting GRUB from IDE, if file buffering is
used, GRUB hangs right after the "GRUB" message, early in the boot
process.  So I added a flag that allows grub_main() to enable buffering
when it is safe to do so.  It always worked file from an ISO image
(generated with grub-mkrescue) in VMware and QEMU, but when I actually
installed GRUB to my CompactFlash drive and booted it, it hung after
"GRUB" if file buffering was enabled at the start.

It may be poor style to expose `grub_file_buffering_enabled' as a
global variable in file.h, but after I refactored it as a function, I
wondered whether the overhead of creating a whole new function as a
'setter' was wasteful since it would be called only once.  Please
advise if you prefer it as a function, or if there is a better solution.

Regards,
Colin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: file-buffering.patch --]
[-- Type: text/x-patch; name=file-buffering.patch, Size: 6316 bytes --]

=== modified file 'ChangeLog'
--- ChangeLog	2008-07-21 09:40:01 +0000
+++ ChangeLog	2008-07-22 15:42:00 +0000
@@ -1,3 +1,17 @@
+2008-07-22  Colin D Bennett <colin@gibibit.com>
+
+	* include/grub/file.h (grub_file_buffering_enabled): New global
+	variable.
+
+	* kern/file.c (BUFFER_SIZE): New constant.
+	(grub_file_buffering_enabled): New global variable.
+	(grub_file_open): Allocate file readahead buffer.
+	(grub_file_close): Free file readahead buffer.
+	(grub_file_read): Use file readahead buffer.
+
+	* kern/main.c (grub_main): Enable file readahead buffering when it is
+	safe to do so.
+
 2008-07-21  Bean  <bean123ch@gmail.com>
 
 	* kern/i386/pc/startup.S (gate_a20_try_bios): Change test order for

=== modified file 'include/grub/file.h'
--- include/grub/file.h	2007-08-02 17:40:37 +0000
+++ include/grub/file.h	2008-07-22 15:23:31 +0000
@@ -1,6 +1,6 @@
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2007,2008  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -39,6 +39,19 @@
   /* The file size.  */
   grub_off_t size;
 
+  /* The file offset when `buffer' starts, or -1 to indicate that
+     buffer has no data.  */
+  grub_off_t buffer_offset;
+
+  /* The number of bytes in `buffer'.  */
+  grub_size_t buffer_length;
+
+  /* The number of bytes allocated for `buffer'.  */
+  grub_size_t buffer_capacity;
+
+  /* Readahead buffer.  */
+  char *buffer;
+
   /* Filesystem-specific data.  */
   void *data;
 
@@ -48,6 +61,11 @@
 };
 typedef struct grub_file *grub_file_t;
 
+/* Enable file readahead buffering if nonzero.  Buffering is disabled by
+   default, since it seems to cause problems during the early GRUB boot
+   process.  The grub_main function enables it.  */
+extern int grub_file_buffering_enabled;
+
 /* Get a device name from NAME.  */
 char *EXPORT_FUNC(grub_file_get_device_name) (const char *name);
 

=== modified file 'kern/file.c'
--- kern/file.c	2008-01-25 20:57:40 +0000
+++ kern/file.c	2008-07-22 15:23:31 +0000
@@ -1,7 +1,7 @@
 /* file.c - file I/O functions */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2002,2006,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2002,2006,2007,2008  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -24,6 +24,12 @@
 #include <grub/fs.h>
 #include <grub/device.h>
 
+/* File readahead buffer size in bytes.  */
+#define BUFFER_SIZE 1024
+
+/* Use readahead buffering for file reads?  */
+int grub_file_buffering_enabled = 0;
+
 /* Get the device part of the filename NAME. It is enclosed by parentheses.  */
 char *
 grub_file_get_device_name (const char *name)
@@ -81,6 +87,10 @@
   
   file->device = device;
   file->offset = 0;
+  file->buffer_offset = -1;
+  file->buffer_length = 0;
+  file->buffer_capacity = 0;
+  file->buffer = 0;
   file->data = 0;
   file->read_hook = 0;
     
@@ -97,6 +107,13 @@
   if ((file->fs->open) (file, file_name) != GRUB_ERR_NONE)
     goto fail;
 
+  if (grub_file_buffering_enabled)
+    {
+      file->buffer = grub_malloc (BUFFER_SIZE);
+      if (file->buffer)
+        file->buffer_capacity = BUFFER_SIZE;
+    }
+
   return file;
 
  fail:
@@ -110,6 +127,15 @@
   return 0;
 }
 
+static inline grub_size_t
+grub_min (grub_size_t a, grub_size_t b)
+{
+  if (a < b)
+    return a;
+  else
+    return b;
+}
+
 grub_ssize_t
 grub_file_read (grub_file_t file, char *buf, grub_size_t len)
 {
@@ -124,10 +150,52 @@
   
   if (len == 0)
     return 0;
-  
-  res = (file->fs->read) (file, buf, len);
-  if (res > 0)
-    file->offset += res;
+
+  if (grub_file_buffering_enabled && file->buffer)
+    {
+      res = 0;   /* The while loop uses res to accumulate the # bytes read. */
+      while (len)
+        {
+          /* If the start of the chunk requested is not in the buffer,
+             then fill the buffer starting at the requested offset.  */
+          if (file->buffer_offset == -1
+              || file->offset < file->buffer_offset
+              || file->offset >= file->buffer_offset + file->buffer_length)
+            {
+              grub_ssize_t res2;
+
+              /* Read into the buffer, and then use that data.  */
+              file->buffer_offset = -1;
+              file->buffer_length = grub_min (file->buffer_capacity,
+                                              file->size - file->offset);
+              res2 = (file->fs->read) (file, file->buffer, file->buffer_length);
+              if (res2 > 0)
+                {
+                  file->buffer_length = res2;
+                  file->buffer_offset = file->offset;
+                }
+              else
+                return res;
+            }
+
+          /* Use as much buffered data as possible.  */
+          grub_size_t bufstart = file->offset - file->buffer_offset;
+          grub_size_t buflen = len;
+          if (bufstart + len > file->buffer_length)
+            buflen = file->buffer_length - bufstart;
+          grub_memcpy (buf, file->buffer + bufstart, buflen);
+          buf += buflen;
+          res += buflen;
+          len -= buflen;
+          file->offset += buflen;
+        }
+    }
+  else
+    {
+      res = (file->fs->read) (file, buf, len);
+      if (res > 0)
+        file->offset += res;
+    }
 
   return res;
 }
@@ -135,6 +203,9 @@
 grub_err_t
 grub_file_close (grub_file_t file)
 {
+  grub_free (file->buffer);
+  file->buffer = 0;
+
   if (file->fs->close)
     (file->fs->close) (file);
 

=== modified file 'kern/main.c'
--- kern/main.c	2008-06-19 19:08:57 +0000
+++ kern/main.c	2008-07-22 15:23:31 +0000
@@ -128,6 +128,9 @@
   grub_env_export ("prefix");
   grub_set_root_dev ();
 
+  /* Enable file readahead buffering, now that it is safe to do so.  */
+  grub_file_buffering_enabled = 1;
+
   /* Load the normal mode module.  */
   grub_load_normal_mode ();
   


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] File readahead buffering

[Pavel Roskin]

On Tue, 2008-07-22 at 08:43 -0700, Colin D Bennett wrote:
> This patch speeds up loading a TGA image on my test system from 29
> seconds to approximately 1 second.
> 
> I noticed that on my 1 GHz test system running from an IDE CompactFlash
> drive, loading a certain TGA image in GRUB takes about 29 seconds.

I'm sorry for straying from your point, but maybe we should drop TGA
support.  It was the first image format for GRUB to support, but now PNG
is supported, and it should be better in all aspects.

> It turns out that when booting GRUB from IDE, if file buffering is
> used, GRUB hangs right after the "GRUB" message, early in the boot
> process.  So I added a flag that allows grub_main() to enable buffering
> when it is safe to do so.  It always worked file from an ISO image
> (generated with grub-mkrescue) in VMware and QEMU, but when I actually
> installed GRUB to my CompactFlash drive and booted it, it hung after
> "GRUB" if file buffering was enabled at the start.

I think we should be prefer simple and reliable code, rather than add
complexity and risks of failure to achieve higher speeds.

-- 
Regards,
Pavel Roskin

Re: [PATCH] File readahead buffering

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

On Tue, 22 Jul 2008 14:48:31 -0400
Pavel Roskin <proski@gnu.org> wrote:

> On Tue, 2008-07-22 at 08:43 -0700, Colin D Bennett wrote:
> > This patch speeds up loading a TGA image on my test system from 29
> > seconds to approximately 1 second.
> > 
> > I noticed that on my 1 GHz test system running from an IDE
> > CompactFlash drive, loading a certain TGA image in GRUB takes about
> > 29 seconds.
> 
> I'm sorry for straying from your point, but maybe we should drop TGA
> support.  It was the first image format for GRUB to support, but now
> PNG is supported, and it should be better in all aspects.

I agree that TGA is not, in general, a great choice for an image format
(unless it is faster to load a large background image -- a 1024x768 RGB
PNG file may take more time to decompress than a TGA image would take
to load -- although perhaps an uncompressed PNG file would be
comparable in speed to load).  However, I have not been able to load any
PNG images that I have tried to use. Something about the chunk type not
being supported.

> > It turns out that when booting GRUB from IDE, if file buffering is
> > used, GRUB hangs right after the "GRUB" message, early in the boot
> > process.  So I added a flag that allows grub_main() to enable
> > buffering when it is safe to do so.  It always worked file from an
> > ISO image (generated with grub-mkrescue) in VMware and QEMU, but
> > when I actually installed GRUB to my CompactFlash drive and booted
> > it, it hung after "GRUB" if file buffering was enabled at the start.
> 
> I think we should be prefer simple and reliable code, rather than add
> complexity and risks of failure to achieve higher speeds.

If the buffering is not done in the file I/O layer, then the font
loading, theme file loading, image file loading, etc. will all need to
do their own buffering, which IMHO is more error prone and makes those
modules use more code to handle low level I/O stuff, which detracts
from their specific purpose.

Also, this is no small increase in speed, but from 10x to 100x increase
in performance for some cases where small sequential reads are
performed.

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] File readahead buffering

[Pavel Roskin]

On Tue, 2008-07-22 at 12:06 -0700, Colin D Bennett wrote:

> I agree that TGA is not, in general, a great choice for an image format
> (unless it is faster to load a large background image -- a 1024x768 RGB
> PNG file may take more time to decompress than a TGA image would take
> to load -- although perhaps an uncompressed PNG file would be
> comparable in speed to load).  However, I have not been able to load any
> PNG images that I have tried to use. Something about the chunk type not
> being supported.

Strange.  It's working for me.  You may want to post that file to the
list.

> If the buffering is not done in the file I/O layer, then the font
> loading, theme file loading, image file loading, etc. will all need to
> do their own buffering, which IMHO is more error prone and makes those
> modules use more code to handle low level I/O stuff, which detracts
> from their specific purpose.
> 
> Also, this is no small increase in speed, but from 10x to 100x increase
> in performance for some cases where small sequential reads are
> performed.

OK, then you may have a case.

-- 
Regards,
Pavel Roskin

Re: [PATCH] File readahead buffering

[Bean]

On Wed, Jul 23, 2008 at 3:06 AM, Colin D Bennett <colin@gibibit.com> wrote:
> On Tue, 22 Jul 2008 14:48:31 -0400
> Pavel Roskin <proski@gnu.org> wrote:
>
>> On Tue, 2008-07-22 at 08:43 -0700, Colin D Bennett wrote:
>> > This patch speeds up loading a TGA image on my test system from 29
>> > seconds to approximately 1 second.
>> >
>> > I noticed that on my 1 GHz test system running from an IDE
>> > CompactFlash drive, loading a certain TGA image in GRUB takes about
>> > 29 seconds.
>>
>> I'm sorry for straying from your point, but maybe we should drop TGA
>> support.  It was the first image format for GRUB to support, but now
>> PNG is supported, and it should be better in all aspects.
>
> I agree that TGA is not, in general, a great choice for an image format
> (unless it is faster to load a large background image -- a 1024x768 RGB
> PNG file may take more time to decompress than a TGA image would take
> to load -- although perhaps an uncompressed PNG file would be
> comparable in speed to load).  However, I have not been able to load any
> PNG images that I have tried to use. Something about the chunk type not
> being supported.
>

Hi,

Please upload the png file that cause problem.

Also note that png use DEFLATE compression. I write a decoder in png,
which may be a little slow. Perhaps I can import the decoder from
zlib, if copyright allows.

-- 
Bean

Re: [PATCH] File readahead buffering

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

El mié, 23-07-2008 a las 10:05 +0800, Bean escribió:
> On Wed, Jul 23, 2008 at 3:06 AM, Colin D Bennett <colin@gibibit.com> wrote:
> > On Tue, 22 Jul 2008 14:48:31 -0400
> > Pavel Roskin <proski@gnu.org> wrote:
> > However, I have not been able to load any
> > PNG images that I have tried to use. Something about the chunk type not
> > being supported.
> >
> 
> Hi,
> 
> Please upload the png file that cause problem.
> 
> Also note that png use DEFLATE compression. I write a decoder in png,
> which may be a little slow. Perhaps I can import the decoder from
> zlib, if copyright allows.
Does the GRUB decoder distinguish between critical and ancillary chunks?
A decoder finding a critical chunk that it does not understand should
abort, but finding an unknown ancillary chunk is not an error - maybe
his file has an ancillary chunk like zTXt and the GRUB png reader is
rejecting it as having unknown chunks. I didn't even take a look at the
decoder code so I could be dead wrong, but I think it's a possibility
worth investigating...


[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] File readahead buffering

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

On Tue, 22 Jul 2008 17:44:53 -0400
Pavel Roskin <proski@gnu.org> wrote:

> On Tue, 2008-07-22 at 12:06 -0700, Colin D Bennett wrote:
> 
> > I agree that TGA is not, in general, a great choice for an image
> > format (unless it is faster to load a large background image -- a
> > 1024x768 RGB PNG file may take more time to decompress than a TGA
> > image would take to load -- although perhaps an uncompressed PNG
> > file would be comparable in speed to load).  However, I have not
> > been able to load any PNG images that I have tried to use.
> > Something about the chunk type not being supported.
> 
> Strange.  It's working for me.  You may want to post that file to the
> list.

Actually I'm testing it now, and PNG images are working... I'll post an
update when I have finished testing.  Maybe the failures I experienced
in the past were due to an old version of GRUB.
 
> > If the buffering is not done in the file I/O layer, then the font
> > loading, theme file loading, image file loading, etc. will all need
> > to do their own buffering, which IMHO is more error prone and makes
> > those modules use more code to handle low level I/O stuff, which
> > detracts from their specific purpose.
> > 
> > Also, this is no small increase in speed, but from 10x to 100x
> > increase in performance for some cases where small sequential reads
> > are performed.
> 
> OK, then you may have a case.

FWIW, here are some results of timing the loading of the same 1024x768
image in TGA, PNG, and JPEG formats -- first without the file buffering
patch, and then with the file buffering patch:

(Loading times in seconds.)

                   TGA  PNG 0  PNG 100  JPEG q80  JPEG q100
                 -----  -----  -------  --------  ---------
   No buffering     17     9        12         1          7 
      Buffering      1     1         1         1          1

I see that PNG files load at least as fast as TGA files, so that is
great news.  It also seems to show the degree of benefit provided by
using buffered file reads.  These tests were done by using the
'tgatest', 'pngtest', and 'jpegtest' commands, and manually timing the
results (accuracy +/- one second or so).

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] File readahead buffering

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

On Wed, 23 Jul 2008 10:05:36 +0800
Bean <bean123ch@gmail.com> wrote:

> On Wed, Jul 23, 2008 at 3:06 AM, Colin D Bennett <colin@gibibit.com>
> wrote:
> > On Tue, 22 Jul 2008 14:48:31 -0400
> > Pavel Roskin <proski@gnu.org> wrote:
> >
> >> On Tue, 2008-07-22 at 08:43 -0700, Colin D Bennett wrote:
> >> > This patch speeds up loading a TGA image on my test system from
> >> > 29 seconds to approximately 1 second.
> >> >
> >> > I noticed that on my 1 GHz test system running from an IDE
> >> > CompactFlash drive, loading a certain TGA image in GRUB takes
> >> > about 29 seconds.
> >>
> >> I'm sorry for straying from your point, but maybe we should drop
> >> TGA support.  It was the first image format for GRUB to support,
> >> but now PNG is supported, and it should be better in all aspects.
> >
> > I agree that TGA is not, in general, a great choice for an image
> > format (unless it is faster to load a large background image -- a
> > 1024x768 RGB PNG file may take more time to decompress than a TGA
> > image would take to load -- although perhaps an uncompressed PNG
> > file would be comparable in speed to load).  However, I have not
> > been able to load any PNG images that I have tried to use.
> > Something about the chunk type not being supported.
> >
> 
> Hi,
> 
> Please upload the png file that cause problem.
> 
> Also note that png use DEFLATE compression. I write a decoder in png,
> which may be a little slow. Perhaps I can import the decoder from
> zlib, if copyright allows.

Ok, after some more testing, I have found that if I create PNG images
with ImageMagick or The GIMP, they work.  However, I have an image that
I was trying to use as a GRUB background which is from a MythTV theme
called Titivillus.  The PNG file from the MythTV theme doesn't load in
GRUB.  I get the message "error: png: block type fixed not supported".

The file is at 
http://gibibit.com/upload/bg-myth.png
and ImageMagick's "identify -verbose" tells me it was created with
Adobe ImageReady.

Since the free tools that I've tried seem to work fine, this is not a
big deal to me, but perhaps before a widespread GRUB 2 release we might
want to improve the PNG reader compatibility.

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] File readahead buffering

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

On Wed, 23 Jul 2008 07:33:32 -0700
Colin D Bennett <colin@gibibit.com> wrote:

> On Wed, 23 Jul 2008 10:05:36 +0800
> Bean <bean123ch@gmail.com> wrote:
> 
> > On Wed, Jul 23, 2008 at 3:06 AM, Colin D Bennett <colin@gibibit.com>
> > wrote:
> > > On Tue, 22 Jul 2008 14:48:31 -0400
> > > Pavel Roskin <proski@gnu.org> wrote:
> > >
> > >> On Tue, 2008-07-22 at 08:43 -0700, Colin D Bennett wrote:
> > >> > This patch speeds up loading a TGA image on my test system from
> > >> > 29 seconds to approximately 1 second.
> > >> >
> > >> > I noticed that on my 1 GHz test system running from an IDE
> > >> > CompactFlash drive, loading a certain TGA image in GRUB takes
> > >> > about 29 seconds.
> > >>
> > >> I'm sorry for straying from your point, but maybe we should drop
> > >> TGA support.  It was the first image format for GRUB to support,
> > >> but now PNG is supported, and it should be better in all aspects.
> > >
> > > I agree that TGA is not, in general, a great choice for an image
> > > format (unless it is faster to load a large background image -- a
> > > 1024x768 RGB PNG file may take more time to decompress than a TGA
> > > image would take to load -- although perhaps an uncompressed PNG
> > > file would be comparable in speed to load).  However, I have not
> > > been able to load any PNG images that I have tried to use.
> > > Something about the chunk type not being supported.
> > >
> > 
> > Hi,
> > 
> > Please upload the png file that cause problem.
> > 
> > Also note that png use DEFLATE compression. I write a decoder in
> > png, which may be a little slow. Perhaps I can import the decoder
> > from zlib, if copyright allows.
> 
> Ok, after some more testing, I have found that if I create PNG images
> with ImageMagick or The GIMP, they work.  However, I have an image
> that I was trying to use as a GRUB background which is from a MythTV
> theme called Titivillus.  The PNG file from the MythTV theme doesn't
> load in GRUB.  I get the message "error: png: block type fixed not
> supported".

Oops!  After more testing, I have found PNG images that I saved in The
GIMP which also refuse to load due to the "block type fixed not
supported" error.  One ZIP file [1] contains images that all fail to
load in GRUB with that error, and the other [2] contains similar images
that load just fine.

[1] gibibit.com/upload/grub-png-bad.zip
[2] gibibit.com/upload/grub-png-good.zip

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] File readahead buffering

[Bean]

[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]

On Wed, Jul 23, 2008 at 10:56 PM, Colin D Bennett <colin@gibibit.com> wrote:
> On Wed, 23 Jul 2008 07:33:32 -0700
> Colin D Bennett <colin@gibibit.com> wrote:
>
>> On Wed, 23 Jul 2008 10:05:36 +0800
>> Bean <bean123ch@gmail.com> wrote:
>>
>> > On Wed, Jul 23, 2008 at 3:06 AM, Colin D Bennett <colin@gibibit.com>
>> > wrote:
>> > > On Tue, 22 Jul 2008 14:48:31 -0400
>> > > Pavel Roskin <proski@gnu.org> wrote:
>> > >
>> > >> On Tue, 2008-07-22 at 08:43 -0700, Colin D Bennett wrote:
>> > >> > This patch speeds up loading a TGA image on my test system from
>> > >> > 29 seconds to approximately 1 second.
>> > >> >
>> > >> > I noticed that on my 1 GHz test system running from an IDE
>> > >> > CompactFlash drive, loading a certain TGA image in GRUB takes
>> > >> > about 29 seconds.
>> > >>
>> > >> I'm sorry for straying from your point, but maybe we should drop
>> > >> TGA support.  It was the first image format for GRUB to support,
>> > >> but now PNG is supported, and it should be better in all aspects.
>> > >
>> > > I agree that TGA is not, in general, a great choice for an image
>> > > format (unless it is faster to load a large background image -- a
>> > > 1024x768 RGB PNG file may take more time to decompress than a TGA
>> > > image would take to load -- although perhaps an uncompressed PNG
>> > > file would be comparable in speed to load).  However, I have not
>> > > been able to load any PNG images that I have tried to use.
>> > > Something about the chunk type not being supported.
>> > >
>> >
>> > Hi,
>> >
>> > Please upload the png file that cause problem.
>> >
>> > Also note that png use DEFLATE compression. I write a decoder in
>> > png, which may be a little slow. Perhaps I can import the decoder
>> > from zlib, if copyright allows.
>>
>> Ok, after some more testing, I have found that if I create PNG images
>> with ImageMagick or The GIMP, they work.  However, I have an image
>> that I was trying to use as a GRUB background which is from a MythTV
>> theme called Titivillus.  The PNG file from the MythTV theme doesn't
>> load in GRUB.  I get the message "error: png: block type fixed not
>> supported".
>
> Oops!  After more testing, I have found PNG images that I saved in The
> GIMP which also refuse to load due to the "block type fixed not
> supported" error.  One ZIP file [1] contains images that all fail to
> load in GRUB with that error, and the other [2] contains similar images
> that load just fine.
>
> [1] gibibit.com/upload/grub-png-bad.zip
> [2] gibibit.com/upload/grub-png-good.zip

Hi,

This patch should fix the problem.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: png.diff --]
[-- Type: text/x-diff; name=png.diff, Size: 1903 bytes --]

diff --git a/video/readers/png.c b/video/readers/png.c
index 608fa5e..9dac4b6 100644
--- a/video/readers/png.c
+++ b/video/readers/png.c
@@ -68,7 +68,7 @@
 #define DEFLATE_HCLEN_BASE	4
 #define DEFLATE_HCLEN_MAX	19
 #define DEFLATE_HLIT_BASE	257
-#define DEFLATE_HLIT_MAX	286
+#define DEFLATE_HLIT_MAX	288
 #define DEFLATE_HDIST_BASE	1
 #define DEFLATE_HDIST_MAX	30
 
@@ -391,6 +391,41 @@ grub_png_get_huff_code (struct grub_png_data *data, struct huff_table *ht)
 }
 
 static grub_err_t
+grub_png_init_fixed_block (struct grub_png_data *data)
+{
+  int i;
+
+  grub_png_init_huff_table (&data->code_table, DEFLATE_HUFF_LEN,
+			    data->code_values, data->code_maxval,
+			    data->code_offset);
+
+  for (i = 0; i < 144; i++)
+    grub_png_insert_huff_item (&data->code_table, i, 8);
+
+  for (; i < 256; i++)
+    grub_png_insert_huff_item (&data->code_table, i, 9);
+
+  for (; i < 280; i++)
+    grub_png_insert_huff_item (&data->code_table, i, 7);
+
+  for (; i < DEFLATE_HLIT_MAX; i++)
+    grub_png_insert_huff_item (&data->code_table, i, 8);
+
+  grub_png_build_huff_table (&data->code_table);
+
+  grub_png_init_huff_table (&data->dist_table, DEFLATE_HUFF_LEN,
+			    data->dist_values, data->dist_maxval,
+			    data->dist_offset);
+
+  for (i = 0; i < DEFLATE_HDIST_MAX; i++)
+    grub_png_insert_huff_item (&data->dist_table, i, 5);
+
+  grub_png_build_huff_table (&data->dist_table);
+
+  return grub_errno;
+}
+
+static grub_err_t
 grub_png_init_dynamic_block (struct grub_png_data *data)
 {
   int nl, nd, nb, i, prev;
@@ -699,8 +734,9 @@ grub_png_decode_image_data (struct grub_png_data *data)
 	  }
 
 	case INFLATE_FIXED:
-	  return grub_error (GRUB_ERR_BAD_FILE_TYPE,
-			     "png: block type fixed not supported");
+          grub_png_init_fixed_block (data);
+	  grub_png_read_dynamic_block (data);
+	  break;
 
 	case INFLATE_DYNAMIC:
 	  grub_png_init_dynamic_block (data);

Re: [PATCH] File readahead buffering

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

On Thu, 24 Jul 2008 17:51:23 +0800
Bean <bean123ch@gmail.com> wrote:

> On Wed, Jul 23, 2008 at 10:56 PM, Colin D Bennett <colin@gibibit.com>
> wrote:
> > On Wed, 23 Jul 2008 07:33:32 -0700
> > Colin D Bennett <colin@gibibit.com> wrote:
> >
> >> Ok, after some more testing, I have found that if I create PNG
> >> images with ImageMagick or The GIMP, they work.  However, I have
> >> an image that I was trying to use as a GRUB background which is
> >> from a MythTV theme called Titivillus.  The PNG file from the
> >> MythTV theme doesn't load in GRUB.  I get the message "error: png:
> >> block type fixed not supported".
> >
> > Oops!  After more testing, I have found PNG images that I saved in
> > The GIMP which also refuse to load due to the "block type fixed not
> > supported" error.  One ZIP file [1] contains images that all fail to
> > load in GRUB with that error, and the other [2] contains similar
> > images that load just fine.
> >
> > [1] gibibit.com/upload/grub-png-bad.zip
> > [2] gibibit.com/upload/grub-png-good.zip
> 
> Hi,
> 
> This patch should fix the problem.

Hi Bean,

Thanks for the PNG fix!  I tested it and it fixes the problem.  8-)
I am now using PNG files in my testing and they are working great.

I'm also using my file I/O buffering patch and it helps performance
massively when loading images.  I hope that in using the file
buffering patch myself I can find any potential problems with it.

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] File readahead buffering

[Bean]

On Sun, Jul 27, 2008 at 1:32 AM, Colin D Bennett <colin@gibibit.com> wrote:
> Hi Bean,
>
> Thanks for the PNG fix!  I tested it and it fixes the problem.  8-)
> I am now using PNG files in my testing and they are working great.
>
> I'm also using my file I/O buffering patch and it helps performance
> massively when loading images.  I hope that in using the file
> buffering patch myself I can find any potential problems with it.
>
> Regards,
> Colin
>

Hi,

Actually, you can solve this by reading trunks at at time. For
example, you can write a wrapper for grub_file_read that prefetch 8192
bytes.

-- 
Bean