Avif and webp recognition is bugged

installgentoo commented 2 years ago

image v0.23.14

Expected

Avif/webp should work

Actual behaviour

Format not recognised on images of odd size

Reproduction steps

Convert image to avif with imagemagick, use libaom backbend for libheif. You get an apparently valid file which begins with 0x00 0x00 0x00 0x1C 0x66 0x74 0x79 0x70 0x61 0x76 0x69 0x66 Magic bytes for avif are (b"\0\0\0 ftypavif", ImageFormat::Avif), and space is 0x20 You can already see the issue. We need another variant of avif with fileseparator in palce of space.

installgentoo commented 2 years ago

also even though dav1d works properly, image fails on decoder_to_image()

the image is returned as image::color::ColorType::Bgra8

an example file https://github.com/installgentoo/assorted_curiosities/blob/master/trash/test.avif properly displayed by firefox, imagemagick and geeqie.

installgentoo commented 2 years ago

// Cross-correlate pixel format with planes and alignment.
// wrapping_sub is wanted. If num_planes is 0, this turns in a very big number that
// still represents an invalid number of planes.
let last_src_plane = src_format.num_planes.wrapping_sub(1);
if !pixel_format::is_compatible(src_pixel_format, width, height, last_src_plane) {
    return Err(ErrorKind::InvalidValue);
}

specifically fails on this checking Bt601 -> Lrgb and I420 -> Bgra

installgentoo commented 2 years ago

Okay i've debugged a bit more, and removing that check at dcv-color-primitives-0.1.16/src/lib.rs:827 makes my avifs load correctly.

Could some actual developer help me figure out where the last_src_plane(i suspect?) is set incorrectly? This is something in image crate methinks.

installgentoo commented 2 years ago

On top of that dcv issue dav1d 1.0.0 seems to just break avif support. Apparently you have to flush something somewhere now.

HeroicKatora commented 2 years ago

On top of that dcv issue dav1d 1.0.0 seems to just break avif support

Do you happen to have a reproduction? I suspect we should do a call to primary_decoder.flush()?; here: https://github.com/image-rs/image/blob/master/src/codecs/avif/decoder.rs#L40-L41

installgentoo commented 2 years ago

I'm actively trying to debug.

You can grab the test.avif image from my second post and try opening it for yourself.

ooops, wrong button

HeroicKatora commented 2 years ago

https://github.com/strukturag/libheif/blob/0f8496f22d284e1a69df12fe0b72f375aed31315/libheif/heif_decoder_dav1d.cc#L178-L183

Huh. Thanks, good pointer.

installgentoo commented 2 years ago

#if EPERM > 0
#define DAV1D_ERR(e) (-(e)) ///< Negate POSIX error code.
#else
#define DAV1D_ERR(e) (e)
#endif

This is in dav1d. So apparently -11 is EAGAIN

installgentoo commented 2 years ago

Alright, first of all - yes, yes it is. -11 is EAGAIN. I just did

            let mut ret = -11;

            while ret == -11 {
                ret = dav1d_get_picture(self.dec, &mut pic);
            }

in dav1d crate, and that just works. I dunno what's actually supposed to be happening there, whether you have to resend like libheif, or how to define dav1d's EAGAIN properly.

Secondly, yes, dcv issue is still there.

fabiosky commented 2 years ago

I see that the image size passed to dcv color primitives is 288x285. The height is not a multiple of two, and that case is not supported for the I420 source pixel format. See https://docs.rs/dcv-color-primitives/latest/dcv_color_primitives/fn.convert_image.html,

InvalidValue if source or destination image formats have a number of planes which is not compatible with their pixel formats

Thus, it is the root cause generating the InvalidValue. Obviously disabling the check will cause the color conversion to happen, but causes undefined behaviour (maybe visual seams) in the lower horizontal image border.

If you pass, for example, a 288x284 image, the conversion will succeed. About planes and strides, everything is passed correctly in the read_image.

installgentoo commented 2 years ago

we should add an empty line, then?(and remove it from decoded image)

fabiosky commented 2 years ago

I opened https://github.com/aws/dcv-color-primitives/issues/65 to track dcv color primitives action items. Would take a while, in the meantime, the procedure to handle the case is the following:

Assuming your luma plane (y) has original dimensions width, height:

If width not a multiple of two, need to duplicate the rightmost column to get W = w + 1 which becomes a multiple of two
The same for height applies, duplicate the uppermost row to get H = h + 1 which becomes a multiple of two.

For the chroma planes (u, v) [note: their size could be different than luma one!]

Check if the plane width is exactly W / 2. If not, duplicate the rightmost plane column.
Do the same for plane height, should be H / 2, otherwise duplicate the uppermost plane row.

Color conversion has to happen passing W, H (not the original dimensions.

You will get a W, H rgb image. Obvously you may want to crop the rightmost column and uppermost row depending on the match on original width, height

installgentoo commented 2 years ago

@fabiosky how do you actually duplicate columns? the file content is magic.

btw this also affects webp

FlareFlo commented 1 year ago

Still occurs with Decoding(DecodingError { format: Exact(Avif), underlying: Some(Error(-11)) }) on 0.24.6. There was a cheap "hack" mentioned in the tracking Dav1d issue, is it possible to adopt said fix until the underlying issue is fixed? The image used displays and works correctly in firefox, chrome and gimp.

FlareFlo commented 1 year ago

It might generally be a good idea to update dAV1d-rs, as the current version used by image was released more than a full year ago (6.0 => 9.3). It appears that said issue was addressed in this commit. ~~While I am not well acquainted with any of said things, i might make an attempt at a PR to bump the version.~~ Updating dAV1d to 9.3 was possible with all current tests passing, but trying to call into this function, did not work out, as i have very little knowledge on the inner workings of dAV1d.

image-rs / image