Add a function to apply Exif rotation

[Shnatsel]

This is required to display JPEG photos correctly.

Doesn't wire it up to the JPEG decoding process yet, because that would be a semver-breaking change. But at least provides all the pieces for an API user to apply the correct rotation, together with #2291

Depends on #2292

Why this API?

A common case for a JPEG image is not to be rotated at all, i.e. have the orientation value of 1. Therefore we do not want to unconditionally make a copy of the image - often it is just a no-op. We need to do this in place.

Sadly nobody has implemented in-place 90 and 270 degree rotation yet (see #2294), so we are forced to make a copy in some cases. This copying behavior prevents us from implementing this in imageops module, which is generic over the input and so we cannot use the "replace the input with a copy" trick there.

So there are two options:

1. Enshrine the unnecessary copying in the API by returning something like Result<Option<GenericImage>> and make the caller deal with it
2. Only implement Exif rotation on DynamicImage until Add in-place versions of all rotation operations #2294 is fixed

This PR picks the second option.

[ripytide]

I still think that rotations and flips are image processing operations and as such belong in imageproc and so should not be exposed from the image crate. But if they are needed for a few codecs then keeping a small private module of rotation/flipping functions in image is fine.

[Shnatsel]

@ripytide it doesn't really make sense to move it to imageproc if image needs to use it anyway. Neither does it make sense to put it in a private module, because API consumers need it too, so we'd have to expose it in some way anyway; and copy-pasting it into imageproc and exposing it from there is not going to help anyone, least of all image maintainers.

[ripytide]

So I think there are two main use-cases in play here:

• Users wanting to rotate/flip an image in their code.
• Codec authors needing to rotate/flip images as part of the codec decode/encode process due to exif and other such metadata.

For the first use-case, It makes the most sense to me for users to look in a single location for operations that process/modify images such as rotating/flipping images, hence I would expect as a user to look in the imageproc crate. Otherwise as a user I would be confused, which image processing methods do look for in image vs imageproc? I think this is called the single-responsibility principle.

For the second use-case I can see why it is incompatible with the above solution to the first use-case since if the rotate/flip functions were located in the imageproc crate then the codec libraries could not depend on imageproc since that would create a circular dependency:

codec_crate -> imageproc -> image -> codec_crate

I think this is a bit of a problem, and the root cause is the grouping of multiple functionalities in the image crate:

• Encoding/Decoding images (ImageDecoder and similar traits/types)
• Non-Encoded Image representation (ImageBuffer and similar traits/types)

The circular dependency above is due to the fact that imageproc is depending on image but only needs the second functionality. Were the two functionalities split into separate crates then this circular dependency would not be an issue:

image-encode-decode -> codec_crate -> imageproc -> image-representation

I think this is called tight/loose coupling

Perhaps we should treat the root cause of the issue and split the image crate?

[ripytide]

Oh I've just found the #793 issue, that would have saved me some typing regarding the crate splitting 😄

[Shnatsel]

@fintelia I've updated the code based on your feedback. Does that look good?

I wasn't quite sure where to put the Orientation so I put it in its own file and exported it as a top-level type.

[Shnatsel]

@fintelia I've addressed all the feedback. All good points, thank you!

Sorry it took so long. Somehow I was convinced I've already done it, I must have dreamt it or something 😅

[Shnatsel]

There are some cargo deny and clippy issues on CI, but they are unrelated to the changes made in this PR.

[fintelia]

Left two comments on naming. Otherwise looks good to me

[fintelia]

It is a bit long, but perhaps Rotate90FlipHorizontal for consistency?

[Shnatsel]

I tried that, and my eyes just glaze over when I try to read it. I genuinely believe this option is more readable.

And if its meaning is ever unclear, there is always the doc comment.

[fintelia]

Rotate270FlipHorizontal?

[fintelia]

Actually, I wonder if we should create a new metadata module for the Orientation enum. It would be a natural place if we end up adding other types from #2222

[Shnatsel]

I've renamed the module to metadata. I did re-export the Orientation type on the top level still. I think that's appropriate for this type; we might revisit that when we add more metadata types.

I've also added a function to convert back into Exif and added a doc link from Orientation to apply_orientation().