reformat_libyuv: use RAWToI444/RAWToJ444 if available #2598

jzern · 2025-02-07T00:28:16Z

These are available since libyuv 1903.

Bug: b:345259429

These are available since libyuv 1903. Bug: b:345259429

wantehchang

LGTM. Thanks!

It would be good to also have Vignesh or Yannis take a look.

wantehchang · 2025-02-07T01:51:01Z

src/reformat_libyuv.c

@@ -80,6 +80,9 @@ unsigned int avifLibYUVVersion(void)
 // These defines are used to create a NULL reference to libyuv functions that
 // did not exist prior to a particular version of libyuv.
 // Versions prior to 1755 are considered too old and not used (see CMakeLists.txt).
+#if LIBYUV_VERSION < 1903


Optional: RAWToJ444 was added in version 1902, but the Neon optimization was added later without bumping the version. And then RAWToI444 was added in version 1903. So it is fine to test 1903 here. I suggest citing the relevant CLs:

https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6207845
https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6220890
https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6223658

See the comment at lines 96-97 as an example.

See the comment at lines 96-97 as an example.

I'll add them, but they feel redundant given the LIBYUV_VERSION check. That and other examples are undocumented.

The gainmap sources use various color primaries.

wantehchang · 2025-02-07T02:08:09Z

tests/gtest/avifgainmaptest.cc

@@ -991,6 +991,8 @@ void ToneMapImageAndCompareToReference(
      &tone_mapped->clli, &diag);
  ASSERT_EQ(result, AVIF_RESULT_OK)
      << avifResultToString(result) << " " << diag.error;
+  // libyuv RAWToJ444 uses BT.601 coefficients.
+  tone_mapped_rgb.avoidLibYUV = AVIF_TRUE;


We should look into this. RAWToJ444 should only be used when matrix_coefficients is BT.601. I remember the code in reformat_libyuv.c checks that properly. Let's double check.

I found the following code in reformat_libyuv.c at lines 276-284:

avifResult avifImageRGBToYUVLibYUV8bpc(avifImage * image, const avifRGBImage * rgb) { assert((image->depth == 8) && (rgb->depth == 8)); // libavif uses byte-order when describing pixel formats, such that the R in RGBA is the lowest address, // similar to PNG. libyuv orders in word-order, so libavif's RGBA would be referred to in libyuv as ABGR. // libyuv only handles BT.601 for RGB to YUV, and not all range/order/subsampling combinations. // BT.470BG has the same coefficients as BT.601. if ((image->matrixCoefficients == AVIF_MATRIX_COEFFICIENTS_BT470BG) || (image->matrixCoefficients == AVIF_MATRIX_COEFFICIENTS_BT601)) {

Is it not doing the right thing? Or is RAWToJ444 not accurate enough?

I found the following code in reformat_libyuv.c at lines 276-284:

avifResult avifImageRGBToYUVLibYUV8bpc(avifImage * image, const avifRGBImage * rgb) { assert((image->depth == 8) && (rgb->depth == 8)); // libavif uses byte-order when describing pixel formats, such that the R in RGBA is the lowest address, // similar to PNG. libyuv orders in word-order, so libavif's RGBA would be referred to in libyuv as ABGR. // libyuv only handles BT.601 for RGB to YUV, and not all range/order/subsampling combinations. // BT.470BG has the same coefficients as BT.601. if ((image->matrixCoefficients == AVIF_MATRIX_COEFFICIENTS_BT470BG) || (image->matrixCoefficients == AVIF_MATRIX_COEFFICIENTS_BT601)) {

Is it not doing the right thing? Or is RAWToJ444 not accurate enough?

With a single test image the output was bit exact. I didn't inspect the path for this test all that closely.

I found the following code in reformat_libyuv.c at lines 276-284:

avifResult avifImageRGBToYUVLibYUV8bpc(avifImage * image, const avifRGBImage * rgb) { assert((image->depth == 8) && (rgb->depth == 8)); // libavif uses byte-order when describing pixel formats, such that the R in RGBA is the lowest address, // similar to PNG. libyuv orders in word-order, so libavif's RGBA would be referred to in libyuv as ABGR. // libyuv only handles BT.601 for RGB to YUV, and not all range/order/subsampling combinations. // BT.470BG has the same coefficients as BT.601. if ((image->matrixCoefficients == AVIF_MATRIX_COEFFICIENTS_BT470BG) || (image->matrixCoefficients == AVIF_MATRIX_COEFFICIENTS_BT601)) {

Is it not doing the right thing? Or is RAWToJ444 not accurate enough?

With a single test image the output was bit exact. I didn't inspect the path for this test all that closely.

I also made an assumption about the files based on their names: colors_hdr_rec2020.avif and colors_wcg_hdr_rec2020.avif.

[ RUN ] All/CreateGainMapTest.Create/8 PSNR (tone mapped vs reference): 99 PSNR (tone mapped vs reference): 54.0596 tests/gtest/avifgainmaptest.cc:1002: Failure Expected: (psnr) >= (min_psnr), actual: 54.0596 vs 55 Google Test trace: tests/gtest/avifgainmaptest.cc:973: hdr_headroom: 0.000000 PSNR (tone mapped vs reference): 54.0596 tests/gtest/avifgainmaptest.cc:1002: Failure Expected: (psnr) >= (min_psnr), actual: 54.0596 vs 55 Google Test trace: tests/gtest/avifgainmaptest.cc:973: hdr_headroom: 0.000000 PSNR (tone mapped vs reference): 99 [ FAILED ] All/CreateGainMapTest.Create/8, where GetParam() = ("colors_sdr_srgb.avif", "colors_hdr_rec2020.avif", 1, 10, 1, 55, 100) (50 ms) ... [ RUN ] All/CreateGainMapTest.Create/10 PSNR (tone mapped vs reference): 58.7199 PSNR (tone mapped vs reference): 53.6957 tests/gtest/avifgainmaptest.cc:1002: Failure Expected: (psnr) >= (min_psnr), actual: 53.6957 vs 55 Google Test trace: tests/gtest/avifgainmaptest.cc:973: hdr_headroom: 0.000000 PSNR (tone mapped vs reference): 53.6957 tests/gtest/avifgainmaptest.cc:1002: Failure Expected: (psnr) >= (min_psnr), actual: 53.6957 vs 55 Google Test trace: tests/gtest/avifgainmaptest.cc:973: hdr_headroom: 0.000000 PSNR (tone mapped vs reference): 58.7199 [ FAILED ] All/CreateGainMapTest.Create/10, where GetParam() = ("colors_sdr_srgb.avif", "colors_wcg_hdr_rec2020.avif", 1, 10, 1, 55, 80) (50 ms)

It was added in 1902, not 1903. Relevant changes: https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6207845 https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6220890 https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6223658

jzern · 2025-02-07T03:15:15Z

With a single test image the output was bit exact. I didn't inspect the path for this test all that closely.

I'll look at this further. I only posted the workaround to make sure there were no other failures and was planning on coming back to this after looking at some other things.

jzern · 2025-02-07T03:25:17Z

Prior to this change:

[ RUN      ] All/CreateGainMapTest.Create/8
PSNR (tone mapped vs reference): 99
PSNR (tone mapped vs reference): 60.8035
PSNR (tone mapped vs reference): 60.8035
PSNR (tone mapped vs reference): 99

With this change:

[ RUN      ] All/CreateGainMapTest.Create/8
PSNR (tone mapped vs reference): 99
PSNR (tone mapped vs reference): 54.0596
PSNR (tone mapped vs reference): 54.0596
PSNR (tone mapped vs reference): 99

At issue are some of the swaps, where the source is BT.601; the other steps are BT.2020 and are likely being skipped as you noted. I'll dump the images to have a look.

jzern · 2025-02-07T21:43:34Z

Prior to this change:
[ RUN      ] All/CreateGainMapTest.Create/8
PSNR (tone mapped vs reference): 99
PSNR (tone mapped vs reference): 60.8035
PSNR (tone mapped vs reference): 60.8035
PSNR (tone mapped vs reference): 99
With this change:
[ RUN      ] All/CreateGainMapTest.Create/8
PSNR (tone mapped vs reference): 99
PSNR (tone mapped vs reference): 54.0596
PSNR (tone mapped vs reference): 54.0596
PSNR (tone mapped vs reference): 99
At issue are some of the swaps, where the source is BT.601; the other steps are BT.2020 and are likely being skipped as you noted. I'll dump the images to have a look.

Looking at the other CreateGainMapTests with -DCONFIG_LIBYUV=0 there are some changes to PSNR, but mostly in the 0.1-0.3dB range. These are high PSNR values and I haven't checked if the non-libyuv case uses float. This may be acceptable, but I'll look a little more closely.

TODO: Windows has lower PSNR than Linux

reformat_libyuv: use RAWToI444/RAWToJ444 if available

ff95eae

These are available since libyuv 1903. Bug: b:345259429

wantehchang approved these changes Feb 7, 2025

View reviewed changes

ToneMapImageAndCompareToReference: disable libyuv

aaae7ae

The gainmap sources use various color primaries.

wantehchang reviewed Feb 7, 2025

View reviewed changes

jzern added 2 commits February 6, 2025 18:53

correct RAWToJ444 version check

d8d4a33

It was added in 1902, not 1903. Relevant changes: https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6207845 https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6220890 https://chromium-review.googlesource.com/c/libyuv/libyuv/+/6223658

add reference links for source of RAWTo[IJ]444

0f4b3bd

jzern added 2 commits February 7, 2025 21:42

avifgainmaptest: adjust psnr thresholds

8053f75

avifgainmaptest: update psnr thresholds x2

ec144c9

TODO: Windows has lower PSNR than Linux

wantehchang approved these changes Feb 8, 2025

View reviewed changes

wantehchang modified the milestone: v1.2.0 Feb 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reformat_libyuv: use RAWToI444/RAWToJ444 if available #2598

reformat_libyuv: use RAWToI444/RAWToJ444 if available #2598

jzern commented Feb 7, 2025

wantehchang left a comment

wantehchang Feb 7, 2025

jzern Feb 7, 2025

wantehchang Feb 7, 2025

wantehchang Feb 7, 2025

jzern Feb 7, 2025

jzern Feb 7, 2025

jzern Feb 7, 2025

jzern commented Feb 7, 2025

jzern commented Feb 7, 2025

jzern commented Feb 7, 2025

reformat_libyuv: use RAWToI444/RAWToJ444 if available #2598

Are you sure you want to change the base?

reformat_libyuv: use RAWToI444/RAWToJ444 if available #2598

Conversation

jzern commented Feb 7, 2025

wantehchang left a comment

Choose a reason for hiding this comment

wantehchang Feb 7, 2025

Choose a reason for hiding this comment

jzern Feb 7, 2025

Choose a reason for hiding this comment

wantehchang Feb 7, 2025

Choose a reason for hiding this comment

wantehchang Feb 7, 2025

Choose a reason for hiding this comment

jzern Feb 7, 2025

Choose a reason for hiding this comment

jzern Feb 7, 2025

Choose a reason for hiding this comment

jzern Feb 7, 2025

Choose a reason for hiding this comment

jzern commented Feb 7, 2025

jzern commented Feb 7, 2025

jzern commented Feb 7, 2025