search

RenderKit / oidn

Intel® Open Image Denoise library
https://www.openimagedenoise.org/
Apache License 2.0
1.77k stars 164 forks source link

Crash when providing Normal but no Albedo buffer #9

Closed Dade916 closed 5 years ago

Dade916 commented 5 years ago

Oidn throws a segmentation fault when I provide a normal buffer without an albedo one. It is a small detail but I assume it should report an error and to not crash.

atafra commented 5 years ago

Could you please provide some details on how you call OIDN (e.g. a code snippet), which compiler and OS you use, etc.? This is very strange because it does check for this case and should set an error. I've never seen it crash so far. Thanks!

Dade916 commented 5 years ago

I'm sorry, the error was on my side, once fixed, Oidn started to correctly report the error:

[LuxCore][5.891] IntelOIDNPlugin error: unsupported combination of input features

The error was in the definition of the normal buffer so it may have affected all my tests (including the comparison on Twitter with Bayesian Collaborative Denoise). Going to redo the tests.

robpieke commented 2 years ago

Sorry for breathing life into an old issue with a question, but is there a plan to remove the "if you use N you have to also use albedo" constraint? Is this documented?

atafra commented 2 years ago

There are no plans yet because it is not an artificial constraint: for all combinations of input features we need separate trained network models, which is quite expensive and increases the size of the library as well. If you provide your own trained model, it should work even in the current version.

We haven't included a normal only model yet because typically albedo provides a much higher quality improvement than normals, and in most renderers if normals are available, albedos are too. Do you have a use case where you have normals but albedo is not available?

robpieke commented 2 years ago

Wow, thanks for the quick reply!

Do you have a use case where you have normals but albedo is not available?

Only in that I'm providing a general tool to users and they might have this input set. So I find myself debating whether my tool should check for this specific input and provide a customised user-friendly error, or if I should just try commit() and rely on the output of getError() (which is perhaps a bit cryptic for someone who knows nothing about the tech).

I don't want to introduce logic that I'll need to remember to remove again, but it sounds like there are no immediate plans to support N without albedo in the shipped models and any such custom logic probably won't be immediately throw-away.

Thanks again!

atafra commented 2 years ago

I think adding custom logic for this would be indeed quite future-proof but we'll consider making the error messages a bit more specific.