Skip to content

Commit

Permalink
detect AOM 3.6.0 bug (#790)
Browse files Browse the repository at this point in the history
  • Loading branch information
@farindk
farindk committed Jul 14, 2023
1 parent 4a2f42f commit 05675b3
Showing 1 changed file with 17 additions and 0 deletions.
17 changes: 17 additions & 0 deletions libheif/plugins/encoder_aom.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,23 @@ struct heif_error aom_encode_image(void* encoder_raw, const struct heif_image* i

int bpp_y = heif_image_get_bits_per_pixel_range(image, heif_channel_Y);


// --- check for AOM 3.6.0 bug

bool is_aom_3_6_0 = (aom_codec_version() == 0x030600);

if (is_aom_3_6_0) {
// This bound might be too tight, as I still could encode images with 8193 x 4353 correctly. Even 8200x4400, but 8200x4800 fails.

This comment has been minimized.

Sign in to view

Copy link
@wantehchang

wantehchang Jul 14, 2023

Contributor

This condition is a simplified version of a necessary condition for the bug. It has false positives from two sources.

  1. A necessary condition has false positives, i.e., it returns true for some image sizes that are not affected by this bug.

  2. Since this condition omits a check required by the necessary condition, it returns true for some very large image sizes that are not affected by this bug.

This comment has been minimized.

Sign in to view

Copy link
@farindk

farindk Jul 14, 2023

Author Contributor

There is a 'not' in front of it. Thus, the condition here is stricter than necessary.
Unless I'm wrong. If I'm wrong, please give me a counter example where my condition is too loose.

This comment has been minimized.

Sign in to view

Copy link
@wantehchang

wantehchang Jul 14, 2023

Contributor

A counter example is source_width = 32768 and source_height = 17410.

libaom 3.6.0 will use the maximum parameters level (31) for 32768 x 17410, so this image size is not affected by the bug. But your condition returns true for this image size.

This comment has been minimized.

Sign in to view

Copy link
@farindk

farindk Jul 14, 2023

Author Contributor

Yes, put the point is that in your example, my condition will say "error" even though the bug does not trigger. Hence, my condition is tighter. It excludes more cases than would be encoded fine, but it does not let through cases that would give corrupted output.

This comment has been minimized.

Sign in to view

Copy link
@wantehchang

wantehchang Jul 17, 2023

Contributor

Dirk: That's exactly what I meant by "false positives", i.e., your condition will return true (which means "error") for some image sizes that are not affected by this bug. I should have also noted that your condition has no false negatives.

So we are in agreement.
// Let's still keep it as most images will be smaller anyway.
if (!(source_width <= 8192 * 2 && source_height <= 4352 * 2 && source_width * source_height <= 8192 * 4352)) {

This comment has been minimized.

Sign in to view

Copy link
@wantehchang

wantehchang Jul 14, 2023

Contributor

In AV1, source_width and source_height can be as large as 2^16, so the product source_width * source_height may overflow an int. We need to cast at least one of the operands to int64_t so that the multiplication is done in 64 bits.

This comment has been minimized.

Sign in to view

Copy link
@farindk

farindk Jul 14, 2023 •
edited
Loading

Author Contributor

Right, but in this particular case, the condition also includes the first two checks (width <= ... / height <= ...).
Hence, when there would be an overflow, the condition is false anyway.
err = {heif_error_Encoding_error,
heif_suberror_Encoder_encoding,
"AOM v3.6.0 has a bug when encoding large images. Please upgrade to at least AOM v3.6.1."};
return err;
}
}


// --- copy libheif image to aom image

aom_image_t input_image;
Expand Down

0 comments on commit 05675b3

Please sign in to comment.