search

Twinklebear / oidn-rs

Rust bindings to Intel's OpenImageDenoise Library
MIT License
24 stars 12 forks source link

Updated API for in-place filtering, updated examples, updated README – #2. #12

Closed virtualritz closed 3 years ago

virtualritz commented 3 years ago

Same as before plus requested fixes.

Twinklebear commented 3 years ago

Thanks @virtualritz , this looks good! For the first filter run overhead, I'm thinking to do something like a builder API, where after you set your color/albedo/normal parameters you'd call commit on the filter to get back a CommittedFilter that you can then call filter on. But, I do want to check on the first launch vs. subsequent run filter cost first if that's actually a problem.

Twinklebear commented 3 years ago

This essentially reverts the change made for #5 to allow not having to keep the buffers around and have the extra lifetime, but if we want to do that for the committed filter thing anyways, we'd want an API like what's in this PR where we keep them around.

virtualritz commented 3 years ago

This essentially reverts the change made for #5 to allow not having to keep the buffers around and have the extra lifetime, but if we want to do that for the committed filter thing anyways, we'd want an API like what's in this PR where we keep them around.

Yeah, I was originally 2nding that request in #5. But if I look at real world use of this API – I still have to see proof that keeping the filter around gives a speed advantage. This is used to filter offline rendered images. The fastest I can get an image of a reasonably complex scene from a path tracer is a little under a second. That's with 1spp. Not common, I'd say. So if the filter takes 30ms more of 'startup' time, I don't think this matters.

Twinklebear commented 3 years ago

That was my thought as well, since OIDN is typically for single final-frame denoising and not as much for real-time rendering, the overhead may not be such a big deal to be worth a lot of reworking of the API. I'm not sure if it'd make the usage a bit clearer, or more annoying by introducing a second object and "commit" step