Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<16 pixels encoding issue with aom #365

Closed
kleisauke opened this issue Oct 23, 2020 · 12 comments · Fixed by #402
Closed

<16 pixels encoding issue with aom #365

kleisauke opened this issue Oct 23, 2020 · 12 comments · Fixed by #402

Comments

@kleisauke
Copy link
Contributor

Since commit b22820a AVIF images smaller than <16 pixels are encoded to 16×16 with cropping. This works great with rav1e, but for some reason it cannot be opened in Chrome when encoding with aom. libvips also seems to fail in processing this image (issue libvips/libvips#1808 might be relevant here).

Test images:
https://t0.nl/x-aom.avif - 10×10 encoded with aom v2.0.0
https://t0.nl/x-rav1e.avif - 10×10 encoded with rav1e v0.3.4

For reference, the images encoded as 16×16:
https://t0.nl/x2-aom.avif
https://t0.nl/x2-rav1e.avif

The test images are generated with:

# aom v2.0.0
vips crop zebra.jpg x-aom.avif[compression=av1] 0 0 10 10
vips crop zebra.jpg x2-aom.avif[compression=av1] 0 0 16 16

# rav1e v0.3.4
vips crop zebra.jpg x-rav1e.avif[compression=av1] 0 0 10 10
vips crop zebra.jpg x2-rav1e.avif[compression=av1] 0 0 16 16

Building a debug build of Chromium reveals this verbose message:

[29580:29588:1023/105621.829690:VERBOSE1:avif_image_decoder.cc(534)] Frame size "10x10" differs from container size "16x16"

https://github.com/chromium/chromium/blob/d3f2d5be6762b18b922d109a19250812ea88f9e0/third_party/blink/renderer/platform/image-decoders/avif/avif_image_decoder.cc#L534

@0xC0000054
Copy link
Contributor

0xC0000054 commented Oct 23, 2020

The issue appears to be that the AOM encoder does not extend the image size to 16x16 before encoding it.
Chrome requires that all images match the size that is declared in the AVIF container.

@kleisauke
Copy link
Contributor Author

Curiously, the actually encoded image of x-aom.avif is 7×8 in size, see:

$ heif-info x-aom.avif
MIME type: image/avif
image: 10x10 (id=1), primary
  color profile: nclx
  alpha channel: no
  depth channel: no
$ heif-convert x-aom.avif x.jpg
File contains 1 images
Written to x.jpg
$ vipsheader x.jpg
x.jpg: 7x8 uchar, 3 bands, srgb, jpegload

(Note that I'm not sure why Chrome reports that x-aom.avif is encoded as 16×16)

With x-rav1e.avif I see this:

$ heif-info x-rav1e.avif
MIME type: image/avif
image: 10x10 (id=1), primary
  color profile: nclx
  alpha channel: no
  depth channel: no
$ heif-convert x-rav1e.avif x.jpg
File contains 1 images
Written to x.jpg
$ vipsheader x.jpg
x.jpg: 10x10 uchar, 3 bands, srgb, jpegload

@kleisauke
Copy link
Contributor Author

It seems that the dimensions returned here:

const int source_width = heif_image_get_width(image, heif_channel_Y);
const int source_height = heif_image_get_height(image, heif_channel_Y);

Were not the updated extend_to_size image dimensions, this patch seems to fix this:

diff --git a/libheif/heif_image.cc b/libheif/heif_image.cc
index 1111111..2222222 100644
--- a/libheif/heif_image.cc
+++ b/libheif/heif_image.cc
@@ -277,6 +277,9 @@ bool HeifPixelImage::extend_to_size(int width, int height)
              &plane->mem[(plane->m_height - 1) * plane->stride],
              subsampled_width * nbytes);
     }
+
+    plane->m_width = subsampled_width;
+    plane->m_height = subsampled_height;
   }
 
   m_width = width;

The reason that the rav1e encoder was not affected by this bug is probably because it did not retrieve the dimensions of the heif_channel_Y channel, see:

if (rav1e_config_parse_int(rav1eConfig.get(), "width", image->image->get_width()) == -1) {
return heif_error_codec_library_error;
}
if (rav1e_config_parse_int(rav1eConfig.get(), "height", image->image->get_height()) == -1) {
return heif_error_codec_library_error;
}

@kleisauke
Copy link
Contributor Author

kleisauke commented Sep 5, 2022

I could no longer reproduce this with libheif v1.13.0 and libaom v3.2.0. Tested with:

# Make 10x10 test image
$ vips black x.jpg 10 10
$ vipsheader x.jpg
x.jpg: 10x10 uchar, 1 band, b-w, jpegload

# Convert to AVIF (with aom) and back to JPEG
$ heif-enc -A -o x.avif x.jpg
$ heif-convert x.avif x2.jpg

# Output should be 10x10
$ vipsheader x2.jpg
x2.jpg: 10x10 uchar, 3 bands, srgb, jpegload

I'll close.

Edit: this was fixed with commit ec1dc46.

@kleisauke
Copy link
Contributor Author

Note that after commit xiph/rav1e@625a2bb in rav1e (version 0.4.0), it's probably also no longer necessary to pad the image size to 16x16 pixels.

*encoded_width = std::max(input_width, 16U);
*encoded_height = std::max(input_height, 16U);

// --- round image size to minimum size
uint32_t rounded_width, rounded_height;
rav1e_query_encoded_size(encoder,
image->image->get_width(),
image->image->get_height(),
&rounded_width,
&rounded_height);
bool success = image->image->extend_padding_to_size(rounded_width, rounded_height);

--- a/libheif/heif_encoder_rav1e.cc
+++ b/libheif/heif_encoder_rav1e.cc
@@ -481,8 +481,8 @@ void rav1e_query_input_colorspace2(void* encoder_raw, heif_colorspace* colorspac
 void rav1e_query_encoded_size(void* encoder, uint32_t input_width, uint32_t input_height,
                               uint32_t* encoded_width, uint32_t* encoded_height)
 {
-  *encoded_width = std::max(input_width, 16U);
-  *encoded_height = std::max(input_height, 16U);
+  *encoded_width = input_width;
+  *encoded_height = input_height;
 }
 
 
@@ -491,16 +491,8 @@ struct heif_error rav1e_encode_image(void* encoder_raw, const struct heif_image*
 {
   auto* encoder = (struct encoder_struct_rav1e*) encoder_raw;
 
-  // --- round image size to minimum size
-
-  uint32_t rounded_width, rounded_height;
-  rav1e_query_encoded_size(encoder,
-                           image->image->get_width(),
-                           image->image->get_height(),
-                           &rounded_width,
-                           &rounded_height);
-
-  bool success = image->image->extend_padding_to_size(rounded_width, rounded_height);
+  bool success = image->image->extend_padding_to_size(image->image->get_width(),
+                                                      image->image->get_height());
   if (!success) {
     struct heif_error err = {
         heif_error_Memory_allocation_error,

@silverbacknet
Copy link

Ultimately, whether the padding is done in libheif or the encoder itself, it's going to be done somewhere, so that doesn't save much. I guess it simplifies the code a bit, though.

@farindk
Copy link
Contributor

farindk commented Sep 5, 2022

@kleisauke Good. That will simplify the code. I'll test it and then do the change.
@silverbacknet Right, but if this is done in rav1e, cropping is implicit. If libheif does the cropping, it has to add a clap box, which adds a few bytes.

@farindk
Copy link
Contributor

farindk commented Sep 5, 2022

I've changed it. Actually, encoding small images with rav1e was even broken in v1.13.0 because the encoded size did not match the size assumed in the headers.

@kleisauke
Copy link
Contributor Author

Great! Out of curiosity, do you think this block is still needed within the aom encoder?:

bool success = image->image->extend_padding_to_size(image->image->get_width(),
image->image->get_height());
if (!success) {
err = {heif_error_Memory_allocation_error,
heif_suberror_Unspecified,
kError_out_of_memory};
return err;
}

(I noticed that this was deleted in commit d506bcc, so perhaps something similar can be done to the aom encoder)

@farindk
Copy link
Contributor

farindk commented Sep 5, 2022

I have also been wondering about that. Probably it's not necessary, but I'd like to go through the history to make sure I don't miss anything here.

@farindk
Copy link
Contributor

farindk commented Sep 5, 2022

Yes, extend_padding_to_size also seems unnecessary in the AOM encoder. I've removed it.

@farindk
Copy link
Contributor

farindk commented Sep 5, 2022

BTW: avoiding the crop in libheif makes the output file about 50 bytes smaller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants