Avif YUV decoder, drop dcv, high bit depth

[awxkee]

This should close #2130, #2232, #1504, #1647.

Here is also bugfix because actually YUV400 just tried to copy Y plane data straight to RGBA what is always fails.
YUV decoding is intentionally was made only to RGBA color format without Rgb, Gray, GrayAlpha and others to not break current ColorType resolution strategy and to not make PR even larger.

YUV decoder made as per ITU-R standard. Here is info.

Closes #2130, Closes #2232, Closes #1504, Closes #1647, Closes #1930

[HeroicKatora]

I like the transform, quite clean and encompassing in terms of what ITU actually specified. I don't see Unsupported being used correctly and an error type instead of String would be highly preferable.

[awxkee]

I changed Unspecified to Bt.601.

Even here it is marked as Bt.709 (what is a bug I consider )

image/src/codecs/avif/encoder.rs

Lines 35 to 40 in cf9c532

	pub enum ColorSpace {
	/// sRGB colorspace
	Srgb,
	/// BT.709 colorspace
	Bt709,
	}

Actually ravif is using Bt.601 as default, so to not create gaps in implementation I think it is better to go this way.

[fintelia]

Left a few comments, but I had a hard time reviewing the PR. I've started reading through like three times and every time I get lost trying to keep all the different cases in my head.

I'd encourage finding ways to consolidate different cases. As one example, if yuv400_to_rgba8 took a YuvPlanarImage with u_plane and v_plane set to &[] then you wouldn't need a completely separate code path to deal with it.

[awxkee]

Left a few comments, but I had a hard time reviewing the PR. I've started reading through like three times and every time I get lost trying to keep all the different cases in my head.

I'd encourage finding ways to consolidate different cases. As one example, if yuv400_to_rgba8 took a YuvPlanarImage with u_plane and v_plane set to &[] then you wouldn't need a completely separate code path to deal with it.

Sorry, I tried to keep it small, but all these takes some space.

[awxkee]

Yes, using YuvPlanarImage for YUV 4:0:0 will remove some code. However, if you'll decide to expose this method to whole create it will be definitely not clear how to use this.
If it is fine to you it is fine by me.

[awxkee]

This is more clean now.

However, actual complexity just became obscured instead of explicit.
If someone we’ll try to use those methods better to think twice about what he’s doing.

[awxkee]

Perhaps, I can do some simple strategy pattern here, for each case GBR, 4:0:0, or other, something like that.

trait AvifResolutionStrategy {
    fn is_supported(&self, picture: Picture) -> bool;
    fn process<R: Read>(picture: Picture, buf: &mut [u8]) -> ImageResult<()>;
}

This will also keep everything clear, but will notably increase amount of code.

Otherwise, if you don't like that one, I think at the moment there is the best code at least in AvifDecoder, for a price that YUV converters magically accept or not accept some arguments.

[HeroicKatora]

At least from the standpoint of review, the complexity has gone down drastically—so thank you for the refactorings. The 'odd' case still is an unfortunate fence post in the loop and it looks bulky. But more importantly it is possible to read many sections of code independent from each other now. In terms of numerics, I'm predisposed to the yuv transform which probably helped.

I wouldn't refactor with more type system. That's just going to decrease readability and the dispatch here doesn't need to be dynamic, so please don't. If anything iterate on this structure instead 👍

Thanks for contributing these to the image-rs project. In my head I'm considering copying to a separate crate but let's land it first. Since it is quite a bulk, I'd let @fintelia weigh in with comments before the merge; but this is quite close to approval from my side.

[awxkee]

I also splat a little 4:2:0 and 4:2:2 processing, I think it'd make quite easier to understand what's going on there.

[fintelia]

Left one more comment. I've only looked at the interface in decoder.rs, so I'll defer to other reviewers about whether this PR is otherwise ready to merge.

One other thing to mention is that image-png just got support for coding-independent code points (aka ITU-REC-H.273) in image-rs/image-png#529. PNG lacks supports for YUV, so it is just the metadata information about what colorspace the RGB channels are in. Might be worth thinking about how to have a unified interface between AVIF and PNG

[awxkee]

So, as there again was raised a concern about what actually targets are,
I would like to hear what matrix should be set on Unspecified.

To simplify a question, if the compatibility with ravif is preferred then Bt.601 should be kept, if compatibility with browsers is a goal then Bt.709.

[fintelia]

I'd say we should probably match web browsers. Plus, we tend to assume sRGB throughout the crate, so this wouldn't be a big departure

[HeroicKatora]

LGTM

[Shnatsel]

Now that this PR is approved I'm going to hit the merge button, unless @fintelia objects?

I'll also start preparing a point release because we have plenty of fixes and improvements in git already.

[HeroicKatora]

@Shnatsel You can press the merge button next time. As far as I understood @fintelia had already deferred the remaining review to me. I just got side tracked between the LGTM and merging with #dayjob work. Thanks for preparing the release notes already 👍