Add a Freeimage plugin with PluginV3 support.

[michael080808]

Hi, guys! I just spent 2 days to add a small PluginV3 support for the Freeimage.
I need Freeimage as my backend to handle with very complex image datasets including 16-bit/32-bit PNG and others.

I tried my best to follow the code from
https://github.com/imageio/imageio/blob/master/imageio/plugins/pillow.py
https://github.com/imageio/imageio/blob/master/imageio/plugins/freeimage.py
https://github.com/imageio/imageio/blob/master/imageio/plugins/freeimagemulti.py
but I only tested some JPEG and PNG from imageio url images.
GIF and other formats are not fully tested because of my poor understand of the code.

Another thing is that I did not understand how to achieve exclude_applied in the PluginV3.metadata.
Docs are also needed to add to these codes. These parts need your guys help.

Here are the examples of the new 'freeimage' plugin:

import time

import imageio
from matplotlib import pyplot as plt

uris = [
    # 'chelsea.bsdf',
    'newtonscradle.gif',
    'bricks.jpg',
    'meadow_cube.jpg',
    'wood.jpg',
    # 'cockatoo.mp4',
    # 'stent.npz',
    'astronaut.png',
    'camera.png',
    'checkerboard.png',
    'chelsea.png',
    'clock.png',
    'coffee.png',
    'coins.png',
    'horse.png',
    'hubble_deep_field.png',
    'immunohistochemistry.png',
    'moon.png',
    'page.png',
    'text.png',
    'wikkie.png',
    # 'chelsea.zip',
]

if __name__ == '__main__':
    for uri in uris:
        with imageio.v3.imopen('imageio:' + uri, io_mode='r', plugin='freeimage', flags=0, test=None) as image_file:
            for index, image in enumerate(image_file.iter()):
                plt.title(uri + ' %d' % index), plt.imshow(image), plt.show(), time.sleep(1)
        img = imageio.v3.imread('imageio:' + uri, index=0, plugin='freeimage', flags=0, test=None)
        plt.title(uri), plt.imshow(img), plt.show(), time.sleep(1)
        imageio.v3.imwrite(uri, img, plugin='freeimage', flags=0)

[FirefoxMetzger]

Awesome stuff 🤩 a rewrite of the free image plugin is very needed. Thanks for taking the time :)

I left detailed feedback for the first 100-ish lines and will have to come back for the rest once I can dedicate another hour or two to reviewing. The code so far looks good but is a little too branch heavy to merge as is. This is an easy fix though and I left some comments where I saw opportunities to refactor. I think we shouldn't need a lot of rounds before we can merge :)

Once again, great job and great initiative 🚀

[FirefoxMetzger]

We use black as our default formatter. I don't have a strong preference on how code should be formatted (as long as it passes linting), but I suspect that our linter does 😇 .

[michael080808]

I use PyCharm IDE to code the python file. It's a little annoying with its formmatter and auto wrap line. So, it's freely to change the format to meet the black.

[FirefoxMetzger]

Is this a residue from debugging and should be removed?

[michael080808]

Yeah, I forgot to remove it. That's only for debugging.

[FirefoxMetzger]

We don't have to explicitly raise StopIteration here. Since we are using yield inside the function it will become a generator and Python will automatically raise this exception when the function returns.

[michael080808]

Maybe my Python writting style is a little redundant and obsessive to the lines and indents. So there may be a lot unreasonable codes to understand. Thanks for your advise!

[FirefoxMetzger]

Same here. We can just let the function return as usual :)

[FirefoxMetzger]

Suggested change

	raise InitializationError('Can not iterate with current settings.')
	raise ValueError('Can not iterate with current settings.')

This should not be an InitializationError. We only raise InitializationError during initialization of a plugin to communicate with plugin selection that the plugin is not able to read images of this type.

If possible, we should instead try and use standard exceptions and I think the one that fits best here is a ValueError:

Raised when an operation or function receives an argument that has the right type but an inappropriate value, and the situation is not described by a more precise exception such as IndexError.

[FirefoxMetzger]

We should reduce nested branches as much as possible. I think we can write the following here:

if not fi.has_lib():
    raise ImportError("FreeImage Library not installed.")

try:
    ...

[FirefoxMetzger]

Suggested change

	raise InitializationError(f"Freeimage can not {self._io_mode.name} {request.raw_uri}.") from None
	raise InitializationError(f"Freeimage can not {self._io_mode.name} `{request.raw_uri}`.") from None

We typically add ticks around the URI to visually separate it from the exception message.

[FirefoxMetzger]

Are we expecting a specific exception here? Maybe we don't want to change anything, but I am always a bit wary when adding a generic "catch all exceptions" block, because it might swallow something that we should have raised instead.

[michael080808]

I remember that when _fif = -1 means FreeImage can not recognize the image format. Because after this Exception some procedures should do. I'll double check the v2 code to corfirm it.

[FirefoxMetzger]

How come you are storing the kwargs here? Is it required to know all arguments used during initialization when later reading/writing data?

[michael080808]

Yes, that's true. I just found that imopen and imread/imwrite give arguments from different places. Is there any better solution?

[FirefoxMetzger]

Suggested change

# print(inspect.currentframe().f_code.co_name, self._kwargs)

Left over debugging comment?

[FirefoxMetzger]

Another thing is that I did not understand how to achieve exclude_applied in the PluginV3.metadata.

As for your question, the exclude_applied flag removes all metadata that the reader has applied to the image. The classic example is a file that sets the EXIF flag rotation. Most readers (but not all) will auto-rotate the image when reading and will also return the rotation flag when reading the metadata. This can be confusing for the user as they can't easily tell if they have to manually rotate the image (reader doesn't auto-rotate) or not (reader auto-rotates). The same is true for other metadata, e.g., auto-applied gamma correction, white balance, etc.

To differentiate this, we exclude any metadata that the reader applies by default. This way a reader that auto-rotates won't report the rotation metadata and the user can have a clear feedback on whether they should rotate manually or not.

Another way to think about it is to view exclude_applied as a flag that allows the user to choose between "raw" metadata and "actual" metadata. "raw" metadata (exclude_applied=False) is the metadata that is stored in the file whereas "actual" metadata (exclude_applied=True) is metadata that is still relevant for the image that is served by read/iter.

Docs are also needed to add to these codes. These parts need your guys help.

As for the docs we should add them (and I will help you get them into the correct state). I would like to do a full review first though, and then maybe discuss adding some tests for the new code 🙃 . This way we avoid having to rewrite documentation because we might do slight changes to the signature of some functions as part of the review.

[michael080808]

Thanks for your patient review! This is my first time to pull request on Github in fact. I am just a little worried about my code's quality. It's very useful for your advise. I just copied a lot V2 code to make sure the logic of procedures right. (Maybe there are some historical codes which do not make sense.) As I said I did not learn the operations of FreeImage very well but I'll try my best to learn it and make the code better.

By the way, this little code is used for my graduation thesis for postgraduate degree. I'm not sure I'll be free in the following months. However, I'll try my best to catch up.

[FirefoxMetzger]

This is my first time to pull request on Github in fact.

Awesome 🚀 thank you for contributing to open source :) If you are unsure how things work or have questions about the process, feel free to ask and I will explain.

I am just a little worried about my code's quality. It's very useful for your advise.

Thanks! Don't stress too much about the initial code quality. The code you see in FOSS repos is quite refined, but that's the result of thorough peer review. A PR usually starts out in a state quite close to what you have submitted, then people provide their feedback, and we incrementally improve what's there. This process allows sharing best practices and knowledge, e.g., you can learn some of the tricks I've picked up over the years and I can learn from some of your ideas.

I'm not sure I'll be free in the following months. However, I'll try my best to catch up.

No problem. The one decision we need to make then is if you want to keep working on this PR or if we should open it to outside contributions. I.e., should I try to find someone else how adds what's missing before we can merge (documentation, unit tests, some refactor) or do you want to do it when you are more free again?

[michael080808]

No problem. The one decision we need to make then is if you want to keep working on this PR or if we should open it to outside contributions. I.e., should I try to find someone else how adds what's missing before we can merge (documentation, unit tests, some refactor) or do you want to do it when you are more free again?

You can find some other contributers to finish the jobs. That is OK for me. I think it can accelerate the procedures and make more people use it ASAP. Best wishes for all the contributers and the project! 😊