Release 0.11.6 was a breaking change

[HeroicKatora]

In particular the structure AvifContext is affected. It should have been 0.12, probably. This is observed cause breakage in builds of image: https://github.com/image-rs/image/runs/3961972322?check_suite_focus=true

[HeroicKatora]

read_avif was also affected (added an argument): https://docs.rs/mp4parse/0.11.6/mp4parse/fn.read_avif.html

[HeroicKatora]

I'm also looking for some guidance on this upgrade. Previously, AvifContext::alpha_item() -> Option<&[u8]> had been used to determine if an alpha channel was present. If so it was then decoded (Decoder::send_data)—otherwise the color type was set to pure RGB without alpha channel. I don't see how this information could be determined from the current API based on the documentation. Indeed alpha_item is private, and both alpha_item_coded_data and alpha_item_bits_per_channel substitute a value when the internal Option is None.

Is an empty slice a unique return value that can only happen when no alpha is present? This reading would be supported by the history but I find it odd in the context of Rust's type system..

• It was made private in 7245b46 but it still had a public Option<&[u8]> accessor at the time.
• In 3cb73c9 the signature was subsequently changed to a definitive slice. The documentation was also adjusted to:

  If avif_image.alpha_image.coded_data is set to a positive length and non-null data, then the avif_image contains valid alpha channel data

[baumanj]

In particular the structure AvifContext is affected. It should have been 0.12, probably. This is observed cause breakage in builds of image: https://github.com/image-rs/image/runs/3961972322?check_suite_focus=true

Sorry if that came as a surprise. Though semver doesn't guarantee anything about breaking changes prior to 1.0:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

The API definitely isn't stable at this point, but knowing how it's used will be helpful for moving it in that direction. And even more importantly, it's really focused on the C API in mp4parse_capi, rather than providing a good API for rust consumers using mp4parse directly. We're definitely open to providing more reasonable public APIs though, so filing issues is a good signal for us on where we can improve.

Is an empty slice a unique return value that can only happen when no alpha is present? This reading would be supported by the history but I find it odd in the context of Rust's type system..

That's correct on both counts, so checking for alpha_item_coded_data.is_empty() is probably the best check now. I don't know if it's possible to contrive an AVIF input with a zero-byte alpha channel, but I can't imagine a consumer wanting to handle it any differently. Still, your point about idiomatic rust is a good one, and these methods would be better returning an Option.

If you have any other feedback on the API, let me know. Otherwise, I'll clean the alpha methods up shortly and resolve the issue when I'm done with that.

[HeroicKatora]

Sorry if that came as a surprise. Though semver doesn't guarantee anything about breaking changes prior to 1.0:

This is definitely not correct (or.. at least conventional), at least in the context of Rust but such a reading would be close to non-sensical in a global context. Here's the official RFC on the matter, 1104-api evolution which clearly defintes to a major change. It does guaratee stability across compatible version ranges (the spec 0.11.5—interpreted as ^0.11.5 is compatible with 0.11.6). It simply should have been 0.12.0. Consider that allowing any change in any version increment makes patch numbers completely useless.

[baumanj]

Thanks for the link to the RFC; I'll check that out. In the meantime, what exactly are you asking for in this issue? Do you want 0.11.6 yanked and re-released as 0.12.x, or something else?

[HeroicKatora]

I mainly wanted to make you aware that your release/versioning policies conflict with established practice in the Rust ecosystem and that this might mean lower usage—or outdated (=broken) dependencies. At the moment a downstream crate can only pin to an exact version number which is generally avoided due to the aforementioned negative impliciations. I'd also ask you to make the policy abundantly clear in the Readme.

This also brings me to the main question. I'm prepared to fixup the breakage in image but I need to choose a version. If you can be convinced that major changes before 1.0 should increase the minor version then I'll happily just bump it to 0.11.6 and the fix will hopefully not reach any further downstream before that fix is out. However, if you continue that policy then doing so would cost us a lot of time in CI false positives, maintenance, issues being crate etc. Pinning is a solution but that's akin to acknowledging use of outdated code. It might mean we retire avif integration for the moment, which honestly would be a shame.

If you can commit to somehow maintaining the integration in some capacity, maybe a shim crate around that provides a small but very stable API that enables this code then of course that would be a decent middle ground as well. (Downside being the extra crate).

[baumanj]

I appreciate the heads-up. I do want to read the primary sources to make sure I fully understand them, but assuming that your interpretation is correct, it seems like yanking 0.11.6, and re-releasing it as 0.12.0 on crates would be quick and easy and the best thing in the short term to solve your issue. Would you agree?

I'll circle back after reading, and unless further discussion leads to a different conclusion, I think we'd be happy to make sure any API-breaking changes increment the minor version number, and not just the patch number.

[HeroicKatora]

It seems like yanking 0.11.6, and re-releasing it as 0.12.0 on crates would be quick and easy and the best thing in the short term to solve your issue. Would you agree?

Yes, I would think this defuses all the potential breakage risks arising from cargo's version resolution. A helpful third-party pointed out that the cargo documentation was better formatted, more recent and clearer: https://doc.rust-lang.org/cargo/reference/semver.html.

This guide uses the terms "major" and "minor" assuming this relates to a "1.0.0" release or later. Initial development releases starting with "0.y.z" can treat changes in "y" as a major release, and "z" as a minor release. "0.0.z" releases are always major changes. This is because Cargo uses the convention that only changes in the left-most non-zero component are considered incompatible.

[baumanj]

https://github.com/mozilla/mp4parse-rust/releases/tag/v0.12.0 and the corresponding crates.io releases are done now, so I'll resolve this issue.

Please feel free to file separate issues for API changes like the alpha stuff we were discussing.

[HeroicKatora]

Thanks, I think the API updates have also been answered sufficiently. I suppose it's only missing clarifying documentation on the pure Rust interface so I'll send a PR if I get around to it.