bug#74476: [PATCH] Explore JPEG loading without quantization

[Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

Alan Third <alan@idiocy.org> writes:

> On Sat, Nov 30, 2024 at 07:32:10PM +0100, Manuel Giraud wrote:
>> Alan Third <alan@idiocy.org> writes:
>> 
>> > Probably I could do with finding some larger images as the whole thing
>> > completes in under a second even without your patch.
>> 
>> FYI, I have used images of 4000 by 3000 pixels.
>> 
>> > I've had a quick dig into lookup_rgb_color and assuming you have a
>> > true colour display and there's no gamma calculation going on (I don't
>> > know when that happens) it shouldn't be doing a whole lot more.
>> > Perhaps it's just the extra over-head of calling a function?
>> 
>> It seems a bit much for just a function call.  Or maybe it is the
>> init_color_table call?  Should it be done for each jpeg_load?
>
> It's 4000x3000 function calls, so it might have an effect...

Yes, you're right.  I missed this 😅.

> I think it should be done for each load. And I just noticed you need
> to add a free_color_table call after you're done with it.

Yes, I have re-add it in the attached new version of the patch.  I also
re-add the ir, ig and ib indices computation.  WDYT?

> I get that it's a waste of time allocating the table on a true colour
> display because it's never going to be used, but it seems to make no
> performance difference here and it is still needed on other display
> types.

Ok, I thought about making this allocation conditional to having a true
color display or not... but if you say that there is no performance
gain, it might not worth it.

> Purely for testing purposes I tried changing the PUT_PIXEL call to
> this:
>
>     PUT_PIXEL (ximg, x, y, x_make_truecolor_pixel (FRAME_DISPLAY_INFO(f), r, g, b));
>
> I think it might have given an improvement, but it was so slight I
> can't say for sure. That obviously can't be used outside of testing,
> but it lets us rule out the function call, etc.

Yes and the gamma correction part is also missing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Do-not-use-libjpeg-quantization-bug-74476.patch --]
[-- Type: text/x-patch, Size: 4502 bytes --]

From ebb19d8eb64a8e59dd9200f24e69c92a93c723fb Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 21 Nov 2024 17:19:59 +0100
Subject: [PATCH] Do not use libjpeg quantization (bug#74476)

* src/image.c (jpeg_load_body): Remove libjpeg quantization.
---
 src/image.c | 80 +++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 49 deletions(-)

diff --git a/src/image.c b/src/image.c
index db7f6acd171..b5e7bef2305 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8949,9 +8949,8 @@ jpeg_load_body (struct frame *f, struct image *img,
   FILE *fp = NULL;
   JSAMPARRAY buffer;
   int row_stride, x, y;
-  int width, height;
-  int i, ir, ig, ib;
-  unsigned long *colors;
+  int width, height, ncomp;
+  int ir, ig, ib;
   Emacs_Pix_Container volatile ximg_volatile = NULL;
 
   /* Open the JPEG file.  */
@@ -9049,12 +9048,17 @@ jpeg_load_body (struct frame *f, struct image *img,
 
   jpeg_read_header (&mgr->cinfo, 1);
 
-  /* Customize decompression so that color quantization will be used.
-	 Start decompression.  */
-  mgr->cinfo.quantize_colors = 1;
+  /* Start decompression.  */
   jpeg_start_decompress (&mgr->cinfo);
   width = img->width = mgr->cinfo.output_width;
   height = img->height = mgr->cinfo.output_height;
+  ncomp = mgr->cinfo.output_components;
+  if (ncomp > 2)
+    ir = 0, ig = 1, ib = 2;
+  else if (ncomp > 1)
+    ir = 0, ig = 1, ib = 0;
+  else
+    ir = 0, ig = 0, ib = 0;
 
   if (!check_image_size (f, width, height))
     {
@@ -9073,55 +9077,34 @@ jpeg_load_body (struct frame *f, struct image *img,
       sys_longjmp (mgr->setjmp_buffer, 1);
     }
 
-  /* Allocate colors.  When color quantization is used,
-     mgr->cinfo.actual_number_of_colors has been set with the number of
-     colors generated, and mgr->cinfo.colormap is a two-dimensional array
-     of color indices in the range 0..mgr->cinfo.actual_number_of_colors.
-     No more than 255 colors will be generated.  */
-  USE_SAFE_ALLOCA;
-  {
-    if (mgr->cinfo.out_color_components > 2)
-      ir = 0, ig = 1, ib = 2;
-    else if (mgr->cinfo.out_color_components > 1)
-      ir = 0, ig = 1, ib = 0;
-    else
-      ir = 0, ig = 0, ib = 0;
-
-    /* Use the color table mechanism because it handles colors that
-       cannot be allocated nicely.  Such colors will be replaced with
-       a default color, and we don't have to care about which colors
-       can be freed safely, and which can't.  */
-    init_color_table ();
-    SAFE_NALLOCA (colors, 1, mgr->cinfo.actual_number_of_colors);
-
-    for (i = 0; i < mgr->cinfo.actual_number_of_colors; ++i)
-      {
-	/* Multiply RGB values with 255 because X expects RGB values
-	   in the range 0..0xffff.  */
-	int r = mgr->cinfo.colormap[ir][i] << 8;
-	int g = mgr->cinfo.colormap[ig][i] << 8;
-	int b = mgr->cinfo.colormap[ib][i] << 8;
-	colors[i] = lookup_rgb_color (f, r, g, b);
-      }
-
-#ifdef COLOR_TABLE_SUPPORT
-    /* Remember those colors actually allocated.  */
-    img->colors = colors_in_color_table (&img->ncolors);
-    free_color_table ();
-#endif /* COLOR_TABLE_SUPPORT */
-  }
-
-  /* Read pixels.  */
-  row_stride = width * mgr->cinfo.output_components;
+  /* Allocate scanlines buffer and Emacs color table.  */
+  row_stride = width * ncomp;
   buffer = mgr->cinfo.mem->alloc_sarray ((j_common_ptr) &mgr->cinfo,
 					 JPOOL_IMAGE, row_stride, 1);
+  init_color_table ();
+
+  /* Fill the X image from JPEG data.  */
   for (y = 0; y < height; ++y)
     {
       jpeg_read_scanlines (&mgr->cinfo, buffer, 1);
-      for (x = 0; x < mgr->cinfo.output_width; ++x)
-	PUT_PIXEL (ximg, x, y, colors[buffer[0][x]]);
+      for (x = 0; x < width; ++x)
+	{
+	  int off = x * ncomp;
+	  /* Multiply RGB values with 255 because X expects RGB values
+	     in the range 0..0xffff.  */
+	  int r = buffer[0][off + ir] << 8;
+	  int g = buffer[0][off + ig] << 8;
+	  int b = buffer[0][off + ib] << 8;
+	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, r, g, b));
+	}
     }
 
+#ifdef COLOR_TABLE_SUPPORT
+  /* Remember those colors actually allocated.  */
+  img->colors = colors_in_color_table (&img->ncolors);
+  free_color_table ();
+#endif /* COLOR_TABLE_SUPPORT */
+
   /* Clean up.  */
   jpeg_finish_decompress (&mgr->cinfo);
   jpeg_destroy_decompress (&mgr->cinfo);
@@ -9135,7 +9118,6 @@ jpeg_load_body (struct frame *f, struct image *img,
 
   /* Put ximg into the image.  */
   image_put_x_image (f, img, ximg, 0);
-  SAFE_FREE ();
   return 1;
 }
 
-- 
2.47.0


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]

-- 
Manuel Giraud