Allow read from JPEG without colorspace conversion

[tongyuantongyu]

Implementation for #372. Require a explicit flag(-p, --preserve) to use this path. This patch also adds support to read and write 12bit JPEG files (example here).

[EwoutH]

This looks quite amazing!

@joedrago or @wantehchang, does one of you have a chance to review?

[joedrago]

This seems to be 3 semi-unrelated things all mixed together in a big PR. The general idea of preserving YUV (avoiding YUV -> RGB -> YUV) from JPEG seems quite separable from the larger changes (supporting arbitrary compiled-in-one-at-a-time JPEG depths, additional enums, all of the preserve checking in avifdec, etc.)

Which part looks amazing to you? It might be easier to separate it out and make a PR of just that part.

[EwoutH]

I didn't dive too deep in the implementation, just the feature of avoiding colorspace conversion would be very useful, especially for screen content.

I agree with splitting in multiple PRs.

[tongyuantongyu]

Now have a look on this again I believe support libjpeg with depth other than 8 is not very useful. I'll remove this part. Maybe I'll make a seperate PR to check libjpeg is configured as 8 bit.

I find it's unclear whether we only want to prevent colorspace conversion if the format user asked matches what's in jpeg, or we want to adjust avif's format so that we can prevent colorspace conversion.

If we only prevent conversion on match, the problem is a user can hardly tell the subsampling mode of a jpeg file. Using imagemagick, you need to do this:

$ identify -verbose 1.jpg | grep jpeg:sampling-factor
    jpeg:sampling-factor: 1x1,1x1,1x1

And then we have

sampling-factor	yuv format
1x1,1x1,1x1	YUV444
2x1,1x1,1x1	YUV422
2x2,1x1,1x1	YUV420

The time need on figuring this out might not be shorter than just do the conversion anyway.

@EwoutH @wantehchang, which behavior do you prefer?

[tongyuantongyu]

YUV->RGB->YUV should be lossless, if all YUV value are valid in RGB range, so it's mostly just about performance - but your effort on checking this info and changing flags also count.

[tongyuantongyu]

I updated this PR, and now it only contains 'preserving YUV when reading JPEG if possible' part.

I suggest we add an 'automatic' value for requestedFormat (or let AVIF_PIXEL_FORMAT_NONE be that), like 0 for requestedDepth, which allows selecting best format according to input file.

[tongyuantongyu]

@joedrago, now this PR is only about preserving YUV when reading from JPEG, could you have a review if you think this is a good feature, or otherwise inform me so that I'll close this PR?

[joedrago]

I haven't considered this a high priority feature as I don't believe there is any demand for it. Is there any reason you don't want to just let the PR stay open? It is okay with me if you want to close it and we can make a fresh PR in the future if need be.

[joedrago]

Franky speacking, without it libavif is too dangerous to use.

This seems a bit hyperbolic.

The feature @tongyuantongyu is providing in this PR is for avifenc, not libavif. libavif is designed to accept unaltered YUV already, so I'm not sure what you're intending to say here.

I'm quite familiar with the precision loss when converting between RGB and YUV, but in this specific case (reading in a JPEG for packing), you're going to take the image given from JPEG and compress it down even further, creating more loss. I suppose you could create a QP0 AVIF which could keep the YUV in the JPEG perfectly intact, but not only would the resultant AVIF be larger than the JPEG (making compressing it into AVIF moot), but eventually someone is going to convert those precise YUV values back into RGB using the same mechanism before displaying it in any typical viewer (such as a browser).

So in this specific case (avifenc, not libavif), this actually buys you very little. At this point I've spent more time replying to this PR than it probably would have taken to review it, so I'll review it now.

[joedrago]

@tongyuantongyu I think the way that avifJPEGReadCopy() ensures that the requested destination format matches exactly is very likely to cause this code to not be used often, without the end user being aware at all. I think avifenc.c and avifjpeg.c might need to signal a bit more information to each other on the enduser's expectations, or perhaps we just need to add some careful printfs in avifjpeg.c indicating to the user that they are or are not receiving this benefit. As an example, I took a JPEG that is JCS_YCbCr internally, and simply ran avifenc abstract.jpg out.avif on it, and it silently avoided this path because I didn't also specify --yuv 420. We need to think about how the enduser can at least be made aware of whether this path occurred, or perhaps ensure it, if they require it for some reason. This will only get worse as the commandline gets more "interesting". For example, someone simply doing avifenc --lossless input.jpg output.avif will never get this path (ironically).

I have a few comments I can add inline with the diff and we can discuss each there, but generally I think this current design is almost never going to end up invoking avifJPEGCopyPixels() in a way that the enduser can rely on or even know about.

[joedrago]

I think checking against MC=12 here might not be worth it, and I don't believe AVIF_COLOR_PRIMARIES_BT470M is correct either way? The derived matrix coefficients for BT601 either come from AVIF_COLOR_PRIMARIES_BT470BG or AVIF_COLOR_PRIMARIES_BT601 (I'd have to do the math to remember precisely), but I'm almost positive it isn't AVIF_COLOR_PRIMARIES_BT470M.

[joedrago]

Alright, I did the math, and you're right:

calcYUVCoefficients = (primaries) ->
  rX = primaries[0]
  rY = primaries[1]
  gX = primaries[2]
  gY = primaries[3]
  bX = primaries[4]
  bY = primaries[5]
  wX = primaries[6]
  wY = primaries[7]
  rZ = 1.0 - (rX + rY) # (Eq. 34)
  gZ = 1.0 - (gX + gY) # (Eq. 35)
  bZ = 1.0 - (bX + bY) # (Eq. 36)
  wZ = 1.0 - (wX + wY) # (Eq. 37)
  kr = (rY * (wX * (gY * bZ - bY * gZ) + wY * (bX * gZ - gX * bZ) + wZ * (gX * bY - bX * gY))) /
       (wY * (rX * (gY * bZ - bY * gZ) + gX * (bY * rZ - rY * bZ) + bX * (rY * gZ - gY * rZ)))

  # (Eq. 32)
  kb = (bY * (wX * (rY * gZ - gY * rZ) + wY * (gX * rZ - rX * gZ) + wZ * (rX * gY - gX * rY))) /
       (wY * (rX * (gY * bZ - bY * gZ) + gX * (bY * rZ - rY * bZ) + bX * (rY * gZ - gY * rZ)))

  # (Eq. 33)
  coeffs = []
  coeffs[0] = kr
  coeffs[2] = kb
  coeffs[1] = 1.0 - coeffs[0] - coeffs[2]

  return coeffs

main = ->
  primaries =
    "BT470M":  [ 0.67, 0.33, 0.21, 0.71, 0.14, 0.08, 0.310, 0.316 ]
    "BT470BG": [ 0.64, 0.33, 0.29, 0.60, 0.15, 0.06, 0.3127, 0.3290 ]
    "BT601":   [ 0.630, 0.340, 0.310, 0.595, 0.155, 0.070, 0.3127, 0.3290 ]

  console.log "Looking for 0.299, 0.114:\n"

  for name, p of primaries
    c = calcYUVCoefficients(p)
    console.log "#{name}: Kr: #{c[0].toFixed(4)} Kb: #{c[2].toFixed(4)}"

main()

... outputs:

Looking for 0.299, 0.114:

BT470M: Kr: 0.2990 Kb: 0.1146
BT470BG: Kr: 0.2220 Kb: 0.0713
BT601: Kr: 0.2124 Kb: 0.0866

So you're right that AVIF_COLOR_PRIMARIES_BT470M happens to match. That said, I don't think CICP 4/x/12 is an interesting combination to check. Perhaps if we come up with a solution for the "enduser expectations" issue cited above, this might end up being fine.

[joedrago]

I've supplied the math I did for my check afterwards. I think we should avoid offering this functionality when MC=12, to be safe.

[joedrago]

(I'm not sure if this was a question for me or not, but I will supply some info and hope I answer something for you.)

For any AVIF_MATRIX_COEFFICIENTS_* enum that ends up being a typical matrix multiply, we have a table of coefficients that simply cite the values in Table 4 of H.273:

https://github.com/AOMediaCodec/libavif/blob/master/src/colr.c#L81

libavif won't do any of that math on MC=1, for example, it'll just use whatever is in that table. That said, you can certainly start with any set of color primaries and (using the math above or in libavif's calcYUVInfoFromCICP()), you can derive the coefficients. They correspond exactly for CP/MC=1 (BT709) and CP/MC=9 (BT2020), for example.

[tongyuantongyu]

I've supplied the math I did for my check afterwards. I think we should avoid offering this functionality when MC=12, to be safe.

My consideration is, if YUV->RGB->YUV gives the same result, we just skip it and use YUV directly.

[joedrago]

No, they aren't.

From H.273:

I appreciate your interest in this particular PR and curiosities surrounding color math, but this is way out of scope, and incorrect. Please open a fresh GitHub issue if you discover incorrect constants in libavif.

[joedrago]

As an aside, I have general concerns that people might expect this to be in some what similar/equivalent to JPEG-XL's ability to repackage a JPEG to JPEG-XL without any data loss, which will never be the case. As I alluded to earlier, the best case we can offer in terms of preserving these values would be a --min 0 --max 0, subsampling-matched, CICP-matched AVIF which would certainly be larger than the original JPEG.

I don't think this is the case with anyone commenting currently on this thread, but I wanted to make it clear for anyone that reads this in the future. Assuming we can figure out the enduser ergonomics around how to request/demand this path and be signaled whether or not it used this path, this will be a minor precision bump (at best, most JPEGs come from RGB anyway) on decode just before AVIF compresses the data down even further. That resultant AVIF will then be converted back to RGB in the future before being displayed.

[joedrago]

I updated this PR, and now it only contains 'preserving YUV when reading JPEG if possible' part.

I suggest we add an 'automatic' value for requestedFormat (or let AVIF_PIXEL_FORMAT_NONE be that), like 0 for requestedDepth, which allows selecting best format according to input file.

Using AVIF_PIXEL_FORMAT_NONE for this might be fine. We'd have to audit all of the decoders to ensure that they behave correctly when the requestedFormat is AVIF_PIXEL_FORMAT_NONE. It might be smarter to be explicit about whether or not the format (or any other value) was explicitly set, though.

[joedrago]

@tongyuantongyu I've looked over the decoders and avifdec.c, and I think we can get away with defaulting input.requestedFormat to AVIF_PIXEL_FORMAT_NONE, and then adding these two lines to avifutil.h:

// Used by image decoders when the user doesn't explicitly choose a format with --yuv
#define AVIF_APP_DEFAULT_PIXEL_FORMAT AVIF_PIXEL_FORMAT_YUV444

The Y4M decoder won't have to change, as it doesn't leverage requestedFormat already. In both the JPEG and PNG decoders though, they'll have to detect when requestedFormat == AVIF_PIXEL_FORMAT_NONE and will have to (at least) fall back to AVIF_APP_DEFAULT_PIXEL_FORMAT. This would allow for the "automatic" feature you've suggested.

[joedrago]

If having an automatic format makes the most common paths use this new feature, then the only main issue left would be signaling when the JPEG reader uses this. Perhaps we can start simple and just printf() when it does use it, as the case up until now has been to print nothing. We can make this more explicit in the future if we want, but as long as the enduser knows that they will at least see a line printed when this happens, we can add more restrictions to this later.

[tongyuantongyu]

Command line tools need some changes or this feature, probably a --yuv auto and make it default is good. @joedrago, do you want these changes in this PR or have another PR for it?

[joedrago]

Alright, I've done a full review, and have made minor adjustments to the code, starting with the previous push from @tongyuantongyu (the repeated goto success was heading the opposite direction than I was intending with my comment). I then implemented --yuv auto, and took a shot at interpreting it correctly in avifjpeg.c. All commits are here:

https://github.com/AOMediaCodec/libavif/compare/raw_input_from_jpeg

It should be 2 commits from @tongyuantongyu (b6eb607 and ce701aa), followed by two commits from me (98c9b2f and d686a55).

98c9b2f contains all the changes to the PR. It includes some clarifying comments, fixing the detection of "compatible YUV coefficients", and a minor flow change where I eliminated one goto and distributed out one call to jpeg_finish_decompress().

d686a55 is my implementation of --yuv auto, using AVIF_PIXEL_FORMAT_NONE as "auto" during image reading, as we discussed.

@tongyuantongyu and @wantehchang - Please review these two new commits (and for @wantehchang, please review all 4 commits if you haven't read @tongyuantongyu's commits yet), and let me know if they are acceptable. If so, these 4 commits are what I'll end up merging into master.

Edit: Updated SHAs as I rebased to origin/master.

[tongyuantongyu]

In avifenc.c#746 we may instead set input.requestedFormat to AVIF_PIXEL_FORMAT_YUV444 if it's AVIF_PIXEL_FORMAT_NONE.

And we may also set yuvFormat to AVIF_PIXEL_FORMAT_YUV400 if we got a grayscale PNG input.

[joedrago]

This is now in. We can add minor changes/features to it in separate PRs.