johannesvollmer / exrs

100% Safe Rust OpenEXR file library
Other
149 stars 23 forks source link

How to get from complicated `SpecificChannel` type to `AnyChannel`? #103

Open KarelPeeters opened 3 years ago

KarelPeeters commented 3 years ago

I'm trying to return an Image from a function. The image is build using SpecificChannels::build() similar to what is done in 4_specific_write.rs. As far as I can tell the return type is impossible to write because of the closure. Even if it was possible the type is very complex, the compiler writes it as:

Image<Layer<SpecificChannels<[closure@src\main.rs:114:24: 122:10], Recursive<Recursive<Recursive<Recursive<Recursive<Recursive<Recursive<Recursive<Recursive<Recursive<NoneMore, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>, ChannelDescription>>>>

If I understood correctly the solution to this problem is to return a FlatImage instead, which basically supports a dynamic amount of channels and so doesn't need to have such a complex type.

I can't figure out how to to get there from the image I currently have though. I tried just calling .into(), and I tried looking though the docs for any connection between AnyChannel(s) and SpecificChannel(s) but couldn't find anything. Is this conversion currently supported? What do I need to do to get to it?

johannesvollmer commented 3 years ago

Yes, the return type of SpecificChannels::build was not made to be written out. However, in some cases when returning an image from a function, this might not be necessary.

You might get away writing something like fn returns_image() -> Image<Layer<impl WritableChannels<'_>>>. This still unlocks the call to image.write() even though the exact type of the channels is not spelled out, instead using the WritableChannels trait. With this approach, you can't do anything else with this image, other than writing it. Read more about the image type in the guide.

If you want to use dynamic channels, you can replace SpecificChannels with AnyChannels. Unfortunately, there is no way to construct AnyChannels from SpecificChannels. Instead of a conversion, you will have to construct the AnyChannels from scratch. This currently requires you to allocate all samples of each channel in a separate vector, see the example.

https://github.com/johannesvollmer/exrs/blob/c4a591f4b21580f5d4478455e7a28416ef770e80/examples/3_simple_write.rs#L56

The resulting type of this image could be Image<Layer<AnyChannels<FlatSamples>>> if you want to write a single layer. Or Image<Layers<AnyChannels<FlatSamples>>> if you want multiple layers (this type is also aliased as FlatImage).

You construct the image itself just as before, only the channels have to be changed. For example:

    let pixels = AnyChannels::sort(smallvec![ r, g, b, a, x, y, z, a, b, c, n, l, m ]);
    let image = Image::from_channels((2000, 1400), pixels);
    image.write().to_file("tests/images/out/strange_channels.exr").unwrap();

Does this help at all? :D

johannesvollmer commented 3 years ago

Did you manage to get it work?

KarelPeeters commented 3 years ago

Hey! Sorry it took a while to get back to you, I've been busy this last week!

I'm getting stuck when trying to get the Image<Layer<impl WritableChannels<'_>>> idea to work, this is a reduced test case:

use exr::image::{Image, Layer, FlatImage};
use exr::image::write::channels::WritableChannels;
use exr::image::write::WritableImage;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let data: Vec<f32> = vec![0.0, 1.0, 2.0];

    let image = to_exr_image(&data);
    image.write().to_file("a.exf")
}

fn to_exr_image(data: &Vec<f32>) -> Image<Layer<impl WritableChannels<'_>>> {
    let channels = exr::image::SpecificChannels::build()
        .with_channel("R")
        .with_pixel_fn(move |exr::math::Vec2(x, _)| {
            (data[x], )
        });

    Image::from_channels((data.len(), 1), channels)
}

gives me the following error:

error[E0597]: `image` does not live long enough
  --> src\main.rs:9:5
   |
9  |     image.write().to_file("a.exf")?;
   |     ^^^^^ borrowed value does not live long enough
...
12 | }
   | -
   | |
   | `image` dropped here while still borrowed
   | borrow might be used here, when `image` is dropped and runs the destructor for type `Image<Layer<impl WritableChannels<'_>>>`

I don't really understand what causes this error, I don't even really unstand the error output here, what is still borrowing image? The only thing I can think of is the Result:::Err but that's not the case as adding .unwrap() instead of ? still leaves the same error. The inferred lifetimes of to_exr_image also seem fine, replacing the function header with this still doesn't change anything:

fn to_exr_image<'a>(data: &'a Vec<f32>) -> Image<Layer<impl WritableChannels<'a>>>

Regardless, for now the other option works great. It's a bit sad that I need to create separate data vectors for each channel but it's not that bad, this is not a performance critical part of the program anyway.

johannesvollmer commented 3 years ago

Hi! Cool that it works with allocations, thanks for coming back.

The lifetimes in this trait do not relate to the internal data, but to a temporary writer object. I now realize that this approach cannot work because the lifetime would somehow have to declare a higherkinded lifetime, sorry. You'll have to use allocations for now, sadly.

Anyways, this goes to the list of Todos :)

johannesvollmer commented 3 years ago

Wait a moment!!11!! There is indeed a different way to handle this! Always has been! @KarelPeeters

How could I forget that?! (Maybe there should have been some documentation).

1. How to avoid the Recursive Type

Instead of using SpecificChannels::build(..).with_channels(..).collect(..) you can construct in yourself like this, which allows you to use tuples instead of nested types:

SpecificChannels {
    channels: ( /* tuple instead of recursive */
        ChannelDescription::named("R", SampleType::F32),
        ChannelDescription::named("G", SampleType::F32),
        ChannelDescription::named("B", SampleType::F32),
        ...
    ),
    pixels: my_image
}

Tuple size is possible up 8 for now, as I have not yet release your PR to crates.io (coming soon). Then, we have SpecificChannels<[Closure], (ChannelDescription, ChannelDescription, ChannelDescription, ...)>.

2. How to avoid Using Closures

Closures are very restricted types. But don't have to use them. You can put any type there, instead of a closure, as long as you implement GetPixel on it. For example, you can implement GetPixel for your existing image data type, and then you should finally be able to return SpecificChannels<MyImage, (ChannelDescription, ChannelDescription, ChannelDescription, ...)>. Maybe you can even use &MyImage instead of MyImage, if you can implement GetPixel for the reference of your image, instead of the image data type itself.

3. Leave this issue open

As a reminder to add better documentation, please :D Also please let me know if you are interested in trying this out, and if it worked

KarelPeeters commented 3 years ago

Ah cool that works too! If I replace the closure with my own wrapper type it's at least possible to express the return type, although Image<Layer<SpecificChannels<ImageWrapper, (ChannelDescription, ChannelDescription, ChannelDescription, ChannelDescription, ChannelDescription, ChannelDescription, ChannelDescription)>>> is still not very ergonomical.

Interestingly Image<Layer<impl WritableChannels<'_>>> in code similar to my example before still runs into a lifetime issue, I'm not sure why, the error message is not very helpful (just "does not live long enough"). Maybe there's a good reason for it though, I'll have to investigate some more.

I'll go with this GetPixel way of doing things for the cases where I have a known amount of channels and go with the other option for cases where I dont. Thanks!

johannesvollmer commented 3 years ago

Nice. Its compile time checked, so we have to write it out manually, unfortunately. There are two solutions for this that I could imagine:

Allowing [ChannelDescription; N] in addition to the tuple type can simplify the type signature without losing type checking. Sounds like a simple change.

Allowing AnyChannel to contain a GetPixel instead of only allowing an allocated Vec<f32>. This sounds rather involved, but should be possible.

Do you have an opinion on that?

johannesvollmer commented 3 years ago

Interestingly Image<Layer<impl WritableChannels<'_>>> in code similar to my example before still runs into a lifetime issue, I'm not sure why, the error message is not very helpful (just "does not live long enough").

That is because the Writable channels trait is using a complicated lifetime type in Rust. The trait function does not simply borrow for a while and then let go. Instead, it actually returns a new object which borrows from the image, as long as this new object exists. There is no way for you to somehow specify this lifetime. The trait is not designed to be used as a return type, unfortunately, so it will probably never work.