johannesvollmer / exrs

100% Safe Rust OpenEXR file library
Other
154 stars 24 forks source link

Add autocrop functionality #84

Closed virtualritz closed 3 years ago

virtualritz commented 4 years ago

This would compute a tight(ened) data window by looking at all pixels in all channels. Empty pixels are those with value zero except in depth channels where it's those bigger or equal to the max value of the type (float or half).

This is a convenience function many EXR writers for 3D renderers offer.

Could be a parameter to infer_meta_data() maybe? For most uses cases having such a shrunken data window is the desirable behavior so it could even be a default that has to be turned off explicitly.

johannesvollmer commented 4 years ago

Yeah, I've seen this optimization before and I thought it was awesome.

However, there are some things that are not clear to me:

Due to all these questions, image processing is not a trivial task. That's why exrs does not offer any kind of image processing yet.

For a long time now, I have been thinking about offering an additional crate that offers more sophisticated image processing for exrs images. This would include auto crop, color conversion, and similar stuff. The benefit of this being a separate crate is that it can be updated more regularly and have a more opinionated set of features. Another benefit would be that exrs would remain a file reading library instead of slowly becoming an image processing library.

virtualritz commented 4 years ago

will this always discard transparent pixels, or only if they are also black? [...]

No. If any pixel has a nonzero value in any channel that pixel will be inside the data window. Alpha is not special. As I said – this considers all channels.

will this use the GPU or will it be slow?

It will not be slow. Even if it just uses the CPU. The time it takes to visit every pixel and do a float comparison is absolutely dwarfed by the time it takes to write an EXR to disk. No need to worry about this.

what algorithm is the best for this, for GPU, for CPU, with and without Multithreading, and SIMD?

The implementation e.g. 3Delight uses is naive. And it's fast enough. It doesn't register when you have many minutes or hours to render an image and still seconds to write it. Even if it takes a full second.

For a long time now, I have been thinking about offering an additional crate that offers more sophisticated image processing for exrs images.

That's cool. But this feature doesn't process the image in any way. No pixel values are changed. It just sets some metadata.

johannesvollmer commented 4 years ago

Well, I don't agree with all those points, but I think this feature could be part of exrs now that we talked about the performance aspect.

However, I'd prefer if it was a function call rather than a boolan flag, because this makes it more flexible for the user. I'll make some plans for this

dgsantana commented 4 years ago

Been away (and still am), lost in projects and personal problems to solve. But one of the great thing about exrs crate is speed. Right now is very fast writing, even multilayer files. I can see the auto data-window as something that for multiple layers will be slow, even if we use rayon to multithread it all. It's normal for me to work with 8k multilayer files and 360 stereo multilayer files, so doing a pixel comparison in all layers and channels will be slow, so I'm all for a function to handle this (making it explicit that we want to call an expensive operation), of course, the function would add the required metadata for this to work when called.

virtualritz commented 4 years ago

The speed is insane. It's actually an issue on macOS if you wanted to chain commands.

When the renderer process that calls my display driver writing to EXR finishes the EXR on disk is zero bytes! It takes a few seconds until it has the final size (about 8.5MB). If there was another process (e.g. in a shell script) trying to read this it would fail. I actually though I was getting insane trying to debug why my EXR was 0 bytes for half an hour. Until once I got interrupted and when I returned to my machine the file was there.

I have never seen this with any other application. My only explanation is that the write is buffered to RAM and the flush to disk just takes this long. This is on a 2014 13" MBP on macOS 10.15.5.

virtualritz commented 4 years ago

On autocrop: this would be something the crate user has to request of course. The ‘auto’ refers to the method/flag figuring out the crop bounds. Not this being switched on by default in the crate’s API.

dgsantana commented 4 years ago

The speed is insane. It's actually an issue on macOS if you wanted to chain commands.

When the renderer process that calls my display driver writing to EXR finishes the EXR on disk is zero bytes! It takes a few seconds until it has the final size (about 8.5MB). If there was another process (e.g. in a shell script) trying to read this it would fail. I actually though I was getting insane trying to debug why my EXR was 0 bytes for half an hour. Until once I got interrupted and when I returned to my machine the file was there.

I have never seen this with any other application. My only explanation is that the write is buffered to RAM and the flush to disk just takes this long. This is on a 2014 13" MBP on macOS 10.15.5.

Weird, but macOS is the only platform we haven't tested. We do something similar in Unity, where we save multilayer files from front framebuffer, g-buffers and some extra passes. And on Windows 10 and Linux, its fast, on non-ssd disks, much faster then our old way, where we used the official OpenEXR library.

virtualritz commented 4 years ago

Did you measure how much faster than the official lib it is? That would be interesting. A lot of people may want to use this crate (most probably with a C/C++ wrapper) if they learn about this.

johannesvollmer commented 4 years ago

Awesome to hear from you! Actually, I never did real comparisons and just assumed that exrs would be slower than the original implementation, haha. Didn't expect we would be faster! Cool.

If I remember correctly, writing the file should include a flush call at the end, so the write function should not return until the file is actually on disk, I guess. Don't know if the OS will still do something else after that

johannesvollmer commented 4 years ago

Now that I checked, I think it's actually missing. Maybe it got lost in a refactoring, I'll open an issue to add this in again.

Edit: found it, this function is the base of all image writing and should actually wait for flushing:

https://github.com/johannesvollmer/exrs/blob/0fa7fb15bf1e89939ee0fb37c5881db10d5df43e/src/block/lines.rs#L228-L231

virtualritz commented 4 years ago

Aha! That explains what I am seeing on macOS. Lol! Edit: Let's see what the speed is after this is added. I wouldn't be surprised if this may be responsible for the vast perceived speed difference.

johannesvollmer commented 4 years ago

I just found out that it should actually wait, I edited the previous comment because I didn't expect you to instantly react haha

johannesvollmer commented 4 years ago

If you have the time you could clone exrs and insert a breakpoint at that line to check whether your file triggers this line or not

virtualritz commented 4 years ago

Indeed. I don't have time to experiment with this atm though. If some studio pays me to work more on the EXR display driver because of the built-in IOID denoising I added I will have to get to the bottom of this. Until then let's call it a curiosity. ;)

dgsantana commented 4 years ago

Did you measure how much faster than the official lib it is? That would be interesting. A lot of people may want to use this crate (most probably with a C/C++ wrapper) if they learn about this.

As soon as I have some time I will do the comparison, the OpenEXR version I have is Windows only (because I rely on other C++ libs like WMF9), and I spend all my time on Linux lately, and avoid Windows at all cost :stuck_out_tongue: . Moving our code to Rust and finding this awesome crate, allowed us to easily make it cross-platform. I can write a C++ wrapper that can be included in this crate if @johannesvollmer wants, but I would wait for things to stabilize. But I think for VFX and Film work, in general, this crate as a lot of potential.

virtualritz commented 4 years ago

I know the 3Delight guys wrote their own EXR lib in C/C++ because the official one sucked so much (quote). From what I can tell they kept the same API to allow an easy fallback in case e.g. a customer needs a missing feature urgently (you just swap out the lib at link time for the official one until you have parity for that particular feature).

So I guess if @dgsantana made the effort to make a C++ wrapper for the same reason it would make sense to stick to the original C++ API. For new users from other languages a C wrapper would probably be easier to work with though ... Two different use cases.

dgsantana commented 4 years ago

I would say, its probably easier to write a C header first and then a C++ wrapper, there is also the need for a Rust C-ABI export crate, which will be an unsafe part. I already have a C-ABI Rust library (adapted to our use case), since I need to interop with C# and Python code.

johannesvollmer commented 4 years ago

The API of exrs is made for Rust, which will make it difficult to adapt to C or C++. Especially the use of enums with data, closures and traits will reduce the quality of the C API that you would have to write, if I'd have to guess

Maybe, for the common case of RGBA images this may be less dramatic, but for the full API this might be rather unergonomic.

If you want to try that, I won't stop you! I myself however will rather spend my time to add the missing features

johannesvollmer commented 4 years ago

For a C/C++ wrapper I'd recommend not to replicate the reference API, as it contains a lot of noise and backwards compatibility :D

dgsantana commented 4 years ago

I agree C++ official API isn't very clean. I would only offer C API for common use cases probably, I agree it would feel weird to port all the Rust API. And in the end, I'm all for "forcing" more and more the use of Rust, I know I use it for everything I can right now. But knowing that the industry (mostly VFX) moves slowly (just see the https://vfxplatform.com/ and the official libraries that should be used), it will take quite a while to adopt Rust in pipelines.

johannesvollmer commented 4 years ago

Back to autocrop! I just realized that RLE compression basically already visits each pixel once and drastically compresses images that have large areas of the same color (including all values being zero). This is already the default compression method in exrs when writing a file. Therefore, the impact of this feature will probably be visible only in uncompressed images, and of course also for runtime memory consumption when opening the image again.

Is there a smart algorithm or should we just have a plain line by line check from border to center?

johannesvollmer commented 4 years ago

Also, there must be a special behaviour where all pixels would be cropped away, as the OpenEXR artificially forbids empty images. I personally don't like the idea of inserting a single transparent pixel, but prefer instead returning an Error or something similar.

johannesvollmer commented 4 years ago

I just realized that the auto crop feature being a function has the benefit that it can be used to shrink images that you read, not only the images you write, nice

johannesvollmer commented 4 years ago

I think I found a nice way to use rayon and iterators for this, still O(n), but parallel

johannesvollmer commented 4 years ago

Ok, so I have pushed some changes, but it aint workin yet

https://github.com/johannesvollmer/exrs/commit/d1129331a83d678fab25a5ab9fced4c47355be3d

johannesvollmer commented 4 years ago

For now, it will only work with the simple image type, as the rgba image type does not store the pixels itself.

Note however that this whole simple/rgba/full image module stuff is going to change, as I'm planning to merge all those different kinds of image representations into a single API sooner or later

virtualritz commented 4 years ago

Could remove_excess be added as a flag to WriteOptions so it can be set when using e.g. ImageInfo::write_pixels_to_file()?

On that note: I think it would be better to have the name reflect what this does. Namely shrink-wrapping the data window. The current name suggests data is being removed/modified.

Maybe auto_crop is also not the worst since it's a standard term the VFX industry has settled on for this functionality. After all they are the folks who came up with EXR in the 1st place. Just google autocrop exr.

3Delight, Arnold, Houdini, Natron, Nuke, OpenImageIO, Redshift, RenderMan, etc. all use the term autocrop for this functionality. Even outliers use part of the term. VRay uses exr auto data window (still has auto in it). Royal Render uses EXR crop (has crop in it).

virtualritz commented 4 years ago

On my suggestion to replicate the original C++ API for the wrapper: After looking at namely API again I agree with @johannesvollmer. Bad suggestion of me. Kindly disregard. :)

johannesvollmer commented 4 years ago

The current name suggests data is being removed/modified

Yeah, this is exactly the case lol. It actually removes the pixels from your image as you call it, resulting in less memory being consumed. This is the case because there is intentionally no separate hidden data window. The size of the pixels array directly corresponds to the data window size. Therefore, the pixel data must actually be removed for proper cropping. Adjusting the data window without removing the pixels will result in an invalid image.

Could remove_excess be added as a flag

It's currently not a flag because I am under the impression that auto crop is only one of the many useful features that will be placed in a separate higher-level image processing crate at a later point in time. The goal of exrs is first and foremost to encode and decode data, and not preparing or converting image data. The flags are only intended for file input and output.

Furthermore, I want this auto crop functionality to be modular and replaceable. Using a flag does not encourage people to swap this algorithm for their own crop algorithm if necessary.

I can see only one feature that may be implemented into the Write Options: Specifying a cropping rectangle in the flags that crops the image pixels as they are written.

johannesvollmer commented 4 years ago

depth channels where it's those bigger or equal to the max value of the type (float or half)

This is currently not done. How would exrs know which channels are used as depth buffers? This is one of the reasons why I encourage you to write your own cropping algorithms. A different application might have inverted masks that should discarded where the value is 1.0, should exrs also do this one?

virtualritz commented 4 years ago

Hmm, I am just thinking that this is something that could be added for the fill pixels closure if you want to to provide the flexibility of customizing this. The closure could be made an enum so there would be two types of closures. One returning direct pixel data and one returning it wrapped in a Result which when None would mean that the pixel is empty.

I honestly think thought that it is very unlikely people will need this customization. I would add it when someone opens an issue for it. But that's just me looking at how people use stuff in VFX in the 20 years I worked in that industry. :P

virtualritz commented 4 years ago

For the depth based cropping. As I described it is how it is implemented in two renderers I have seen the source code of. There is a depth data type called 'Z' that is used by the renderer to identify such channels. All that is needed is a similar flag in the crate.

One of the two renderers I have seen this in source code form is 3Delight's EXR display driver. Which has been used on thousands of feature film and series productions since 1999. People in VFX, particularly TDs, ask for a lot of shit. Just because. Most of which ends up being not implemented because ... feature creep and there's usually a workaround with existing functionality.

I spoke to the developers about this. Curiously they said no one ever asked for customizing cropping behavior or the way depth is used for this in the EXR autocrop feature in 3Delight. For your consideration.

I wouldn't worry. Implement it as I described (I have permission to paste the source snippet from 3Delight, if needed).

Cross the bridge of adding support for inverted or whatever when you get to it. Aka: when someone opens an issue. Or even better: a PR where they already added what they need. ;)

This is extremely handy but trivial functionality, implementation wise. No need to over-engineer it.

johannesvollmer commented 4 years ago

Sounds reasonable. I still think this deserves its own crate though, which allows for separate release cycles and dependencies. Maybe in one single repository. This could easily be done together with the task where we extract the image data

johannesvollmer commented 4 years ago

I want to make those changes to the API first (#89), because this feature depends on that. Therefore, I want to delay all further changes until the API in the merge-images branch is rewritten

virtualritz commented 4 years ago

Congrats to releasing 0.9.0. :)

I'm not sure how to use autocrop. I saw the Image::remove_excess() but I use ImageInfo::write_pixels_to_file(). So how would I use the autocrop functionality in that case? Regarding remove_excess: I would have never found this with this name. Maybe at least mention autocrop in the docs so a "normal" user of this crate with a VFX background has a chance to discover this. :P

Docs say this removes empty layers entirely. Is this safe? I mean what happens when I have a spaceship leaving the picture and coming back after 10 frames. What happens in an app like Nuke when an EXR sequence starts missing a layer mid-sequence? Just asking btw. – I have no idea.

johannesvollmer commented 4 years ago

The current version is unfortunately still very limited - it is only supported in the 'simple image', not with the rgba image. I'm currently still working on a rewrite of the data structures, which will simply the task of porting that algorithm to all data representations :)

Also, I'm convinced that the auto crop feature deserves an example file.

As a programmer, it just felt wrong to insert a transparent 1x1 px layer into the image file, which is why the layer will be removed - if this turns out to be a problem, we will need to insert the arbitrary layer

johannesvollmer commented 4 years ago

I don't have nuke unfortunately, so I can't test that

johannesvollmer commented 3 years ago

@virtualritz hi! the data structure rework is almost finished, so I'm currently working on the Auto Crop feature again. Just wanted to inform you that you might want to watch out for the next release :)

johannesvollmer commented 3 years ago

Also, I improved the algorithm, it's a lot less brute force an more careful now, yielding a O(1) performance for fully opaque images

virtualritz commented 3 years ago

Super cool. I'm about to release an update to two crates with some examples and a blog post. The examples also use exr.

johannesvollmer commented 3 years ago

It's there, yay

Here is a cropping example for rgb images

Do you think the examples are enough to see how the new API works, or would you say an introduction is required for using it? As it's more complex than the old API, it might be a little too much to rely solely on examples

johannesvollmer commented 3 years ago

@virtualritz I'm interested in the Blog post, would you like to share it here when you're ready?

virtualritz commented 3 years ago

would you like to share it here when you're ready?

For sure.

In the meantime I have some update to my nsi crate that makes getting pixels fun. Including support for rendering to Jupyter notebooks running a Rust kernel. And the output example of the nsi crate uses EXR.