Handling strict parsing via a cargo feature is problematic

[sdroege]

Latest release added a strict cargo feature (disabled by default), which causes unknown features to end up as errors instead of being ignored.

This is problematic because mp4-atom might be pulled in (directly or indirectly) by multiple crates in the dependency tree, and if only one of them enables the feature then it's enabled for all usages. However some crates might depend on the cargo feature to be not enabled (e.g. to be able to handle random files as good as possible despite there being unknown boxes in it).

It would be better to make this a runtime decision at each place where the parser is used.

[bradh]

Latest release added a strict cargo feature (disabled by default), which causes unknown features to end up as errors instead of being ignored.

Yes.

This is problematic because mp4-atom might be pulled in (directly or indirectly) by multiple crates in the dependency tree, and if only one of them enables the feature then it's enabled for all usages.

There aren't many dependents of mp4-atom, so I don't see this as a big issue.

However some crates might depend on the cargo feature to be not enabled (e.g. to be able to handle random files as good as possible despite there being unknown boxes in it).

I'd be open to adding documentation to explicitly note this possibility.

It would be better to make this a runtime decision at each place where the parser is used.

I'm not sure how this would work. What kind of API do you have in mind? For example, if I'm flexible about ignoring stuff under udta but really want to flag an error for a missing codec?

[kixelated]

Like I agree in theory, it's weird that a dependency could change the runtime behavior of a crate.

Before, the library would arbitrarily error on unknown atoms. We changed it to log by default while still giving you an option to enable errors for stuff like unit tests or more opinionated services.

But I can't really envision a scenario where you want both strict and loose. Passing this as context just seems like overkill and would require opting into strict at every call point.

I would be fine with any of these:

1. Add loose and make it mutually exclusive with strict. Default is loose.
2. Remove strict and use tracing_test to error on warnings instead.
3. Same as 1 but with no default. Behaves the same as loose with no features present.

[sdroege]

An example where this would break is a cargo workspace where some crate enables strict while another one doesn't (and because features are additive, both will get strict), and both crates could have basically nothing to do with each other than being in the same workspace (e.g. a validation tool and a transmuxing tool).

[kixelated]

An example where this would break is a cargo workspace where some crate enables strict while another one doesn't (and because features are additive, both will get strict), and both crates could have basically nothing to do with each other than being in the same workspace (e.g. a validation tool and a transmuxing tool).

Absolutely, although it wouldn't "break" in the traditional sense, like a compilation error. Instead the behavior of the transmuxing tool would change, now returning an error for additional non-standard files. And the fix would be to add support to mp4-atom if the non-standard behavior is widespread enough.

Again, I'm fine adding a loose feature flag if the transmuxing tool really really needs to make sure that strict is not enabled, but it will error if both strict and loose are enabled in the same workspace (and the same minor version is used).

Otherwise I don't really have any good ideas. @sdroege what sort of API would you envision?

[sdroege]

The best I can come up with is that decode() etc could take another parameter with a parser configuration. I don't have solutions, I only have problems to offer :)