Unified ImageFormat for both built-in and plugin formats

[RunDevelopment]

I implemented the unified image format idea I talked about in #2683.

What is this PR:

• ImageFormat can now represent both builtin and plugin formats. This is done using a private format registry that contains all formats. ImageFormat is simply an index (=registry ID) into the list of formats.
• The API of ImageFormat is largely unchanged. The only read difference is that e.g. ImageFormat::Png is now a constant instead of an enum variant.
• The hooks API changed slightly to use ImageFormat instead of relying on extensions everywhere. The intended usage is now if let Some(my_format) = hooks::register_decoding_hook("ext", ...) { /* register magic bytes, etc. */ }
  The hooks API now always uses ImageFormat to identify formats. Plugin authors now create an empty format and then add abilities and metadata to it.
• Builtin formats have a special path to use static dispatch instead of forcing dynamic dispatch like for plugins (as per this comment).
• I added 2 new hooks to allow plugins to register extension aliases and MIME types. (Maybe this should be a separate PR?)
• I removed PCX from ImageFormat, since it was deprecated.

Since builtin formats and plugin formats are both represented using ImageFormat, API limitations for plugins like #2616 no longer exist. Users can now use plugin formats like any builtin format.

TODOs:

• ImageFormat::reading_enabled and ImageFormat::writing_enabled are currently specific to features enabled/disabled on the image crate. It's not clear what plugin formats should return here.
  My current understanding is that these functions return whether "all necessary features are enabled." So plugins (which can't define features) should return true by the all quantifier.
• Registering extension aliases currently comes with a memory leak.
• I made some minor changes to the MIME types of builtin formats. There's a TODO comment for each change.

Resolved

• Serialization and hashing for ImageFormat.

  The underlying registry IDs for builtin formats are always the same, but this is not the case for plugins. IDs are assigned sequentially, so plugin IDs depend on the number of builtin formats and the order in which plugin formats are registered. Because of that, both the hash and serialized form of plugin ImageFormats can vary. It is still deterministic, but the fact that code changes (e.g. registering a new plugin) may cause the hash/serialized form of a plugin format to change could still be surprising to users.

  Solved by using main extensions as a stable identifier.

• Some formats previously had extensions that they detected, but did not return in ImageFormat::extensions_str. Was that a bug, or an intentional feature? I currently return all registered extensions in ImageFormat::extensions_str. These were confirmed to be bugs.

• Because hooks::register_format_detection_hook now takes an ImageFormat instead of an extension, it is no longer possible to register magic bytes for formats that aren't registered. I'm not sure if we're okay with this limitation. The new format creation hook makes this possible again.

• hooks::register_decoding_hook(ext, hook) currently resolves aliases when looking for existing formats to override. This is different from before and may be a problem when extensions collide. E.g. it's currently not possible to define a format for APNG, because PNG has ".apng" as an extension alias. Fixed by not resolving main extensions in format creation hook.

• I started adding some validation to extensions when registering plugins and magic bytes (can't be empty). Should this be a separate PR? I removed all validation after feedback.

• ImageFormat::to_mime_type currently returns application/octet-stream for plugins that don't have MIME types registered. Not sure if that is the best default. Maybe the method should be changed to return an Option instead? I want to change the API of ImageFormat as little as possible right now, so let's not do that now.

[197g]

My first thought reading this is: looks far more complex; so what do we gain. The core new mechanics don't come across as fleshed out, quadratic complexity contains lookups when adding aliases, leaking every single call for alias, … I can only review on an slightly abstract high level for now; the first point would be to ask you to simplify. Also this sketch should help us find out if this approach should be pursued, as an experiment I think it provides some argument on that.

As such, I am considering using the main extension of a format for both serialization and hashing instead of the registry ID. This should ensure stability.

If this were a new type, I'd say avoid providing the trait impls you mention to ensure that users will always use the actually comparable formats that are meant for it, i.e. the mime. A method instead of the trait would also work, see f64::total_cmp.

However since this changes an existing type, I'd say it points out a design problem with the approach. Types group a set of values with common behavior but since several behavior can no longer be provided for the new interpretation of the type, is it really the same type anymore? This emphasizes the question, why not introduce this functionality as a new, different type? The point of reading_enabled and writing_enabled being meaningless for non-builtin just drives this home.

Because hooks::register_format_detection_hook now takes an ImageFormat instead of an extension, it is no longer possible to register magic bytes for formats that aren't registered. I'm not sure if we're okay with this limitation.

Not a problem per-se but we'd need to ensure you can emulate it. The extension worked as-if a corresponding format was registered without anyone having to do so explicitly. Currently involves a lot of ceremony where the fallback case requires you to come up with a decoding hook since that seems to be the only way of defining a new ImageFormat? Honestly it's all confusing considering register_decoding_hook takes a str but converts it to Option<ImageFormat> immediately.

And decoding_hook_registered on the other hand takes an OsStr (?), requires an ImageFormat to exist and immediately calls a straightforward constructor as the first thing into the function. It'd make more sense to move that to a method on the type.

Also isn't this backwards? The other way around of creating types via the detection hook or by extension itself and register_decoding_hook taking an ImageFormat makes more sense to me. There's no point registering a decoding that could never be called (it doesn't do anything on its own) ergo the construction of ImageFormat should a pre precondition (an argument) to guide the user to hooks that can be triggered (notwithstanding the point below of avoiding 'sanitization' of things that aren't actually requirements, an entrypoint to create a format without any hooks or decoders attached seems okay?).

Some formats previously had extensions that they detected, but did not return in ImageFormat::extensions_str. Was that a bug, or an intentional feature? I currently return all registered extensions in ImageFormat::extensions_str.

Looks like a bug in the current implementation, thanks for bringing it to attention.

register_format_mime_types

Leaving it out makes a better impression than a not-designed afterthought.

it's currently not possible to define a format for APNG, because PNG has ".apng" as an extension alias.

Design problem to be resolved.

I started adding some validation to extensions when registering plugins and magic bytes (can't be empty).

I really dislike that train of thought and, this is not meant to be harsh but, it's important to see what rubs me the wrong way. If it doesn't make the specification of the methods hard to uphold, why limit users? If a specific problem would be triggered, fix the code. If you know the code does not have a problem with this then catching ominous behavior just makes the interface expectations more complex and thus hard to use. Here, the user may intent to complete the type at a later stage if they want to structure their code to have different registration parts happen in a different sequence. There are sometimes technical requirements that motivate artificial limits but technical and requirement only works as an argument if they are traceable reasoning. (#2647's constant so that the structure can be put on the stack for instance is alright reasoning; even though one may disagree with self-imposing that requirement).

This applies to the 256 limit on format counts as well. It is vaguely a stopgap for all the memory leakage but it doesn't really affect it, and is it even leakage if we're still able to refer to and use the data? Also of course fix the actual leaks with Arc—keep in mind the 'static in various return types is self-imposed. Consider that particularly with the above argument for separate types, we could have &'static [&'static str] for the builtins-enum while also having another return type for the registered type's methods. (I wouldn't mind an allocation for instance, Vec<String> does not seem particularly bad. You're not using this in any tight loop).

[RunDevelopment]

Thanks for taking the time to review!

The core new mechanics don't come across as fleshed out, quadratic complexity contains lookups when adding aliases, leaking every single call for alias, …

Yup. It's far from finished. This PR draft isn't a "just needs some polish, then let's do it as is" but a "I'm working on something that I think could work, please tell me what problems you see and how we could solve them". I didn't want to work on this for a week only for it to be rejected because you folks don't like the idea fundamentally.

As for the issues you mentioned here:

• The quadratic is simple to fix. (Update: fixed; sort of)
• The memory leak isn't as easy. I wanted to use this to start a discussion about whether extension_str should return a 'static. But since you haven't even considered that in your response, I guess that's not an option. Fixing the leak under the constraints of the current API is also possible by limiting how plugins register extension aliases (see code TODOs). As long as we guarantee that hooks can add arbitrarily many extensions, the leak can be fixed.

As such, I am considering using the main extension of a format for both serialization and hashing instead of the registry ID. This should ensure stability.

If this were a new type, I'd say avoid providing the trait impls you mention to ensure that users will always use the actually comparable formats that are meant for it, i.e. the mime. A method instead of the trait would also work, see f64::total_cmp.

I don't think that's the right way to look at this. The ability to hash and ser/de an image format (builtin or plugin) is highly desirable IMO and arguably expected of an identifier. I think hashing and ser/de should definitely remain, and I am committed to making it happen.

I'm just not sure whether using extensions is the best way to do it. It should work since we are kinda using extensions as identifiers already, but I'm just not certain whether extension collision or something else might cause a problem.

However since this changes an existing type, I'd say it points out a design problem with the approach. Types group a set of values with common behavior but since several behavior can no longer be provided for the new interpretation of the type, is it really the same type anymore? This emphasizes the question, why not introduce this functionality as a new, different type? The point of reading_enabled and writing_enabled being meaningless for non-builtin just drives this home.

What "several behavior" specifically? As I see it, everything aside from the image-features-specific {reading,writing}_enabled methods can be done for plugins as well.

Not that {reading,writing}_enabled doesn't fit plugins. Just change "features in image" to "features" and the semantics make sense for plugins. E.g. image-extras also has formats behind features, so having this functionality for plugins too would be useful. The reason I haven't done any of that is that it would require significantly reworking the hooks API. Right now, hooks essentially define formats by registering abilities for them, which makes it hard to say: "I don't have that ability, but I would if you enabled a feature".

Speaking of {reading,writing}_enabled: {reading,writing}_enabled and can_{read,write} are already weird. To state what (I assume) their intended behavior is:

• reading_enable: Returns whether all necessary features for reading a format are enabled.
• can_read: Returns whether a format can be read in principle, meaning IF its necessary features are enabled.

I say "assume", becuse the doc of reading_enable just restates the method name, and the doc of can_read doesn't mention features at all. Assuming that I understood their behavior correctly, I would like to point out that the matrix of return values for these methods is weird:

	can_read: true	can_read: false
read_enabled: true	Reading is supported and enabled	???
read_enabled: false	Reading is supported but disabled	Reading is unsupported

What does it mean for a format to have reading enabled but not supported? If it just means "not supported", then having a single method returning a 3-state enum instead of having can_read+reading_enabled combining to 4 states would be better.

Sorry if this seems a little off-topic, but I had some issues even understanding what these methods are supposed to do. So before we talk about how/if we can make plugins fit these methods, I would like to talk about whether the current API to inquire about reading/writing ability should be kept as is in the first place.

Not a problem per-se but we'd need to ensure you can emulate it. The extension worked as-if a corresponding format was registered without anyone having to do so explicitly.

Okay. I wasn't sure if we even wanted to allow users to register a detection for an otherwise unknown format.

Honestly it's all confusing considering register_decoding_hook takes a str but converts it to Option<ImageFormat> immediately.

It needs to check whether the format already exists for overrides and rejection. What's confusing about that?

the construction of ImageFormat should a pre precondition (an argument) to guide the user to hooks that can be triggered ([...] an entrypoint to create a format without any hooks or decoders attached seems okay?).

So you're suggesting that plugin authors start by creating an empty ImageFormat (=one without abilities or metadata) and then add abilities and metadata to it, instead of the act of adding abilities/metadata potentially creating a new format.

If so, then I like it. This seems like a much better foundation for hooks.

If it doesn't make the specification of the methods hard to uphold, why limit users?

The empty signature check is intended to detect user errors early. If a signature is empty, it is naturally a prefix of every possible file, meaning that it will always match every file. Registering an empty signature prevents all signatures after it from working. Seems like a clear bug to me.

Another validation I considered adding is for masked signatures that have one bits where the mask has zero bits. Such signatures can never match anything, which is clearly a bug. I haven't added it yet because I wasn't sure whether it should be a debug_assert-type check or just print a warning (since it's not directly harmful).

The empty extension check is an unnecessary limitation (I think). When I wrote the code, I just didn't want to think about formats where the identifying extension is empty. Shouldn't be an issue to remove.

(Update: I removed all validation for now.)

This applies to the 256 limit on format counts as well.

I added that limit so ImageFormat can still be one byte. So just a bit of premature optimization :)

If you don't want a limit at all, we'd have to use usize for the registry ID. Otherwise, I'd suggest u32.

(Update: I used u32 for now.)

[fintelia]

I haven't looked through the code changes (this PR already exceeds the size I'm personally willing to review...) but it isn't clear to me from the PR description/comments what problem exactly is being solved here?

[RunDevelopment]

Problems it solves:

• Obviously Allow naming formats registered via hooks #2616, since that was what motivated the idea.
• It seamlessly integrates plugins into all features of the library related to image formats, without needing to change those. E.g. ImageReader, load, guess_format all just work with plugins now.
• It makes it easier to extend the capabilities of plugins. E.g. it would be trivial to add support for plugin encoders with the system in this PR, since all functions like save_buffer_with_format and ImageBuffer::save_with_format will just work.

It also doesn't have some problems that some other approaches have. E.g. using something like the internal Format enum in ImageReader everywhere comes with the problems:

• ImageFormat::Abc and "abc" might not be the same format.
• All APIs that want to support plugins need to be explicitly updated.
• Format can (likely) only provide a subset of the functionality of ImageFormat.
• From a user perspective, it's not clear why two (or maybe more) types to represent image formats exist.

[197g]

Problems it solves:

Only the first one of these points is an actual problem. The other two are descriptions of solutions and, without a motivating problem, 'easier' is not a qualifier. Easier compared to what, and how, immediately come to mind as unaddressed. The comparison is also relevant to the second point as ImageReader::open alreadys work with plugins, so there's competing and massively simpler approaches (#2683 being one) to which it should be compared to.

Format can (likely) only provide a subset of the functionality of ImageFormat.

I've explained before why that is not a shortcoming if the 'provided' functionality is forced to be wrong (in effect or implementation).

From a user perspective, it's not clear why two (or maybe more) types to represent image formats exist.

Alternative: rename the existing enum to ImageFeatures with a conversion. That's what it represents currently anyways.

[RunDevelopment]

Only the first one of these points is an actual problem.

True, my bad. Then let me try again:

Problems it solves:

• Allow naming formats registered via hooks #2616. ImageReader doesn't support manually setting plugin formats and setting builtin formats currently ignores overrides.
• guess_format doesn't support plugins.
• load doesn't support plugins.
• save_buffer_with_format, ImageBuffer::save_with_format and similar won't support plugins when support for hook encoders is added.
• Functionality builtin formats support (e.g. extension aliases and metadata like MIME types) is not supported for plugin formats, and there is no obvious way to add this functionality in a way that is transparent to users.

As I see it, all of these problems are directly caused by the divide between builtin formats and plugin formats. In effect, plugin formats are currently second-class citizens as they do not enjoy similar levels of support as builtin formats. If the justification for not adding more formats to image is that they could be plugins now, then I believe that plugins shouldn't be more complicated to use and provide functionality as if they were builtin.

Easier compared to what, and how, immediately come to mind as unaddressed.

My bad, I meant "easy". But I'm also curious what you would like me to compare it to and from what perspective. I (mistakenly) said "easier" in the context of adding new capabilities to plugins, and I am not aware of any current proposals or PRs that attempt to go in that direction.

The comparison is also relevant to the second point as ImageReader::open alreadys work with plugins, so there's competing and massively simpler approaches (#2683 being one) to which it should be compared to.

Frankly, I'm not sure how you would like me to compare them. This PR makes ImageFormat the universal type to represent all formats, thereby resolving the problems I listed. #2683 changes the internal implementation of ImageReader to use string to represent formats, fixing the issue that set_format would ignore plugin overrides and opening the path for #2662 (or similar) to allow users to set plugin formats.

However, if by "simpler approaches" you meant using extension strings as the universal type for formats, then we could compare them. But I'm still not sure what aspects of them you'd like to compare and what aspects you deem important.

From a user perspective, it's not clear why two (or maybe more) types to represent image formats exist.

Alternative: rename the existing enum to ImageFeatures with a conversion. That's what it represents currently anyways.

Could you please elaborate a bit more? I'm sorry, but I don't see how this fits into the picture.