jbrd: allow empty DHT marker

[jonsneyers]

See #2542

In JPEG, the DHT (define huffman table) marker segment can define "one or more" huffman tables.

However, it looks like at least one JPEG file found in the wild (see #2542) contains a DHT segment that is empty, i.e. doesn't contain any huffman table. It is debatable if such JPEG files are valid or not (I think strictly speaking they're not since the spec says "one or more"); but it does look like all common implementations decode them just fine and just ignore the empty DHT segment.

Currently we return an error when we encounter an empty DHT segment.
This pull request changes that and makes it work. However, this requires a bit of trickery, since in the jbrd, we cannot directly represent this situation — the assumption there is that there is at least one table, and an is_last field is used to signal how many there are. We can however use the all-zeroes huffman code (an invalid code) to represent this specific case, and encode an empty DHT marker as if it is a marker containing the all-zeroes huffman code.

In terms of cjxl/djxl behavior, we then get the following on the test case from #2542.

Old cjxl: fails to recompress, with error "DHT marker: no Huffman table found".
New cjxl: recompresses it.

Old djxl: fails to reconstruct the output of new cjxl, with error "Empty Huffman table".
New djxl: reconstructs it correctly.

I don't know how important this edge case is, i.e. how common such JPEG files with empty DHT segments are in the wild — obviously they're rare, but is it 0.1% or 0.0000001%?. What software or hardware produces this?

If we change this jbrd, it would be good to also update the jpeg bitstream reconstruction spec (Annex A of 18181-2) to clarify how to handle this particular case — it's not strictly needed since we can also treat it as an out-of-spec behavior (i.e. both the original jpeg and the jbrd are technically non-conforming with the spec), but it wouldn't hurt to spec it this way.

[veluca93]

I'm in principle against this change, unless there's a known good reason to deviate from the (JPEG/JXL) spec.

…

On Thu, 3 Aug 2023, 09:19 Jon Sneyers, ***@***.***> wrote: @jonsneyers <https://github.com/jonsneyers> requested your review on: #2704 <#2704> jbrd: allow empty DHT marker. — Reply to this email directly, view it on GitHub <#2704 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOPAI5Z5VKSJR2NJ2NAK5TXTNGJVANCNFSM6AAAAAA3CKLHNM> . You are receiving this because your review was requested.Message ID: ***@***.***>

[jonsneyers]

I'm in principle against this change, unless there's a known good reason to deviate from the (JPEG/JXL) spec.

This is imo not a deviation from either of the specs, in the sense that behavior that is certainly in-spec remains exactly the same.

It is about how to deal with the edge case of n=0, which is not useful (there's no good reason a JPEG encoder would do that except maybe if it needs to do no-op padding for some reason) but it can and does exist in JPEG files found in the wild. It's not entirely clear if n=0 is allowed by the JPEG spec (the introductory sentence says "one or more", but the value n in Table B.5 is not really specifically constrained to be > 0). De facto, empty DHT markers are considered in-spec (e.g. libjpeg's djpeg -strict decodes it just fine without any warnings).

This change does not break anything that currently works. It does make something work that didn't work before though, so it's similar to what we did in #359.

The main downside of this change is that it allows you to create jxl-recompressed jpeg files that an older djxl will only be able to decode to pixels, not reconstruct to jpeg; i.e. you'll have to update your djxl to get a successful reconstruction.

[jonsneyers]

To be clear: I'm OK with resolving this either way (keeping current behavior or changing it to make something work that currently doesn't).

This pull request is mainly just to show that making it work can be done in a more or less reasonable way (unlike some of the other limitations we have like CMYK JPEGs) without really breaking anything.

Whether or not it is important to make this work depends imo on how common such JPEGs actually are in the wild.

[veluca93]

Let me clarify my position: I am against this change, unless we know that the fraction of such JPEGs actually in the wild is high enough.

[jonsneyers]

Let me clarify my position: I am against this change, unless we know that the fraction of such JPEGs actually in the wild is high enough.

That's fair enough.

I don't know how to figure out what that fraction is — I suspect that on the web it will be very rare, since I don't think any of the commonly used modern jpeg encoders (i.e. libjpeg-turbo, mozjpeg) are producing such bitstreams, and the bulk of jpegs found on the web will have been encoded by one of them. Also I don't really know how we should define "high enough" — even if the fraction is extremely small, it could still be the case that the entire photo collection of some people has this problem (e.g. if it's a specific somewhat obscure camera or legacy software that produced such files) and for them this change could make the difference between being able to recompress nothing and everything.
For this reason, I'm somewhat tempted to say that "non-zero" is a "high enough" fraction.

Besides the example given in #2542, I also found someone on stackoverflow who has encountered empty DHT segments 5 years ago: https://stackoverflow.com/questions/50579606/jpeg-containing-multiple-dht-marker. That's about all I can find about this.

I remember you once scraped lots of jpegs from the web to test coverage for jpeg recompression, do you still have those or a way to do it again? We're looking here for jpegs that contain the specific sequence 0xFFC40002.

In my opinion, besides the pragmatic question of in-the-wild-occurrence, there's also the principle question of whether it's a valid JPEG bitstream that we currently cannot recompress (so it's kind of a bugfix to make it work) or if it's an invalid one and we're correct to just dismiss it as being corrupt input (and we would maybe even kind of make it 'too lax' if we make it work).

On that question, I think the answer is that it should be considered a valid JPEG bitstream: both official JPEG reference implementations (libjpeg-turbo and ISO libjpeg) decode it without any warning, and so do all other JPEG decoders as far as I know — so we're being the exception if we consider it invalid. But this can be debated since the JPEG standard does say in the introductory sentence that DHT "defines one or more Huffman table specifications" and then later on says "The value n in Table B.5 is the number of Huffman tables specified in the DHT marker segment." but doesn't clearly impose that n > 0 other than by implication from the introductory sentence. I'd say the JPEG standard is not fully clear but tends towards considering it invalid.

In the past, we have taken the de facto behavior of the common implementations as they are deployed widely as more important than the original standard — this is also why we are more or less following the JPEG XT part 1 version of the JPEG spec, i.e. no arithmetic coding, 12-bit,hierarchical, lossless etc but only the commonly implemented parts of the JPEG spec (8-bit, huffman coding, sequential or progressive). According to that design principle, we should imo allow empty DHT markers.

[Logicdemon]

Hi, I have been experimenting with jxl and ran into a problem that led me here after searching around the net.
I am able to convert many jpegs to jxl without issue, however very quickly I ran into problems when trying to convert jpegs that have been optimized. Trying to convert these leads to the error: JxlEncoderAddJPEGFrame() failed

I am running them through a application called FileOptimizer. Mainly it just strips unnecessary meta data, however importantly they are further optimized using "Arithmetic encoding". What I have found is, it is the arithmetic encoded images that are failing every time. I also created an image from stratch in Gimp and saved as an arithmetic encoded jpeg and did not run it through any optimizers, it also failed to be converted to jxl with the same error message.

I have around 20,000 jpgs on my system, they are all arithmetic encoded. I have no issues viewing them or editing them in programs and have been using arithmetic encoded jpegs for years on linux.

Interestingly Imagemagick has no problem converting any of the arithmetic files I have jpeg xl using the mogrify command.

I hope this helps shed a bit more light on things. I am looking forward to to using JXL more in the future with an eye on eventually converting all my images over to the format.

Cheers