Document steps to add a new compression method in ImfCompression.h

[pleprince]

Suggested by @cary-ilm

[pleprince]

Hi @peterhillman. Would you be kind enough to review ? Cheers.

[peterhillman]

Very clear and complete.
I would perhaps have put this in ImfCompressor.cpp, as most people viewing this header file are only looking up values of the enum or the function declarations. There's little point having the documentation in installs of the OpenEXR library, since you can only add compression types by modifying the source. A one-line comment in ImfCompression.h could point readers at those details.

It would also be better for compression types to have a single implementation in the Core library, with the C++ library then calling low level functionality in Core to implement the compression/decompression

[pleprince]

It would also be better for compression types to have a single implementation in the Core library, with the C++ library then calling low level functionality in Core to implement the compression/decompression.

I totally agree. I didn't really know about the C API at the time and it would be better to refactor the compressors in the future.

[peterhillman]

Perhaps switch the order round, so step 2 is the C implementation and step 3 the C++ integration, and add a suggestion in the C++ integration section about having it call the C implementation? If the entire implementation is just compressWithThirdPartyLibrary(data,size) then there's not much point in having the C++ code call the C code, but if its 50,000 lines of clever wavelet code, then you shouldn't write it twice.

[cary-ilm]

This looks good to me, too, thanks! I see you added Zstd support to OpenEXRCore in #1604, which is great, and the compressWithThirdPartyLibrary amounts to 115 lines, probably right at the threshold of what you'd want to duplicate. I agree that listing the C API first helps make sure that's not an afterthought.

[cary-ilm]

@pleprince, we're happy to accept this, but with @peterhillman's suggestion of moving the comment to ImfCompression.cpp since it's not relevant to the external API, and switching the order of the steps as described above. Thanks!

[pleprince]

Hi CarySorry I was busy with something else. I will do that tomorrow.Cheers--PhilippeOn 18 Mar 2024, at 19:20, Cary Phillips ***@***.***> wrote: @pleprince, we're happy to accept this, but with @peterhillman's suggestion of moving the comment to ImfCompression.cpp since it's not relevant to the external API, and switching the order of the steps as described above. Thanks! —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[pleprince]

Sorry for the delay. I hope this will be ok now. Cheers

[cary-ilm]

Thanks. Sorry to be a stickler here, but our project coding standards are to format comments with each line prefixed with //, rather than /* ... */. Look at any other file in the library as a reference. Would you mind reformat using //?

[pleprince]

no worries.