Add dwa support to core #1402

kdt3rd · 2023-05-07T11:30:04Z

Seems to work, although am noticing a small number of ULPs difference in the dct coding between C/C++ for some numbers, can't tell if I should be worried, or if it's just different math ops ordering causing slightly different rounding

This includes a few other (internal) refactors and fixes noticed along the way

cary-ilm

I noted some place where you left "throw Iex" commented out from the original cpp, do you want to clean that up or is it helpful to leave in?

Also, I may have missed some discussion, but you seem to have merged in @meshula's commits from #1385, was that intentional? That PR hasn't been committed yet, should it be closed in favor of this?

Otherwise, I've looked this over and it looks good to me, thanks!

cary-ilm · 2023-05-13T17:45:07Z

src/lib/OpenEXRCore/internal_dwa_classifier.h

+
+    if (*size <= 3) return EXR_ERR_CORRUPT_CHUNK;
+
+    //throw IEX_NAMESPACE::InputExc ("Error uncompressing DWA data"


I left them to note the error string, intending to produce a similar message via report error at some point...

cary-ilm · 2023-05-13T17:49:04Z

src/lib/OpenEXRCore/internal_dwa_classifier.h

+    if (out->_cscIdx < -1 || out->_cscIdx >= 3)
+    {
+        return EXR_ERR_CORRUPT_CHUNK;
+        //throw IEX_NAMESPACE::InputExc ("Error uncompressing DWA data"


dead, and a few below as well

cary-ilm · 2023-05-13T17:52:47Z

src/lib/OpenEXRCore/internal_dwa_compressor.h

+    // the C++ classes used to have one buffer size for compress / uncompress
+    // but here we want to do zero-ish copy...
+    uint8_t* outBufferEnd = me->_decode->unpacked_buffer;
+    //if (outBufferSize > me->_decode->unpacked_alloc_size)


cary-ilm · 2023-05-13T17:53:24Z

src/lib/OpenEXRCore/internal_dwa_compressor.h

+    if (version > 2)
+    {
+        return EXR_ERR_BAD_CHUNK_LEADER;
+        //throw IEX_NAMESPACE::InputExc (


meshula · 2023-05-13T18:55:04Z

I didn't expect that my PR would chain on to Kimball's PR like that.

I'll try closing my PR in expectation of reopening versus the main repo.

meshula · 2023-05-13T19:00:33Z

Ok, closing my PR didn't do it, I guess those changes are actually committed into Kimball's tree. I have follow up work to those commits which I can open on the main repo. after this is landed. My preference would be to not land openexr-c.c and h though as they are not ready for review or distribution. I have follow up work to complete what's in that interface.

cary-ilm · 2023-05-13T20:07:03Z

We'd like to include the dwa support in a 3.1.8 release, but the single-file openexr-c.c seems better suited to a 3.2/4.0 release, doesn't it? @kdt3rd , don't you think you should back those commits out of this PR?

kdt3rd · 2023-05-13T23:36:51Z

@cary-ilm yes, I did not actually accept Nick's PR on my branch, it's interesting that it is chaining up to here... ah, the vagaries of github PRs...

meshula · 2023-05-14T00:55:24Z

Well it's even weirder because my PR was on the main repo, not yours! I have a different PR on your repo, but that has commits not appearing here. I've closed both of them in hopes of untangling things.

kdt3rd · 2023-05-14T20:52:17Z

fyi, have confirmed Nick's commits are not in my local branch at all. I need to rebase onto a branch that removes the old mac os build that keeps timing out, so that should process should re-clean the history. I have one pending fix to push in the dwa support, and another difference in the decoder side to track down, then I think this will be ready to go