GNU bug report logs - #74476
[PATCH] Explore JPEG loading without quantization

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74476 in the body.
You can then email your comments to 74476 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Explore JPEG loading without quantization
Date: Fri, 22 Nov 2024 15:53:18 +0100

[Message part 1 (text/plain, inline)]

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'

[0001-Explore-JPEG-loading-without-quantization.patch (text/patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #8 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>, Alan Third <alan <at> idiocy.org>
Cc: 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 12:19:16 +0200

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

Message #11 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Manuel Giraud <manuel <at> ledu-giraud.fr>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 11:44:29 +0000

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

Message #14 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 15:54:38 +0100

Alan Third <alan <at> 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

Message #17 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 15:37:29 +0000

On Sat, Nov 30, 2024 at 03:54:38PM +0100, Manuel Giraud wrote:
> Alan Third <alan <at> 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

Message #20 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 17:26:03 +0100

Alan Third <alan <at> idiocy.org> writes:

> On Sat, Nov 30, 2024 at 03:54:38PM +0100, Manuel Giraud wrote:
>> Alan Third <alan <at> 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

Message #23 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 18:25:01 +0100

[Message part 1 (text/plain, inline)]

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)

[0001-Explore-JPEG-loading-without-quantization.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #26 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 18:16:39 +0000

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

Message #29 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 19:32:10 +0100

Alan Third <alan <at> 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

Message #32 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 19:49:41 +0100

Alan Third <alan <at> 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

Message #35 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sat, 30 Nov 2024 19:55:17 +0000

On Sat, Nov 30, 2024 at 07:32:10PM +0100, Manuel Giraud wrote:
> Alan Third <alan <at> 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

Message #38 received at 74476 <at> debbugs.gnu.org (full text, mbox):

From: Manuel Giraud <manuel <at> ledu-giraud.fr>
To: Alan Third <alan <at> idiocy.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74476 <at> debbugs.gnu.org
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Sun, 01 Dec 2024 12:13:53 +0100

[Message part 1 (text/plain, inline)]

Alan Third <alan <at> idiocy.org> writes:

> On Sat, Nov 30, 2024 at 07:32:10PM +0100, Manuel Giraud wrote:
>> Alan Third <alan <at> 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.

[0001-Do-not-use-libjpeg-quantization-bug-74476.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Manuel Giraud

Message #43 received at 74476-done <at> debbugs.gnu.org (full text, mbox):

From: Alan Third <alan <at> idiocy.org>
To: Manuel Giraud <manuel <at> ledu-giraud.fr>
Cc: 74476-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#74476: [PATCH] Explore JPEG loading without quantization
Date: Mon, 2 Dec 2024 10:47:42 +0000

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.