image icon indicating copy to clipboard operation
image copied to clipboard

Avif and webp recognition is bugged

Open installgentoo opened this issue 3 years ago • 15 comments

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 avatar Jan 12 '22 03:01 installgentoo

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 avatar Jan 12 '22 05:01 installgentoo

// 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 avatar Jan 12 '22 06:01 installgentoo

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 avatar Jan 12 '22 09:01 installgentoo

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

installgentoo avatar Apr 14 '22 10:04 installgentoo

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

HeroicKatora avatar Apr 14 '22 10:04 HeroicKatora

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

installgentoo avatar Apr 14 '22 10:04 installgentoo

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

Huh. Thanks, good pointer.

HeroicKatora avatar Apr 14 '22 11:04 HeroicKatora

#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 avatar Apr 14 '22 11:04 installgentoo

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.

installgentoo avatar Apr 14 '22 11:04 installgentoo

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.

fabiosky avatar May 17 '22 09:05 fabiosky

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

installgentoo avatar May 17 '22 09:05 installgentoo

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

fabiosky avatar May 17 '22 10:05 fabiosky

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

btw this also affects webp

installgentoo avatar May 19 '22 15:05 installgentoo

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 avatar Apr 13 '23 17:04 FlareFlo

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.

FlareFlo avatar Apr 14 '23 16:04 FlareFlo