syoyo / tinydng

Header-only Tiny DNG/TIFF loader and writer in C++
MIT License
154 stars 30 forks source link

BUG: (very quick) #include <intrin.h> within the scope of the namespace tinydngwriter #30

Closed JosipML closed 1 year ago

JosipML commented 1 year ago

Please remove the include from line 107 in tiny_dng_writer.h:

ifdef _MSC_VER

include <----

...

and add it in front of the namespace tinydngwriter within the #ifdef clause as follows:

ifdef _MSC_VER

include

endif

Including it within the scope of the namespace causes issues with other includes like etc.

syoyo commented 1 year ago

https://github.com/syoyo/tinydng/blob/a5d0a0e728e51d7842007e55fbbc2ea0953c53d1/tiny_dng_writer.h#L109

Or we can simply remove clz32 implementation using intrinsic function for better portability. I guess using naiive clz32 is sufficient(no significant performance loss).

PR is much appreciated!

JosipML commented 1 year ago

I think it is a quite simple functionality as well and that the performance would remain probably very similar. I guess both solutions are good. For me only the include inside a namespace was causing issues causing unrelated errors to pop out.

syoyo commented 1 year ago

I have disabled using MSVC intrinsic functions.

If you find intrinsic functions are useful(gives noticeable performance gain), please send a PR which controls using intrinsic functions with a macro something like

TINY_DNG_USE_INTRINSIC

JosipML commented 1 year ago

Thank you! I have tested it with our code quite extensively today and it seems to be very performant. We export images in the order of ~1000 images and it is very fast from a user perspective, especially in comparison with other file types like PNG.