Reassigning this to ioquake3 since it's their sort of thing.
I think it's a good idea but it needs work because it doesn't apply cleanly anymore, and it's not obvious how to fix since we've upgraded jpeg-6 to 6b so I don't know whether all of the patch is still relevant.
Created attachment 2397[details]
patch applicable to svn r1788, from Fedora via Debian
For what it's worth, here's the patch we're trying out in Debian, derived from Fedora. It applies cleanly to svn r1788 (which contains libjpeg 6b), and appears to work with Debian's libjpeg62 version 6b1-1.
I realise the ioquake3 maintainers have historically avoided using the system libjpeg at all, but it would be great if you could make its use an option (it doesn't have to be the default).
You might also be interested to know that from libjpeg 8 onwards, there's API for jpeg_mem_src and jpeg_mem_dest, which makes most of the ioquake3 changes unnecessary.
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.
(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.
(In reply to comment #6)
> 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.
Well, I'd rather have it that system libjpeg uses system malloc/free. In this case we don't need to enforce using Z_Malloc/Z_Free under all circumstances, and can get rid of jmemnobs.c entirely.
> 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.
Yes! that's reasonable.
> 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
This is the way to go, I guess.
> 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.
Then I would recommend building ioquake3 with system jpeglib enabled only for distributors, where distributors can be sure they have the required dependencies fulfilled. That sounds reasonable.
> > 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.
Please put that function into tr_image_jpeg.c ... I really don't see the need to add yet another source file just for this one function.
> 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?
Yeah. You can do that probably.
> To be honest, this could benefit from a regression test. I'll try to find time
> to write one.
Please do!
> 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.
Well, let's get rid of that hack in the libjpeg included with ioquake3 so that system jpeglib and ioq3 jpeglib is consistent in that. You can probably make the expansion from 3 to 4 components at the same place where you expand from 1 to 4 components for 8-bit jpegs.
Remember: Don't forget to update the ioq3-changes.diff when you're done!
If you're going to move on this it probably makes sense to upgrade to libjpeg 8 at the same time. i.e. put the libjpeg 8 sources in our tree and provide optional means to link to it externally.
I *think* version 8 includes a custom memory management API like the extensions id added to version 6 in order to use Z_Malloc.
(In reply to comment #8)
> If you're going to move on this it probably makes sense to upgrade to libjpeg 8
> at the same time. i.e. put the libjpeg 8 sources in our tree and provide
> optional means to link to it externally.
>
> I *think* version 8 includes a custom memory management API like the extensions
> id added to version 6 in order to use Z_Malloc.
I agree. It would probably make sense moving to libjpeg8. I know Simon probably is not gonna like it, as Debian seems to stay at the old version 6
(In reply to comment #9)
> (In reply to comment #8)
> > If you're going to move on this it probably makes sense to upgrade to libjpeg 8
> > at the same time. i.e. put the libjpeg 8 sources in our tree and provide
> > optional means to link to it externally.
Potentially. I'll look at using an external libjpeg6 first (because that's what Debian and Fedora have, and they're the reasons for this change), then try with libjpeg8.
> > I *think* version 8 includes a custom memory management API like the extensions
> > id added to version 6 in order to use Z_Malloc.
>
> I agree. It would probably make sense moving to libjpeg8. I know Simon probably
> is not gonna like it, as Debian seems to stay at the old version 6
If they're close enough (which I believe they are - they're mostly API-compatible), we could probably patch ioquake3 for libjpeg6 support until we move to libjpeg8; it's better than having to patch it for system-libjpeg support at all!
(In reply to comment #6)
> 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.
Looking at ldd output for Debian's ioquake3, it seems the part of SDL we link to doesn't actually use libjpeg, so we can probably move to libjpeg8 ahead of the rest of the distribution if necessary; and if one of our dependencies starts using libjpeg6b for some reason, versioned symbols will hopefully save us.
(We *have* libjpeg8, it's just the transition to using it by default that got reverted, as a result of the two-versions-in-one-process problem: <http://lists.debian.org/debian-release/2010/02/msg00147.html>. We now have versioned symbols in both 6b and 8 which should avoid those crashes.)
(In reply to comment #11)
> (We *have* libjpeg8, it's just the transition to using it by default that got
> reverted, as a result of the two-versions-in-one-process problem:
> <http://lists.debian.org/debian-release/2010/02/msg00147.html>. We now have
> versioned symbols in both 6b and 8 which should avoid those crashes.)
Do you want to make the patch? we should use libjpeg8c, so just replace all of jpeg-6b. I'll do it if you won't.
(In reply to comment #12)
> Do you want to make the patch? we should use libjpeg8c, so just replace all of
> jpeg-6b. I'll do it if you won't.
Feel free to work on it; I don't have a massive amount of time for ioq3 development right now, but will pick this up when I can.
The ideal thing from Debian's point of view would be if the bundled version of libjpeg is an unmodified copy of *some* version of IJG libjpeg, or failing that, minimally-modified in ways that are compatible. I don't really mind which version you pick. We only have version 8b in unstable at the moment, but the only change between 8b and 8c seems to be an extra feature in the compression side, so I doubt we need to care about that difference, and I agree it makes sense for you to choose the latest version.
It looks as though jpeg_mem_src in libjpeg 8 removes the need for one of Q3's modifications, and we have an idea of how to fix the 4-byte/3-byte thing mentioned above, so the main remaining thing seems to be the malloc/free glue. I had a look at jpeg_memory_mgr a while ago, but it seems more like an internal abstraction layer than something that's designed to be replaceable by the host application?
(In reply to comment #13)
> Feel free to work on it; I don't have a massive amount of time for ioq3
> development right now, but will pick this up when I can.
Ok... sometime in the coming days i will
> The ideal thing from Debian's point of view would be if the bundled version of
> libjpeg is an unmodified copy of *some* version of IJG libjpeg, or failing
> that, minimally-modified in ways that are compatible.
Yes, don't worry. That's my plan as well.
> It looks as though jpeg_mem_src in libjpeg 8 removes the need for one of Q3's
> modifications, and we have an idea of how to fix the 4-byte/3-byte thing
> mentioned above, so the main remaining thing seems to be the malloc/free glue.
I don't consider it an issue - we can use the system malloc() and free() handlers. When linking against libz that's what happens, too.
> I had a look at jpeg_memory_mgr a while ago, but it seems more like an internal
> abstraction layer than something that's designed to be replaceable by the host
> application?
If I find an easy way to replace the memory allocaters with custom functions I'll do that of course.
(In reply to comment #13)
> (In reply to comment #12)
> > Do you want to make the patch? we should use libjpeg8c, so just replace all of
> > jpeg-6b. I'll do it if you won't.
>
> Feel free to work on it; I don't have a massive amount of time for ioq3
> development right now, but will pick this up when I can.
>
> The ideal thing from Debian's point of view would be if the bundled version of
> libjpeg is an unmodified copy of *some* version of IJG libjpeg, or failing
libjpeg is the last library we bundle so if the need for that is gone I'd really like to to see the code copies of all the bundled libs gone completely. We can keep the Makefile rules nevertheless so anyone who wants to use built-in
libs can just untar the upstream sources in code/.
(In reply to comment #15)
> libjpeg is the last library we bundle so if the need for that is gone I'd
> really like to to see the code copies of all the bundled libs gone completely.
I'd been aiming for a compromise, because the position in the past seems to have been that the bundled libraries were essential for whatever reason (to be nice to Windows and other deficient OSs?), but if the current maintainers' opinions on bundled libraries have changed, an ioquake3 with no bundled libraries at all would be just as good, if not better.
If we move them to a separate svn or simple zip file (heck, there could even be a Makefile target that downloads it for the extra lazy!) everyone could be made happy you know.
Created attachment 2634[details]
Revert increased RGB_PIXELSIZE in libjpeg
Instead, stretch RGB to RGBA in tr_image_jpg, like we do for 8-bit
greyscale.
(As discussed above; this makes it easier to use system libjpeg.)
Created attachment 2635[details]
allow use of system libjpeg (6b or later)
Loosely based on the patch from Fedora, but with a considerably smaller
diffstat, and with the default and the sense of the variable switched.
(Still to do: upgrade to libjpeg8.)
(In reply to comment #11)
> Looking at ldd output for Debian's ioquake3, it seems the part of SDL we link
> to doesn't actually use libjpeg, so we can probably move to libjpeg8 ahead of
> the rest of the distribution if necessary
Unfortunately, libjpeg 6b and 8 headers can't be installed at the same time (ugh), and libsdl1.2-dev currently depends on libjpeg6b-dev, so using an external libjpeg8 is still tricky on Debian. I'll try to get that working again; supplying our own jpeg_mem_src (only) seems to work OK, and perhaps I can even use the one from the bundled libjpeg8 with the rest of the system libjpeg6b.
Did you intend to kill off greyscale support? We previously had code to convert 8-bit greyscale to 32-bit RGBA...
I now realise this could be done much more simply than my second patch though. Pseudo-patch:
- ... || cinfo.output_components != 3
+ ... || (cinfo.output_components != 3 && cinfo.output_components != 1)
and
do
{
+ int i;
buf[--dindex] = 255;
- buf[--dindex] = buf[--sindex];
- buf[--dindex] = buf[--sindex];
- buf[--dindex] = buf[--sindex];
+ for (i = 0; i < cinfo.output_components; i++)
+ buf[--dindex] = buf[--sindex];
} while(sindex);
Created attachment 2639[details]
Allow use of system libjpeg version 6b (the LSB-blessed version)
For this version we need to supply a jpeg_mem_src implementation; the one
from our bundled jpeg-8c seems to work fine.
---
Not my most elegant patch ever, but it seems to work; for the moment we'll need to do this or something very similar in Debian, and probably other distributions (Fedora seems to use libjpeg-turbo, whose ABI is apparently compatible with libjpeg 6b).
I wish libjpeg had parallel-installable headers between major versions, it'd make life much easier... but it's not something distributions can unilaterally add without causing problems for upstreams.
(In reply to comment #23)
> Unfortunately, libjpeg 6b and 8 headers can't be installed at the same time
> (ugh), and libsdl1.2-dev currently depends on libjpeg6b-dev, so using an
> external libjpeg8 is still tricky on Debian. I'll try to get that working
> again; supplying our own jpeg_mem_src (only) seems to work OK, and perhaps I
> can even use the one from the bundled libjpeg8 with the rest of the system
> libjpeg6b.
Hmm. I'd like to do without that workaround, as this hack should not be required in future versions of debian. It seems to me you distributers have pretty good mechanisms to manage required patches that work around shortcomings in the distribution frameworks?
> Did you intend to kill off greyscale support? We previously had code to
> convert
> 8-bit greyscale to 32-bit RGBA...
It's not killed off. Reading through the libjpeg docs I just realized we can have the library do the conversion. Check out the parameter set after the header is read in.
Speaking of libSDL, you said it had a dependency on libjpeg6? Surely you mean libSDL_image.so? I'd like to know whether it would be feasible or plausible to offload all image loading functions to libSDL_image?
> Speaking of libSDL, you said it had a dependency on libjpeg6? Surely you mean
> libSDL_image.so? I'd like to know whether it would be feasible or plausible to
> offload all image loading functions to libSDL_image?
Probably not all. I just saw it doesn't support saving to any image formats. But maybe it would make sense as an additional, fallback image loader which would enable ioquake3 to support a large number of file formats if so desired with little effort.
(In reply to comment #26)
> It seems to me you distributers have
> pretty good mechanisms to manage required patches that work around shortcomings
> in the distribution frameworks?
We can apply distro-specific patches, sure; they're a lot easier to manage now there aren't so many of them.
> Reading through the libjpeg docs I just realized we can
> have the library do the conversion.
Ah, even better :-)
> Speaking of libSDL, you said it had a dependency on libjpeg6? Surely you mean
> libSDL_image.so?
libSDL1.2.so doesn't require libjpeg62, but the development (headers, static library) package indirectly depends on its headers, because there's a DirectFB output plugin, and DirectFB links against libjpeg. Or something.
Created attachment 2312 [details] First chunk of the patchCreated attachment 2313 [details] Second chunk of the patchCreated attachment 2397 [details] patch applicable to svn r1788, from Fedora via Debian For what it's worth, here's the patch we're trying out in Debian, derived from Fedora. It applies cleanly to svn r1788 (which contains libjpeg 6b), and appears to work with Debian's libjpeg62 version 6b1-1. I realise the ioquake3 maintainers have historically avoided using the system libjpeg at all, but it would be great if you could make its use an option (it doesn't have to be the default). You might also be interested to know that from libjpeg 8 onwards, there's API for jpeg_mem_src and jpeg_mem_dest, which makes most of the ioquake3 changes unnecessary.Created attachment 2634 [details] Revert increased RGB_PIXELSIZE in libjpeg Instead, stretch RGB to RGBA in tr_image_jpg, like we do for 8-bit greyscale. (As discussed above; this makes it easier to use system libjpeg.)Created attachment 2635 [details] allow use of system libjpeg (6b or later) Loosely based on the patch from Fedora, but with a considerably smaller diffstat, and with the default and the sense of the variable switched. (Still to do: upgrade to libjpeg8.)Created attachment 2639 [details] Allow use of system libjpeg version 6b (the LSB-blessed version) For this version we need to supply a jpeg_mem_src implementation; the one from our bundled jpeg-8c seems to work fine. --- Not my most elegant patch ever, but it seems to work; for the moment we'll need to do this or something very similar in Debian, and probably other distributions (Fedora seems to use libjpeg-turbo, whose ABI is apparently compatible with libjpeg 6b). I wish libjpeg had parallel-installable headers between major versions, it'd make life much easier... but it's not something distributions can unilaterally add without causing problems for upstreams.> Speaking of libSDL, you said it had a dependency on libjpeg6? Surely you mean > libSDL_image.so? I'd like to know whether it would be feasible or plausible to > offload all image loading functions to libSDL_image? Probably not all. I just saw it doesn't support saving to any image formats. But maybe it would make sense as an additional, fallback image loader which would enable ioquake3 to support a large number of file formats if so desired with little effort.