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

Avif and webp recognition is bugged #1647

Open
installgentoo opened this issue Jan 12, 2022 · 15 comments
Open

Avif and webp recognition is bugged #1647

installgentoo opened this issue Jan 12, 2022 · 15 comments
Labels
help wanted kind: bug topic: formats Towards better encoding format coverage

Comments

@installgentoo
Copy link
Contributor

installgentoo commented Jan 12, 2022

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
Copy link
Contributor Author

installgentoo commented Jan 12, 2022

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
Copy link
Contributor Author

installgentoo commented Jan 12, 2022

// 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

installgentoo commented Apr 14, 2022

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
Copy link
Member

@installgentoo
Copy link
Contributor Author

#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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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

@fabiosky
Copy link

I opened aws/dcv-color-primitives#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
Copy link
Contributor Author

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

btw this also affects webp

@installgentoo installgentoo changed the title Avif recognition is bugged Avif and webp recognition is bugged May 19, 2022
@HeroicKatora HeroicKatora added kind: bug topic: formats Towards better encoding format coverage labels Jun 11, 2022
@FlareFlo
Copy link
Contributor

FlareFlo commented Apr 13, 2023

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
Copy link
Contributor

FlareFlo commented Apr 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted kind: bug topic: formats Towards better encoding format coverage
Projects
None yet
Development

No branches or pull requests

5 participants