Skip to content

Commit

Permalink
fix(oiiotool): Overhaul and fix bugs in mixed-channel propogation (#4127
Browse files Browse the repository at this point in the history 
)

Remember that ImageBuf only stores one pixel data type per buffer, but
of course they may have differing-per-channel types in image files. So
how does oiiotool know what to write? There is a series of heuristics:
-d is for explicit user control, if not, then try to deduce it from the
input files.

A bedrock precept is that `oiiotool in.exr -o out.exr` ought to write
the same channel types that it read in.

The logic to implement this with the various image operations and
possible overrides can be hard to get right. Indeed, I did not get it
right. It made mistakes even in simple cases like the above. And the
code was complicated and hard to understand, as well.

This PR rewrites a couple functions that implement this chunk of this
logic, both correcting several errors as well as (I hope) being clearer
and better commented so that we can more easily reason about it and
modify it in the future.

I also added some additional tests to testsuite/perchannel to verify
some of the cases that were previously failing.

Signed-off-by: Larry Gritz <[email protected]>
  • Loading branch information
@lgritz
lgritz authored Feb 8, 2024
1 parent 94f1c49 commit 46d8c82
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 51 deletions.
125 changes: 74 additions & 51 deletions src/oiiotool/oiiotool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ Oiiotool::remember_input_channelformats(ImageRecRef img)
if (input_channelformats[subchname] == "")
input_channelformats[subchname] = chtypename;
} else {
if (input_channelformats[chname] != "")
if (input_channelformats[chname] == "")
input_channelformats[chname] = chtypename;
}
}
Expand Down Expand Up @@ -764,39 +764,23 @@ set_output_dataformat(ImageSpec& spec, TypeDesc format,
std::map<std::string, std::string>& channelformats,
int bitdepth)
{
// Account for default requested format
if (format != TypeUnknown)
spec.format = format;
if (bitdepth)
spec.attribute("oiio:BitsPerSample", bitdepth);
else
spec.erase_attribute("oiio:BitsPerSample");

// See if there's a recommended format for this subimage
std::string subimagename = spec["oiio:subimagename"];
if (format == TypeUnknown && subimagename.size()) {
auto key = Strutil::fmt::format("{}.*", subimagename);
if (channelformats[key] != "")
spec.format = TypeDesc(channelformats[key]);
}

// Honor any per-channel requests
if (channelformats.size()) {
spec.channelformats.clear();
spec.channelformats.resize(spec.nchannels, spec.format);
spec.channelformats.resize(spec.nchannels, spec.format);
if (!channelformats.empty()) {
std::string subimagename = spec["oiio:subimagename"];
for (int c = 0; c < spec.nchannels; ++c) {
std::string chname = spec.channel_name(c);
auto subchname = Strutil::fmt::format("{}.{}", subimagename,
chname);
if (channelformats[subchname] != "" && subimagename.size())
spec.channelformats[c] = TypeDesc(channelformats[subchname]);
TypeDesc chtype = spec.channelformat(c);
if (subimagename.size() && channelformats[subchname] != "")
chtype = TypeDesc(channelformats[subchname]);
else if (channelformats[chname] != "")
spec.channelformats[c] = TypeDesc(channelformats[chname]);
else
spec.channelformats[c] = spec.format;
chtype = TypeDesc(channelformats[chname]);
if (chtype != TypeUnknown)
spec.channelformats[c] = chtype;
}
} else {
spec.channelformats.clear();
}

// Eliminate the per-channel formats if they are all the same.
Expand All @@ -809,6 +793,11 @@ set_output_dataformat(ImageSpec& spec, TypeDesc format,
spec.channelformats.clear();
}
}

if (bitdepth)
spec.attribute("oiio:BitsPerSample", bitdepth);
else
spec.erase_attribute("oiio:BitsPerSample");
}


Expand All @@ -835,37 +824,71 @@ adjust_output_options(string_view filename, ImageSpec& spec,
// format as the input, or else she would have said so).
// * Otherwise, just write the buffer's format, regardless of how it got
// that way.
TypeDesc requested_output_dataformat = ot.output_dataformat;
auto requested_output_channelformats = ot.output_channelformats;

// spec is what we're going to use for output.

// Accumulating results here
TypeDesc requested_output_dataformat;
std::map<std::string, std::string> requested_channelformats;
int requested_output_bits = 0;

if (was_direct_read && nativespec) {
// If the image we're outputting is an unmodified direct read of a
// file, assume that we'll default to outputting the same channel
// formats it started in.
requested_output_dataformat = nativespec->format;
for (int c = 0; c < nativespec->nchannels; ++c) {
requested_channelformats[nativespec->channel_name(c)]
= nativespec->channelformat(c).c_str();
}
requested_output_bits = nativespec->get_int_attribute(
"oiio:BitsPerSample");
} else if (ot.input_dataformat != TypeUnknown) {
// If the image we're outputting is a computed or modified image, not
// a direct read, then assume that the FIRST image we read in provides
// a template for the output we want (if we ever read an image).
requested_output_dataformat = ot.input_dataformat;
requested_channelformats = ot.input_channelformats;
requested_output_bits = ot.input_bitspersample;
}

// Any "global" format requests set by -d override the above.
if (ot.output_dataformat != TypeUnknown) {
// `-d type` clears the board and imposes the request
requested_output_dataformat = ot.output_dataformat;
requested_channelformats.clear();
spec.channelformats.clear();
if (ot.output_bitspersample)
requested_output_bits = ot.output_bitspersample;
}
if (!ot.output_channelformats.empty()) {
// `-d chan=type` overrides the format for a specific channel
for (auto&& c : ot.output_channelformats)
requested_channelformats[c.first] = c.second;
}

// Any override options on the -o command itself take precedence over
// everything else.
if (fileoptions.contains("type")) {
requested_output_dataformat.fromstring(fileoptions.get_string("type"));
requested_output_channelformats.clear();
requested_channelformats.clear();
spec.channelformats.clear();
} else if (fileoptions.contains("datatype")) {
requested_output_dataformat.fromstring(
fileoptions.get_string("datatype"));
requested_output_channelformats.clear();
}
int requested_output_bits = fileoptions.get_int("bits",
ot.output_bitspersample);

if (requested_output_dataformat != TypeUnknown) {
// Requested an explicit override of datatype
set_output_dataformat(spec, requested_output_dataformat,
requested_output_channelformats,
requested_output_bits);
} else if (was_direct_read && nativespec) {
// Do nothing -- use the file's native data format
spec.channelformats = nativespec->channelformats;
set_output_dataformat(spec, nativespec->format,
requested_output_channelformats,
(*nativespec)["oiio:BitsPerSample"].get<int>());
} else if (ot.input_dataformat != TypeUnknown) {
auto mergedlist = ot.input_channelformats;
for (auto& c : requested_output_channelformats)
mergedlist[c.first] = c.second;
set_output_dataformat(spec, ot.input_dataformat, mergedlist,
ot.input_bitspersample);
requested_channelformats.clear();
spec.channelformats.clear();
}
requested_output_bits = fileoptions.get_int("bits", requested_output_bits);

// At this point, the trio of "requested" variable reflect any global or
// command requests to override the logic of what was found in the input
// files.

// Set the types in the spec
set_output_dataformat(spec, requested_output_dataformat,
requested_channelformats, requested_output_bits);


// Tiling strategy:
// * If a specific request was made for tiled or scanline output, honor
Expand Down
6 changes: 6 additions & 0 deletions testsuite/perchannel/ref/out.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,9 @@ Comparing "../openexr-images/Tiles/Spirals.exr" and "spiral1.exr"
PASS
Comparing "../openexr-images/Tiles/Spirals.exr" and "spiral2.exr"
PASS
hfhf.exr : 64 x 64, 4 channel, half/float/half/float openexr
SHA-1: 6FECD5769C727E137B7580AE3B1823B06EE6F9D9
hfhf_copy.exr : 64 x 64, 4 channel, half/float/half/float openexr
SHA-1: 6FECD5769C727E137B7580AE3B1823B06EE6F9D9
hfhf_mod.exr : 64 x 64, 4 channel, half/float/half/float openexr
SHA-1: 6FECD5769C727E137B7580AE3B1823B06EE6F9D9
11 changes: 11 additions & 0 deletions testsuite/perchannel/run.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,17 @@
command += (oiio_app("oiiotool") + image + " -o spiral2.exr ;\n")
command += diff_command (image, "spiral2.exr")

# Create a file with per-channel data types, make sure that works
command += oiiotool("--create 64x64 4 --d R=half,G=float,B=half,A=float -o hfhf.exr")
command += info_command("hfhf.exr", verbose=False)

# Make sure that read/write unmodified will preserve the channel types
command += oiiotool("hfhf.exr -o hfhf_copy.exr")
command += info_command("hfhf_copy.exr", verbose=False)

# Make sure that read/modify/write preserves the channel types of the input
command += oiiotool("hfhf.exr -mulc 0.5,0.5,0.5,0.5 -o hfhf_mod.exr")
command += info_command("hfhf_mod.exr", verbose=False)

#print "Running this command:\n" + command + "\n"

0 comments on commit 46d8c82

Please sign in to comment.