Re: [patch] batch scanlines when loading JPEG...

[Daniel J Blueman <daniel blueman gmail com>]

Hi Federico,

On Sat, Aug 29, 2009 at 12:27 AM, Federico Mena
Quintero<federico ximian com> wrote:
> On Fri, 2009-08-28 at 16:12 +0100, Daniel J Blueman wrote:
>
>> I've written and tested a small patch that increases efficiency
>> (particularly in small-cache systems) of loading JPEG images: override
>> the recommended lines with what we've already allocated; increase
>> batch size from 4 to 8 lines.
>
> Thanks for trying to make JPEG loading faster; we could certainly use
> that :)  A few comments:

> First, did you actually test that patch (did you send the wrong one)?
> The last hunk doesn't compile:
> +                       /* batch line updates f or efficiency */
> +                       cinfo->rec_outbuf_height = cinfo.rec_outbuf_height > LINEBUF_SIZE ? cinfo.rec_outbuf_height : LINEBUF_SIZE;
> +
>
> That should be "cinfo->blah" instead of "cinfo.blah".

Oops. Looks like I didn't rediff after fixing that.

> Second, that test looks wrong:
>
> cinfo.rec_outbuf_height = cinfo.rec_outbuf_height > LINEBUF_SIZE ? cinfo.rec_outbuf_height : LINEBUF_SIZE;
>
> I just checked the libjpeg sources and it will never return
> cinfo.rec_outbuf_height larger than 4.  Thus, the test is redundant.
> However, if libjpeg ever changes and sets rec_outbuf_height to something
> larger than our LINEBUF_SIZE, you'll be trampling over memory as the
> lines array is not big enough.

Yes, it's clearly better to assign it to the array size - fixed,
attached for consideration.

> Third, I ran a quick benchmark with
>
>  /usr/bin/time eog image.jpg
>
> for a 39 MB JPEG (a 11299x10764-pixel satellite image which expands t o
> about 360 MB in RAM).  Both without your patch and with the patch, it
> loads consistently in about 11.3 seconds.  I couldn't measure any
> difference.

The aim is to increase cache effectiveness on small-cache systems (ARM
etc). The increase will be small, but makes good use of the line
pointer array and code already in place. When colourspace conversion
is performed, there may be additional improvement from better locality
- it's a start.

On a separate note, is your 39MB JPEG progressively encoded?

Daniel
-- 
Daniel J Blueman

diff --git a/gdk-pixbuf/io-jpeg.c b/gdk-pixbuf/io-jpeg.c
index 680a209..77a37e6 100644
--- a/gdk-pixbuf/io-jpeg.c
+++ b/gdk-pixbuf/io-jpeg.c
@@ -46,6 +46,7 @@
 
 /* we are a "source manager" as far as libjpeg is concerned */
 #define JPEG_PROG_BUF_SIZE 65536
+#define LINEBUF_SIZE 8
 
 typedef struct {
 	struct jpeg_source_mgr pub;   /* public fields */
@@ -454,7 +455,7 @@ gdk_pixbuf__jpeg_image_load (FILE *f, GError **error)
 	char   otag_str[5];
 	GdkPixbuf * volatile pixbuf = NULL;
 	guchar *dptr;
-	guchar *lines[4]; /* Used to expand rows, via rec_outbuf_height, 
+	guchar *lines[LINEBUF_SIZE]; /* Used to expand rows, via rec_outbuf_height, 
                            * from the header file: 
                            * " Usually rec_outbuf_height will be 1 or 2, 
                            * at most 4."
@@ -511,6 +512,9 @@ gdk_pixbuf__jpeg_image_load (FILE *f, GError **error)
 	cinfo.do_fancy_upsampling = FALSE;
 	cinfo.do_block_smoothing = FALSE;
 
+	/* batch line updates for efficiency */
+	cinfo.rec_outbuf_height = LINEBUF_SIZE;
+
 	pixbuf = gdk_pixbuf_new (GDK_COLORSPACE_RGB, 
 				 cinfo.out_color_components == 4 ? TRUE : FALSE, 
 				 8, cinfo.output_width, cinfo.output_height);
@@ -738,7 +742,7 @@ gdk_pixbuf__jpeg_image_load_lines (JpegProgContext  *context,
                                    GError          **error)
 {
         struct jpeg_decompress_struct *cinfo = &context->cinfo;
-        guchar *lines[4];
+        guchar *lines[LINEBUF_SIZE];
         guchar **lptr;
         guchar *rowptr;
         gint   nlines, i;
@@ -968,6 +972,9 @@ gdk_pixbuf__jpeg_image_load_increment (gpointer data,
 			cinfo->do_fancy_upsampling = FALSE;
 			cinfo->do_block_smoothing = FALSE;
 
+			/* batch line updates f	or efficiency */
+			cinfo->rec_outbuf_height = LINEBUF_SIZE;
+
 			if (rc == JPEG_SUSPENDED)
 				continue;