WIP: New C core #870

kdt3rd · 2020-11-23T10:05:55Z

This starts to implement a new C-based "core" for OpenEXR.

Current status:

most things are being written with doxygen-style comments, although some functions will need to be expanded upon and a cleanup pass made once we like the style to be made consistent.
base, lighter-weight attributes initial implementation sufficient for use, initial unit tests written
core file I/O / error reporting / memory allocation defined
initial file reading / parsing done, unit tests are in progress
- only enough of the compression techniques to test have been ported, b44, pxr24, etc. are not difficult once we are happy with the core design, but I want to make that system a bit more ... replaceable
writing is largely incomplete at this stage
NB: This does not replace any C++ code at all currently (in fact, the current performance test relies on that!)

This is a massive PR, if it would be easier to re-construct it as individual layers, I can probably do that. But what I'm looking for is feedback on whether the "namespace" mechanism I've implemented degrades from the readability of the code too much to be worth it, or if we should eliminate that before there is too much inertia. Also looking for feedback on how I've chosen to break up the headers into component chunks - I dislike massive files. It is also written in a slightly different style, favoring underscores and such to try to differentiate it more from the C++.

lgritz · 2020-11-23T20:07:09Z

src/lib/OpenEXRCore/openexr_conf.h

+/** Used to declare / use types */
+#define EXR_TYPE(n) exr_ ## n
+/** Used to declare / use enums */
+#define EXR_DEF(n) EXR_ ## n
+/** Used to declare / call functions */
+#define EXR_FUN(n) exr_ ## n
+


Would it be helpful to have

#define EXR_TYPE(n) exr_ ## n ## _t

to more easily distinguish types from functions and be sure they won't clash?

I'm a little leery of these macros. Maybe it's nothing to worry about, but it makes me immediately wonder if they will throw off modern editor's (such as Sublime, VSCode, CLion, etc.) ability to correctly syntax highlight, refactor, connect declarations and usage of functions, show information about a function's declarations and parameter when you hover over a call site, etc.

sure, the _t is a good point, not sure why I didn't do that, I obviously subconsciously was thinking it with the separation.

But these macros are the biggest discussion I'd like to have - now is the time to decide whether to use it or not. And I think in libraries like jpeg where you can have 8-bit and 12-bit variants present, it makes sense. I was initially thinking people might like to customize for their facility, but I don't think this is that big a deal at this level, because the file format is the file format, and we should do the symbol versioning thing to manage different versions.

I'm curious about Larry's point on the IDEs ~ do the common IDEs recognize the expanded function names such that "go to definition/declaration" works? As a sanity check, GLEW's macros tend to work in IDEs, so it might be interesting to see if an editor can eat GLEW, can it eat these macros as well?

lgritz · 2020-11-23T20:27:57Z

src/lib/OpenEXRCore/openexr_decompress.c

+        d = _mm_add_epi8(d, _mm_slli_si128(d, 1));
+        d = _mm_add_epi8(d, _mm_slli_si128(d, 2));
+        d = _mm_add_epi8(d, _mm_slli_si128(d, 4));
+        d = _mm_add_epi8(d, _mm_slli_si128(d, 8));
+        d = _mm_add_epi8(d, vPrev);


Maybe this isn't the time and place to bring this up, but perhaps it's about time to kick these direct intrinsics to the curb and instead use wrappers with understandable names and that, inside them, have implementations for SSE, ARM, generic C fallback, etc. As an example, see OIIO's simd.h.

Yes, I know that my C++ classes are not a good solution for this C code, but the point is that we could make C wrappers these 4-wide ops, with friendly and self-documenting names and a central place to have alternate implementations -- such as we're going to want to get this running fast on the new Macs with ARM processors.

good point, I'll have a look

lgritz · 2020-11-23T20:42:39Z

src/lib/OpenEXRCore/openexr_priv_memory.h

+
+#include "openexr_base.h"
+
+extern void *priv_alloc( size_t bytes ) __attribute__((malloc));


The attribute is surely gcc/clang only and won't work on MSVS, right?

oh, quite possibly, I don't have a windows machine at home, so I end up using CI to discover these :)

MSVC doesn't have an equivalent, I think. Am I reading the gcc docs right, that the attribute says "a returned pointer has no objects embedded in it, so a realloc is safe? I'm curious about what kind of optimizations it enables gcc to do in response? I think this attribute is quite gcc specific and should be restricted as such. As of apple clang 12, clang has started to lose attributes that were implemented for compatibility. Ultimately Apple will upstream these, and clang will diverge from gcc in the end.

my understanding is that it is two fold: one, it allows the optimizer know that the memory is not aliased, so it doesn't have to reload from other non-aliased stores, which realloc can not not.

However, the real reason I attached it was that I was under the impression that the various static analyzers would use this to report further down where memory was requested that the memory was not freed, so it would report the call site, not the routine

ThiagoIze · 2021-01-14T22:20:47Z

I get having a C based API in libraries, but why should the implementation itself be C instead of C++? If there are parts of C++ that aren't wanted, such as exceptions, one can always avoid them, so it's not clear to me what forcing C accomplishes. Since it's not apparent to me, I suppose it would be worth having that outlined in case someone in the future wants to port this to C++.

kthurston · 2021-01-22T21:00:10Z

I get having a C based API in libraries, but why should the implementation itself be C instead of C++? If there are parts of C++ that aren't wanted, such as exceptions, one can always avoid them, so it's not clear to me what forcing C accomplishes. Since it's not apparent to me, I suppose it would be worth having that outlined in case someone in the future wants to port this to C++.

@ThiagoIze - the idea is that this is C only because then it can be used in context where a c++ library is not desired (which has been requested numerous times), and I believe is a hindrance to adoption in things like the ffmpeg project, instead they have implemented a subset of the file format, which is problematic. Further, the roadmap (which is certainly not clear from what has been done so far) is to then re-implement the existing C++ library in terms of this library, although some API will have to be added to do so in a thread-safe manner. So C++ is not going away, just not part of this subset of the overall library. So, if you prefer C++, please continue to use the C++ API, as that will continue to be developed, and in future versions will gain the advantages this C layer was written to address (thread safety where multiple threads can read from the same file being the primary one)

kthurston · 2021-01-22T21:02:47Z

I have removed the pseudo namespace layer. If anyone was following along, I used the following script to convert the code, then touched up a couple of things in the tests which were working by luck only.

#! /bin/bash

echo 's/EXR_TYPE[ ]?[(][ ]?file[ ]?[)]/_priv_exr_file_t/g' > /tmp/fixup.$$
echo 's/EXR_TYPE[ ]?[(][ ]?FILE[ ]?[)]/exr_file_t/g' >> /tmp/fixup.$$
echo 's/EXR_TYPE[ ]?[(][ ]?([A-Za-z_][A-Za-z_0-9]*)[ ]?[)]/exr_\1_t/g' >> /tmp/fixup.$$
echo 's/EXR_DEF[ ]?[(][ ]?([A-Za-z_][A-Za-z_0-9]*)[ ]?[)]/EXR_\1/g' >> /tmp/fixup.$$
echo 's/EXR_FUN[ ]?[(][ ]?([A-Za-z_][A-Za-z_0-9]*)[ ]?[)]/exr_\1/g' >> /tmp/fixup.$$

find src/test/OpenEXRCoreTest -type f -exec sed -r -i -f /tmp/fixup.$$ {} \;
find src/lib/OpenEXRCore -type f -exec sed -r -i -f /tmp/fixup.$$ {} \;
find src/bin/exrinfo -type f -exec sed -r -i -f /tmp/fixup.$$ {} \;

rm /tmp/fixup.$$

meshula · 2021-01-23T02:19:09Z

Removal of the pseudo-namespaces has made the code infinitely easier to read.

src/lib/OpenEXRCore/openexr_priv_structs.h

lgritz · 2021-03-30T05:39:57Z

Hey, can I make a request? Somewhere in one of the main headers (maybe OpenEXRConfig.h) can you please

#define HAS_OPENEXR_CORE 1

This will make it easier for downstream packages to know if they're dealing with a build of OpenEXR that has the new headers and C core functions.

kdt3rd · 2021-03-30T09:08:32Z

This will make it easier for downstream packages to know if they're dealing with a build of OpenEXR that has the new headers and C core functions.

of course, we should do something like this. Although I don't actually like the name "core" - but I guess I will have to see how the rest of this goes for a little while

meshula · 2021-03-30T16:43:42Z

src/lib/OpenEXRCore/opaque.c

-    if (u)
+    if (!u) return pctxt->standard_error (ctxt, EXR_ERR_INVALID_ARGUMENT);
+
+    /* TODO: do we care if the incoming unpacked data is null? */


I think it's correct to reject null.

meshula · 2021-03-30T16:45:01Z

src/lib/OpenEXRCore/decompress.c

@@ -154,13 +154,13 @@ undozip (
    void*                               out,
    size_t                              outsz)
 {
-    uLongf outSize = outsz;
+    uLongf outSize = (uLongf)outsz;


since this a do-over, now might be a time to replace uLongF with a sized type :) Every time I see uLongf, I have to go grep for what it is!

meshula · 2021-03-30T16:48:11Z

src/lib/OpenEXRCore/string.c

@@ -120,10 +120,17 @@ exr_attr_string_create_with_length (
        /* someone might pass in a string shorter than requested length (or longer) */
        if (len > 0)
        {
+#ifdef _MSC_VER


In my warnings cleanup elsewhere in OpenEXR, I'm just suppressing 4996 for the entire file. I can't think of a single case where spot-applying 4996 might be helpful. The warnings are just a constant reminder that Microsoft had an idea that never took hold in the programming community, outside of the windows centric community, anyway.

meshula

some small opinions