torchvision.io and torchvision.utils

[oluwandabira]

As per the original discussion on #471 I'm opening up a proper discussion.

I have created a branch on my fork , vision-io, for writing the code examples.

Major considerations for this implementation are:

• Using an abstraction for actual reading/writing image files so users can use any .NET image library.
• Supporting .NET's async I/O pattern.

[oluwandabira]

A new example, IOReadWrite and its corresponding stub implementation, contain a very basic idea of what the image reading and writing would look like.

The files also contain my reason for including torchvision.utils in the title of this discussion. save_image in PyTorch's torchvision is not in torchvision.io but in torchvision.utils and I was wondering if we want to keep that separation.

I also want commentary on if the method casing is appropriate as @NiklasGustafsson mentioned that torchvision will be moving into its own package and I'm not sure if we're keeping the pythonic naming especially since we would have to add the async methods which would end up as read_image_async or read_imageAsync .

[NiklasGustafsson]

Looking at read_image(), I come to think of how it makes sense for Pytorch to have the GRAY_ALPHA and RGB_ALPHA image modes. How is the alpha channel represented in tensors? Usually, you have one or three channels, but transparency would require a second or fourth channel?

What's the common practice?

[oluwandabira]

torchvision does indeed include a second and forth channel for GRAY_ALPHA and RGB_ALPHA respectively.

I am unsure of what the common practice is but opencv also includes a forth channel when reading png's with IMREAD_UNCHANGED so it's not uncommon.

[NiklasGustafsson]

That's interesting. There are a couple of APIs that assume 1 or 3 color channels. equalize(), for example:

https://pytorch.org/vision/stable/transforms.html#torchvision.transforms.functional.equalize

I wonder if we can just disable the alpha-channel option and add them if there's demand.

[oluwandabira]

I have updated my branch to add more methods, an abstraction for imaging implementations, a single async method read_image_async (to gauge if I'm going in the right direction) and commented out the alpha modes. However, unless we explicitly check that the amount of color channels matches the supplied mode (should we?) it seems up to the IImager implementation to actually disable/enable alpha channels.

[NiklasGustafsson]

I have updated my branch to add more methods

I'll take a look later today. Thanks for doing this work!

it seems up to the IImager implementation to actually disable/enable alpha channels.

Maybe. I'm not sure if it's a producer or a consumer constraint on the API. In the former case, the implementation should disable, in the latter, the API should, IMO.

BTW, I'm doing to make a release to NuGet either today or tomorrow morning (Pacific Time), and then I'm going on vacation starting December 6. There won't be time to get this all settled, reviewed, and released before I disappear, so it'll be 2022 before it can get released. Plenty of time to iterate on it... :-)

[oluwandabira]

I've also add stubs for Video without the Fine-grained video API for now.

[oluwandabira]

1. I'm personally in favor of dropping support for format specific encoding and leaving it up to the IImager implementation to detect the format.
2. I think we should have no default PrimaryImager and instead maybe offer (if possible) separate packages for several popular image libraries (or at least code snippets in the Readme).
3. Each of the image methods takes an imager parameter which, if present, is used in place of the PrimaryImager.

[michaelgsharp]

So we talked about this again this morning. Our plan for ML.NET is to use ImageSharp internally, but to expose an exchange type for the public API's so the user can use any image framework that they want. The latest preview of .NET 7 has actually disabled System.Drawing for all non-windows OS, so this is something that has been bumped up in priority for us and we will need to tackle soon.

[NiklasGustafsson]

In torchvision, the "exchange type" is Tensor, so that's pretty straight-forward.

If ML.NET is taking a dependency on ImageSharp, anyway, then maybe the right thing is to take a hard dependency on ImageSharp and have that as the default imager, but allow a user to define another "Imager" instance globally or via parameters like @oluwandabira is suggesting.

[oluwandabira]

I have updated my branch and made a stub DefaultImager using ImageSharp (also renamed PrimaryImager to DefaultImager). The imager ImagerSharp currently only has a very small section implemented because I don't know if the method I used to create the Tensor is acceptable.

I should note that ImageSharp has a different behavior regarding alpha channels for JPEGs. It adds the alpha channel with no problem (when reading an image as Rgba32) and simply sets all values to 255 as opposed to spitting out an error when reading an image with ImageReadMode.RGB_ALPHA like in the original torchvision.

Also I have added support for image formats with the ImageFormat enum (can't encode without knowing which format) and also added an overload for DecodeImage that allows specifying the image format to decode from. We can add more image formats later and have the IImager implementations decide what to do with them.

On another note, does @NiklasGustafsson still want to move the TorchVision package to its own package? If so it might be best to do it before we add a dependency to TorchSharp just for TorchVision.

[oluwandabira]

Okay, looking through the code, there is a way faster way to do this involving Span<T> but as Span<T> doesn't work in async methods (as I understand it, I'm not that used to C#), I'm not sure about using it since my original plan for the async methods was to just call to regular version.

[NiklasGustafsson]

On another note, does @NiklasGustafsson still want to move the TorchVision package to its own package? If so it might be best to do it before we add a dependency to TorchSharp just for TorchVision.

I'm not sure. It seemed like the right thing to do, just to keep the size of the base package smaller, but now a number of the TV features have been implemented in native code, which complicates things. What do others think? @michaelgsharp, what's your opinion? @dsyme?

my original plan for the async methods was to just call to regular version.

That's not generally a good idea, IMO. It just means spinning up a new thread to do synchronous work and then wait for it. Unless you're unblocking the UI thread, or parallelizing I/O, I don't think it makes sense. Does ImageSharp not have async versions of the I/O methods?

[oluwandabira]

ImageSharp does have async versions for I/O methods, guess I'll have to use those and extend the IImager spec.

[oluwandabira]

Actually, while ImageSharp does indeed have async I/O methods, the current IImager spec doesn't call for any I/O, it operates on byte[] and Tensor and the wrappers are the ones doing the I/O , do I still need aync versions of the ImageSharp I/O methods?

[NiklasGustafsson]

Actually, while ImageSharp does indeed have async I/O methods, the current IImager spec doesn't call for any I/O, it operates on byte[] and Tensor and the wrappers are the ones doing the I/O , do I still need aync versions of the ImageSharp I/O methods?

I see what you mean -- you are right, the Imager instances wouldn't be doing any I/O. I think I assumed that ImageSharp (which I'm not intimate with), would be able to do the read and decode in one operation. For large data, such as video, you wouldn't want to read all the bytes and then decode, I don't think. I'm not sure if it matters for images these days.

I guess what I was really concerned about is the actual I/O:

        public static Tensor read_image(string filename, ImageReadMode mode = ImageReadMode.UNCHANGED, IImager imager = null)
        {
            return (imager ?? DefaultImager).DecodeImage(File.ReadAllBytes(filename), mode);
        }

        public static async Task<Tensor> read_image_async(string filename, ImageReadMode mode = ImageReadMode.UNCHANGED, IImager imager = null)
        {
            return await Task.Run(() =>
            {
                return read_image(filename, mode, imager);
            });
        }

In this code, the _async method should be using File.ReadAllBytesAsync(), await the result, then decode and return.

Something like:

        public static async Task<Tensor> read_image_async(string filename, ImageReadMode mode = ImageReadMode.UNCHANGED, IImager imager = null)
        {
            var data = await File.ReadAllBytesAsync(filename);
            return (imager ?? DefaultImager).DecodeImage(data, mode);
        }

Same for methods writing images.

[oluwandabira]

I've changed the read_image_async method to yours. ImageSharp does include overloads to Image.Load to read from a file and decode it at once (and its async counterpart) so we could change the IImager spec to require file names instead of byte data if need be.

As for video, I have the stub spec for IVideoer and I'm not sure where to go from there. torchvision also includes the Fine-grained video API and I don't know any .NET libraries for that, or for reading/writing) video at all to be honest.

[oluwandabira]

@NiklasGustafsson I've updated my branch with the complete ImagerSharp implementation and non-async reading/writing works... at least when I test it with the sample code.

[NiklasGustafsson]

@oluwandabira -- it's been a couple of months. Are you interested in turning this into a PR?

[oluwandabira]

I've opened up a draft PR.

[NiklasGustafsson]

Thx!