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

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

Hi,

While trying to speed up "emacs as an image viewer", I found that emacs
is using libjpeg with color quantization and it seems that removing this
quantization could speed up JPEG loading a bit.

My simple limited benchmark:

          - M-: (clear-image-cache)
          - Open an image in folder with some large enough pictures in
            it (4000x3000 here)
          - M-: (benchmark-run 10 (image-next-file 1))

Here are the results I get:

without this path: (5.415405491 1 0.09232176400000025)
with: (3.079911418 1 0.0751190459999993)

I don't think that this patch could be applied as is (it is rather
ugly).  And I also think that I probably have missed some (many?) use
case (where color quantization is mandatory).  But I'm submitting this
patch anyway as a conversation starter on the subject.

Thanks,

In GNU Emacs 31.0.50 (build 9, x86_64-unknown-openbsd7.6, X toolkit) of
 2024-11-22 built on computer
Repository revision: c66c0942ea9ac10e6d6324e472150de403a03b69
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101014
System Description: OpenBSD computer 7.6 GENERIC.MP#437 amd64

Configured using:
 'configure CC=egcc CPPFLAGS=-I/usr/local/include
 LDFLAGS=-L/usr/local/lib MAKEINFO=gmakeinfo --prefix=/home/manuel/emacs
 --bindir=/home/manuel/bin --with-x-toolkit=lucid
 --with-toolkit-scroll-bars=no --without-cairo
 --without-compress-install'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Explore-JPEG-loading-without-quantization.patch --]
[-- Type: text/patch, Size: 3823 bytes --]

From e360545b65427d6f0eec05cdeb26b8a05d8a0d61 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] Explore JPEG loading without quantization

---
 src/image.c | 63 +++++++++++++----------------------------------------
 1 file changed, 15 insertions(+), 48 deletions(-)

diff --git a/src/image.c b/src/image.c
index db7f6acd171..374d2f9c454 100644
--- a/src/image.c
+++ b/src/image.c
@@ -8949,9 +8949,9 @@ 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 i, off;
+  unsigned long color;
   Emacs_Pix_Container volatile ximg_volatile = NULL;
 
   /* Open the JPEG file.  */
@@ -9049,12 +9049,11 @@ 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 (!check_image_size (f, width, height))
     {
@@ -9073,53 +9072,22 @@ 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;
+  row_stride = width * ncomp;
   buffer = mgr->cinfo.mem->alloc_sarray ((j_common_ptr) &mgr->cinfo,
 					 JPOOL_IMAGE, row_stride, 1);
   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)
+	{
+	  color = 0;
+	  off = x * ncomp;
+	  /* XXX I suck at bit twiddling.  */
+	  for (i = 0; i < ncomp; ++i)
+	    color += (buffer[0][off + i] << ((ncomp - 1 - i) * 8));
+	  PUT_PIXEL (ximg, x, y, color);
+	}
     }
 
   /* Clean up.  */
@@ -9135,7 +9103,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

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

[Eli Zaretskii]

> Date: Fri, 22 Nov 2024 15:53:18 +0100
> From:  Manuel Giraud via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> While trying to speed up "emacs as an image viewer", I found that emacs
> is using libjpeg with color quantization and it seems that removing this
> quantization could speed up JPEG loading a bit.
> 
> My simple limited benchmark:
> 
>           - M-: (clear-image-cache)
>           - Open an image in folder with some large enough pictures in
>             it (4000x3000 here)
>           - M-: (benchmark-run 10 (image-next-file 1))
> 
> Here are the results I get:
> 
> without this path: (5.415405491 1 0.09232176400000025)
> with: (3.079911418 1 0.0751190459999993)
> 
> I don't think that this patch could be applied as is (it is rather
> ugly).  And I also think that I probably have missed some (many?) use
> case (where color quantization is mandatory).  But I'm submitting this
> patch anyway as a conversation starter on the subject.

Alan, any comments?  I know nothing about this "color quantization"
aspect of JPEG images.

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

[Alan Third]

On Sat, Nov 30, 2024 at 12:19:16PM +0200, Eli Zaretskii wrote:
> > Date: Fri, 22 Nov 2024 15:53:18 +0100
> > From:  Manuel Giraud via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > 
> > While trying to speed up "emacs as an image viewer", I found that emacs
> > is using libjpeg with color quantization and it seems that removing this
> > quantization could speed up JPEG loading a bit.
> > 
> > My simple limited benchmark:
> > 
> >           - M-: (clear-image-cache)
> >           - Open an image in folder with some large enough pictures in
> >             it (4000x3000 here)
> >           - M-: (benchmark-run 10 (image-next-file 1))
> > 
> > Here are the results I get:
> > 
> > without this path: (5.415405491 1 0.09232176400000025)
> > with: (3.079911418 1 0.0751190459999993)
> > 
> > I don't think that this patch could be applied as is (it is rather
> > ugly).  And I also think that I probably have missed some (many?) use
> > case (where color quantization is mandatory).  But I'm submitting this
> > patch anyway as a conversation starter on the subject.
> 
> Alan, any comments?  I know nothing about this "color quantization"
> aspect of JPEG images.

Hi Eli,

I'm not an expert on this part of the image code, but I think the jpeg
colour quantization path exists for colour mapped environments. For
example you can pass it an array of 256 colours and it will map all
the colours in the jpeg to those colours.

I don't think we need to do this. In most cases we're not in a colour
mapped environment, and the existing code doesn't ask the jpeg code to
map to the existing colour map anyway.

AFAICT Emacs provides its own functions for colour mapping, so I think
we should try turning off this quantization and rely on our own
methods. That works for PNGs and others, after all.

Manuel, I think you want to use lookup_rgb_color when setting the
pixels in the final image buffer. This should do the right thing. You
presumably also need to call init_color_table before using it, and I
see calls to colors_in_color_table and free_color_table which look
necessary, but I think they're already in the jpeg code.
-- 
Alan Third

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

[...]

> Manuel, I think you want to use lookup_rgb_color when setting the
> pixels in the final image buffer. This should do the right thing. You
> presumably also need to call init_color_table before using it, and I
> see calls to colors_in_color_table and free_color_table which look
> necessary, but I think they're already in the jpeg code.

Hi Alan,

I don't really understand: my proposed patch is getting rid of calls to
lookup_rgb_color and init_color_table.
-- 
Manuel Giraud

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

[Alan Third]

On Sat, Nov 30, 2024 at 03:54:38PM +0100, Manuel Giraud wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> [...]
> 
> > Manuel, I think you want to use lookup_rgb_color when setting the
> > pixels in the final image buffer. This should do the right thing. You
> > presumably also need to call init_color_table before using it, and I
> > see calls to colors_in_color_table and free_color_table which look
> > necessary, but I think they're already in the jpeg code.
> 
> Hi Alan,
> 
> I don't really understand: my proposed patch is getting rid of calls to
> lookup_rgb_color and init_color_table.

Yes, but they're internal Emacs functions, not related to libjpeg. All
the other image library code uses them, so I think they are necessary
to handle systems that use colour mapping (and possibly Windows too,
it has a different implementation of lookup_rgb_color).

For example, the PNG code looks like this:

  /* Fill the X image and mask from PNG data.  */
  init_color_table ();

  for (y = 0; y < height; ++y)
    {
      png_byte *p = rows[y];

      for (x = 0; x < width; ++x)
	{
	  int r, g, b;

	  r = *p++ << 8;
	  g = *p++ << 8;
	  b = *p++ << 8;
	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, r, g, b));
  ...

-- 
Alan Third

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

> On Sat, Nov 30, 2024 at 03:54:38PM +0100, Manuel Giraud wrote:
>> Alan Third <alan@idiocy.org> writes:
>> 
>> [...]
>> 
>> > Manuel, I think you want to use lookup_rgb_color when setting the
>> > pixels in the final image buffer. This should do the right thing. You
>> > presumably also need to call init_color_table before using it, and I
>> > see calls to colors_in_color_table and free_color_table which look
>> > necessary, but I think they're already in the jpeg code.
>> 
>> Hi Alan,
>> 
>> I don't really understand: my proposed patch is getting rid of calls to
>> lookup_rgb_color and init_color_table.
>
> Yes, but they're internal Emacs functions, not related to libjpeg. All
> the other image library code uses them, so I think they are necessary
> to handle systems that use colour mapping (and possibly Windows too,
> it has a different implementation of lookup_rgb_color).
>
> For example, the PNG code looks like this:

Ok.  Thanks for the clarification, I'll modify my patch.
-- 
Manuel Giraud

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Here is a new version with Alan suggestions.

FTR, the gain is much less interesting.  Here is the new timing with
same benchmark (see below):
(4.381739669 1 0.1034133749999997)

> My simple limited benchmark:
> 
>           - M-: (clear-image-cache)
>           - Open an image in folder with some large enough pictures in
>             it (4000x3000 here)
>           - M-: (benchmark-run 10 (image-next-file 1))
> 
> Here are the results I get:
> 
> without this path: (5.415405491 1 0.09232176400000025)
> with: (3.079911418 1 0.0751190459999993)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Explore-JPEG-loading-without-quantization.patch --]
[-- Type: text/x-patch, Size: 3903 bytes --]

From abf0a548ee1fa66db4bdf1051cd605231e29cf55 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] Explore JPEG loading without quantization

---
 src/image.c | 66 ++++++++++++++---------------------------------------
 1 file changed, 17 insertions(+), 49 deletions(-)

diff --git a/src/image.c b/src/image.c
index db7f6acd171..a31c2e2658a 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 off, r, g, b;
   Emacs_Pix_Container volatile ximg_volatile = NULL;
 
   /* Open the JPEG file.  */
@@ -9049,12 +9048,11 @@ 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 (!check_image_size (f, width, height))
     {
@@ -9073,53 +9071,24 @@ 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)
+	{
+	  off = x * ncomp;
+	  r = buffer[0][off] << 8;
+	  g = buffer[0][off + 1] << 8;
+	  b = buffer[0][off + 2] << 8;
+	  PUT_PIXEL (ximg, x, y, lookup_rgb_color (f, r, g, b));
+	}
     }
 
   /* Clean up.  */
@@ -9135,7 +9104,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

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

[Alan Third]

On Sat, Nov 30, 2024 at 06:25:01PM +0100, Manuel Giraud wrote:
> Here is a new version with Alan suggestions.
> 
> FTR, the gain is much less interesting.  Here is the new timing with
> same benchmark (see below):
> (4.381739669 1 0.1034133749999997)

That's unfortunate, although here I see an improvement of greater than
2x. Probably I could do with finding some larger images as the whole
thing completes in under a second even without your patch.

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?

-- 
Alan Third

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

> On Sat, Nov 30, 2024 at 06:25:01PM +0100, Manuel Giraud wrote:
>> Here is a new version with Alan suggestions.
>> 
>> FTR, the gain is much less interesting.  Here is the new timing with
>> same benchmark (see below):
>> (4.381739669 1 0.1034133749999997)
>
> That's unfortunate, although here I see an improvement of greater than
> 2x.

That's good news.

> 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?
-- 
Manuel Giraud

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Alan Third <alan@idiocy.org> writes:

> On Sat, Nov 30, 2024 at 06:25:01PM +0100, Manuel Giraud wrote:
>> Here is a new version with Alan suggestions.
>> 
>> FTR, the gain is much less interesting.  Here is the new timing with
>> same benchmark (see below):
>> (4.381739669 1 0.1034133749999997)
>
> That's unfortunate, although here I see an improvement of greater than
> 2x. Probably I could do with finding some larger images as the whole
> thing completes in under a second even without your patch.
>
> 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?

I also have a quick look at lookup_rgb_color and it seems to me that if
the user has a true color display (I suspect almost everyone nowadays),
we can spare the computation ct_hash_rgb and hence also spare to
allocate a ct_table.  WDYT?
-- 
Manuel Giraud

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

[Alan Third]

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...

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.

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.

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.
-- 
Alan Third

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

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- 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

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

[Alan Third]

On Sun, Dec 01, 2024 at 12:13:53PM +0100, Manuel Giraud wrote:
> Alan Third <alan@idiocy.org> writes:
> 
> > 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?

Looks good to me.

I've pushed this to master. Thanks!
-- 
Alan Third