Revise OpenEXR sorting of channel names into canonical order (#2595)

Brief background: OpenEXR sorts channel names alphabetically. Most
formats dont allow channel naming at all, let alone forcing a
particular order contrary to the order the user tries to write it to
the file. OIIO promises that if there is RGB, they will be first and
in that order, next will be alpha if present, then depth, then
everything else.

So our exr reader will reshuffle channels into this canonical order
before presenting to the calling app. In addition to literal R, G, and
B being first, also obvious synonyms are accepted and understood to be
first ("Red", "Green", "Blue", and also "Y" in case it's just a
1-channel intensity and then maybe alpha).

A user noticed an interesting edge case: what if it's a normal map or
positional info, so the channel names are x, y, z? You want it in that
order (coincidentally alphabetical), duh. But the current heuristic
will reorder it as Y (mistaking for intensity), Z (mistaking for
depth), then X (a non-special name, so goes last). Ick!

The solution is that we need a more sophisticaed heuristic. This patch
changes the logic to first just sort by layer, then within each layer,
look to see if there are channels "x", and either "y" or "z", and if
so, then we can conclude that it means something spatial and that Y
isn't really intensity and Z isn't really depth for that particular
layer, so an alternate ordering is used. In all other cases, we
continue to use the old heuristic.

Signed-off-by: Larry Gritz <[email protected]>

src/openexr.imageio/exrinput.cpp

@@ -861,13 +861,29 @@ TypeDesc_from_ImfPixelType(Imf::PixelType ptype)
 
 
 
+// Split a full channel name into layer and suffix.
+static void
+split_name(string_view fullname, string_view& layer, string_view& suffix)
+{
+    size_t dot = fullname.find_last_of('.');
+    if (dot == string_view::npos) {
+        suffix = fullname;
+        layer  = string_view();
+    } else {
+        layer  = string_view(fullname.data(), dot + 1);
+        suffix = string_view(fullname.data() + dot + 1,
+                             fullname.size() - dot - 1);
+    }
+}
+
+
 // Used to hold channel information for sorting into canonical order
 struct ChanNameHolder {
-    string_view fullname;
+    string_view fullname;    // layer.suffix
+    string_view layer;       // just layer
+    string_view suffix;      // just suffix (or the fillname, if no layer)
     int exr_channel_number;  // channel index in the exr (sorted by name)
-    string_view layer;
-    string_view suffix;
-    int special_index;
+    int special_index;       // sort order for special reserved names
     Imf::PixelType exr_data_type;
     TypeDesc datatype;
     int xSampling;
@@ -876,31 +892,52 @@ struct ChanNameHolder {
     ChanNameHolder(string_view fullname, int n, const Imf::Channel& exrchan)
         : fullname(fullname)
         , exr_channel_number(n)
+        , special_index(10000)
         , exr_data_type(exrchan.type)
         , datatype(TypeDesc_from_ImfPixelType(exrchan.type))
         , xSampling(exrchan.xSampling)
         , ySampling(exrchan.ySampling)
     {
-        size_t dot = fullname.find_last_of('.');
-        if (dot == string_view::npos) {
-            suffix = fullname;
-        } else {
-            layer  = string_view(fullname.data(), dot + 1);
-            suffix = string_view(fullname.data() + dot + 1,
-                                 fullname.size() - dot - 1);
-        }
+        split_name(fullname, layer, suffix);
+    }
+
+    // Compute canoninical channel list sort priority
+    void compute_special_index()
+    {
         static const char* special[]
             = { "R",    "Red",  "G",  "Green", "B",     "Blue",  "Y",
                 "real", "imag", "A",  "Alpha", "AR",    "RA",    "AG",
                 "GA",   "AB",   "BA", "Z",     "Depth", "Zback", nullptr };
-        special_index = 10000;
         for (int i = 0; special[i]; ++i)
             if (Strutil::iequals(suffix, special[i])) {
                 special_index = i;
-                break;
+                return;
             }
     }
 
+    // Compute alternate channel sort priority for layers that contain
+    // x,y,z.
+    void compute_special_index_xyz()
+    {
+        static const char* special[]
+            = { "R",    "Red",  "G",  "Green", "B", "Blue", /* "Y", */
+                "X",  "Y",  "Z", "real", "imag",
+                "A",  "Alpha", "AR", "RA", "AG",
+                "GA",   "AB",   "BA", "Depth", "Zback", nullptr };
+        for (int i = 0; special[i]; ++i)
+            if (Strutil::iequals(suffix, special[i])) {
+                special_index = i;
+                return;
+            }
+    }
+
+    // Partial sort on layer only
+    static bool compare_layer(const ChanNameHolder& a, const ChanNameHolder& b)
+    {
+        return (a.layer < b.layer);
+    }
+
+    // Full sort on layer name, special index, suffix
     static bool compare_cnh(const ChanNameHolder& a, const ChanNameHolder& b)
     {
         if (a.layer < b.layer)
@@ -916,6 +953,18 @@ struct ChanNameHolder {
     }
 };
 
+
+// Is the channel name (suffix only) in the list?
+static bool
+suffixfound(string_view name, cspan<ChanNameHolder> chans)
+{
+    for (auto& c : chans)
+        if (Strutil::iequals(name, c.suffix))
+            return true;
+    return false;
+}
+
+
 }  // namespace
 
 
@@ -934,9 +983,43 @@ OpenEXRInput::PartInfo::query_channels(OpenEXRInput* in,
     for (auto ci = channels.begin(); ci != channels.end(); ++c, ++ci)
         cnh.emplace_back(ci.name(), c, ci.channel());
     spec.nchannels = int(cnh.size());
-    std::sort(cnh.begin(), cnh.end(), ChanNameHolder::compare_cnh);
+
+    // First, do a partial sort by layername. EXR should already be in that
+    // order, but take no chances.
+    std::sort(cnh.begin(), cnh.end(), ChanNameHolder::compare_layer);
+
+    // Now, within each layer, sort by channel name
+    for (auto layerbegin = cnh.begin(); layerbegin != cnh.end();) {
+        // Identify the subrange that comprises a layer
+        auto layerend = layerbegin + 1;
+        while (layerend != cnh.end() && layerbegin->layer == layerend->layer)
+            ++layerend;
+
+        span<ChanNameHolder> layerspan(&(*layerbegin), layerend - layerbegin);
+        // Strutil::printf("layerspan:\n");
+        // for (auto& c : layerspan)
+        //     Strutil::printf("  %s = %s . %s\n", c.fullname, c.layer, c.suffix);
+        if (suffixfound("X", layerspan) && (suffixfound("Y", layerspan)
+                                            || suffixfound("Z", layerspan))) {
+            // If "X", and at least one of "Y" and "Z", are found among the
+            // channel names of this layer, it must encode some kind of
+            // position or normal. The usual sort order will give a weird
+            // result. Choose a different sort order to reflect this.
+            for (auto& ch : layerspan)
+                ch.compute_special_index_xyz();
+        } else {
+            // Use the usual sort order.
+            for (auto& ch : layerspan)
+                ch.compute_special_index();
+        }
+        std::sort(layerbegin, layerend, ChanNameHolder::compare_cnh);
+
+        layerbegin = layerend;  // next set of layers
+    }
+
     // Now we should have cnh sorted into the order that we want to present
     // to the OIIO client.
+
     spec.format         = TypeDesc::UNKNOWN;
     bool all_one_format = true;
     for (int c = 0; c < spec.nchannels; ++c) {

testsuite/openexr-suite/ref/out.txt

@@ -276,3 +276,21 @@ Reading ../../../../../openexr-images/Tiles/Spirals.exr
     oiio:subimages: 1
 Comparing "../../../../../openexr-images/Tiles/Spirals.exr" and "Spirals.exr"
 PASS
+Reading ../../../../../openexr-images/Beachball/singlepart.0001.exr
+../../../../../openexr-images/Beachball/singlepart.0001.exr :  911 x  876, 20 channel, half openexr
+    SHA-1: C946B18AE0C85563D82394D763541A9F61767C27
+    channel list: R, G, B, A, Z, disparityL.x, disparityL.y, disparityR.x, disparityR.y, forward.left.u, forward.left.v, forward.right.u, forward.right.v, left.R, left.G, left.B, left.A, left.Z, whitebarmask.left.mask, whitebarmask.right.mask
+    pixel data origin: x=654, y=245
+    full/display size: 2048 x 1556
+    full/display origin: 0, 0
+    compression: "zips"
+    multiView: "right", "left"
+    name: "rgba"
+    PixelAspectRatio: 1
+    screenWindowCenter: 0, 0
+    screenWindowWidth: 1
+    oiio:ColorSpace: "Linear"
+    oiio:subimagename: "rgba"
+    oiio:subimages: 1
+Comparing "../../../../../openexr-images/Beachball/singlepart.0001.exr" and "singlepart.0001.exr"
+PASS

testsuite/openexr-suite/run.py

@@ -39,3 +39,10 @@
 files = [ "GoldenGate.exr", "Ocean.exr", "Spirals.exr" ]
 for f in files:
     command += rw_command (imagedir, f)
+
+
+# Check a complicated channel and layer ordering example
+imagedir = OIIO_TESTSUITE_IMAGEDIR + "/Beachball"
+files = [ "singlepart.0001.exr" ]
+for f in files:
+    command += rw_command (imagedir, f)