[quake3-bugzilla] [Bug 4564] Use system libjpeg instead of bundled copy (optionally)

[bugzilla-daemon at icculus.org]

https://bugzilla.icculus.org/show_bug.cgi?id=4564

Thilo Schulz <arny at ats.s.bawue.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |arny at ats.s.bawue.de

Simon McVittie <smcv-ioquake3 at pseudorandom.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |smcv-ioquake3 at pseudorandom.
                   |                            |co.uk

--- Comment #5 from Thilo Schulz <arny at ats.s.bawue.de> 2011-02-08 19:18:16 EST ---
Hey! I would like to apply your patch, but before I do so I need a few
questions answered, and issues resolved.

1. why does jmemnobs.c need to be changed at all? Isn't it part of the system
lib already? If we use the system jpeg lib, why do we need to use jmemnobs.c
then?

2. When and how does the jpeg_mem_src() function get used in the jpeg library?

3. Why the extra c-file jpeg_memsrc for the jpeg_mem_src function? Can't we
just put all into tr_image_jpeg.c?

4. Why this?
-  row_stride = cinfo.output_width * cinfo.output_components;
+  row_stride = cinfo.output_width * 4;

I changed it from 4 to cinfo.output_components because not all JPEGs have 4
channels, like 8-bit greyscale JPEGs.

5. +    /* we have RGB data, we need to expand this out to ARGB */

How come system jpeg lib returns RGB data and builtin jpeglib does return RGBA?
Was the builtin jpeg lib code modified to return RGBA data? It would feel a bit
more consistent if we had the same behaviour for the builtin jpeglib than for
the system jpeg lib.

--- Comment #6 from Simon McVittie <smcv-ioquake3 at pseudorandom.co.uk> 2011-02-10 13:13:35 EST ---
(In reply to comment #5)
> 1. why does jmemnobs.c need to be changed at all? Isn't it part of the system
> lib already? If we use the system jpeg lib, why do we need to use jmemnobs.c
> then?

libjpeg has several interchangeable memory allocators; which one actually gets
used is a compile-time choice. They come from different source files which all
export the same symbols, so you have to link with exactly one of them. jmemnobs
is one of the options.

Q3 uses a modified jmemnobs.c to allocate temporary libjpeg memory on the Q3
hunk instead of the system (e.g. glibc) heap. The system libjpeg is almost
certainly compiled to use the system heap (malloc/free).

I believe the intention here is that the symbols in the executable will
override the ones in libjpeg, causing its memory allocator/deallocator to be
substituted. I'm not sure whether that actually happens, mind... the other
possibility is that the ones in jmemnobs.c are ignored and we use malloc/free,
which is harmless as long as we're consistent about it.

Because jmemnobs.c is (originally) part of libjpeg, it uses internal-only APIs,
accessed by defining JPEG_INTERNALS. The modifications to jmemnobs.c make it
use only the public API of libjpeg if we're using the system libjpeg; they rely
on the system not being DOS or something, but that seems reasonable :-)

I'm not sure how important it is that we use the hunk for these temporary
allocations; it looks as though jpeg_destroy_decompress should free all the
memory it was using anyway, so maybe we can just let libjpeg use the system
malloc/free.

> 2. When and how does the jpeg_mem_src() function get used in the jpeg library?

It's called by tr_image_jpeg, rather than from within libjpeg. One of the Q3
changes was to replace the standard jpeg_stdio_src with an essentially
equivalent jpeg_mem_src, but when we're using the system libjpeg, we can't rely
on it having that modification, and we still need to supply our own
jpeg_mem_src.

The cleanest way to do this would probably be one of:

A. revert the changes to Q3's jdatasrc.c, and have the renderer always use the
same jpeg_mem_src that we'd use with the system libjpeg

B. require the system libjpeg to be version 8 or later, and use jpeg_mem_src
from that

However, libjpeg6b is part of the LSB, libjpeg8 isn't binary-compatible, and
I'm not sure how much will catch fire if we're linked against libjpeg8 but our
copy of SDL or whatever is linked against libjpeg6b. Debian started a
transition to libjpeg8 a few months ago, but then reverted it all because of
that sort of problem.

> 3. Why the extra c-file jpeg_memsrc for the jpeg_mem_src function? Can't we
> just put all into tr_image_jpeg.c?

Conceptually, it's an independent piece of utility code for libjpeg, rather
than part of the Q3 renderer. As I said, in libjpeg 8 it's been incorporated
into libjpeg.

> 4. Why this?
> -  row_stride = cinfo.output_width * cinfo.output_components;
> +  row_stride = cinfo.output_width * 4;
> 
> I changed it from 4 to cinfo.output_components because not all JPEGs have 4
> channels, like 8-bit greyscale JPEGs.

I think you're right, this probably does the wrong thing for 8-bit greyscale
JPEGs; thanks, I didn't spot that while importing the Fedora patch.

It'd probably be feasible to do the expansion from (8-bit greyscale|24-bit RGB)
to 32-bit RGBA just after we decompress a scanline, avoiding the fixup loop
afterwards?

To be honest, this could benefit from a regression test. I'll try to find time
to write one.

> 5. +    /* we have RGB data, we need to expand this out to ARGB */
> 
> How come system jpeg lib returns RGB data and builtin jpeglib does return RGBA?
> Was the builtin jpeg lib code modified to return RGBA data?

Sort of. Q3's libjpeg isn't binary-compatible with "real libjpeg", because of
this change:

-#define RGB_PIXELSIZE  3       /* JSAMPLEs per RGB scanline element */
+#define RGB_PIXELSIZE  4       /* JSAMPLEs per RGB scanline element */

Increasing RGB_PIXELSIZE means we write 4 bytes for every 3 bytes read, which
is a slightly cheating way to get a dummy alpha channel containing
uninitialized data; tr_image_jpg.c goes through setting them all to 255 after
the JPEG has been decompressed.

-- 
Configure bugmail: https://bugzilla.icculus.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.