Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/bin/exr2aces/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ usageMessage (ostream& stream, const char* program_name, bool verbose = false)
{
stream << "Usage: " << program_name << " [options] infile outfile" << endl;

// TODO: The text below needs to be updated to reflect the actual
// SMPTE ST 2065-4 standard.

if (verbose)
stream
<< "\n"
Expand Down
11 changes: 10 additions & 1 deletion src/bin/exrstdattr/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,12 @@ usageMessage (ostream& stream, const char* program_name, bool verbose = false)
" -chromaticities f f f f f f f f\n"
" CIE xy chromaticities for the red, green\n"
" and blue primaries, and for the white point\n"
" (8 floats)\n"
" (8 floats). This attribute is superseded by\n"
" colorInteropID for most purposes.\n"
"\n"
" -colorInteropID s\n"
" Color Interop Forum ID that identifies the\n"
" color space of the RGB image data (string)\n"
"\n"
" -whiteLuminance f\n"
" white luminance, in candelas per square meter\n"
Expand Down Expand Up @@ -644,6 +649,10 @@ main (int argc, char** argv)
{
getChromaticities (attrName, argc, argv, i, part, attrs);
}
else if (!strcmp (argv[i], "-colorInteropID"))
{
getString (attrName, argc, argv, i, part, attrs);
}
else if (!strcmp (argv[i], "-whiteLuminance"))
{
getFloat (attrName, argc, argv, i, part, attrs);
Expand Down
13 changes: 13 additions & 0 deletions src/lib/OpenEXR/ImfAcesFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ acesChromaticities ()
return acesChr;
}

const std::string&
acesColorInteropID ()
{
static const std::string acesID = "lin_ap0_scene";
return acesID;
}

class AcesOutputFile::Data
{
public:
Expand Down Expand Up @@ -92,6 +99,7 @@ AcesOutputFile::AcesOutputFile (
Header newHeader = header;
addChromaticities (newHeader, acesChromaticities ());
addAdoptedNeutral (newHeader, acesChromaticities ().white);
addColorInteropID (newHeader, acesColorInteropID ());

_data->rgbaFile =
new RgbaOutputFile (name.c_str (), newHeader, rgbaChannels, numThreads);
Expand All @@ -111,6 +119,7 @@ AcesOutputFile::AcesOutputFile (
Header newHeader = header;
addChromaticities (newHeader, acesChromaticities ());
addAdoptedNeutral (newHeader, acesChromaticities ().white);
addColorInteropID (newHeader, acesColorInteropID ());

_data->rgbaFile = new RgbaOutputFile (os, header, rgbaChannels, numThreads);

Expand Down Expand Up @@ -143,6 +152,7 @@ AcesOutputFile::AcesOutputFile (

addChromaticities (newHeader, acesChromaticities ());
addAdoptedNeutral (newHeader, acesChromaticities ().white);
addColorInteropID (newHeader, acesColorInteropID ());

_data->rgbaFile =
new RgbaOutputFile (name.c_str (), newHeader, rgbaChannels, numThreads);
Expand Down Expand Up @@ -176,6 +186,7 @@ AcesOutputFile::AcesOutputFile (

addChromaticities (newHeader, acesChromaticities ());
addAdoptedNeutral (newHeader, acesChromaticities ().white);
addColorInteropID (newHeader, acesColorInteropID ());

_data->rgbaFile =
new RgbaOutputFile (name.c_str (), newHeader, rgbaChannels, numThreads);
Expand Down Expand Up @@ -312,6 +323,8 @@ AcesInputFile::Data::~Data ()
void
AcesInputFile::Data::initColorConversion ()
{
// TODO: Add support for colorInteropID.

const Header& header = rgbaFile->header ();

Chromaticities fileChr;
Expand Down
10 changes: 10 additions & 0 deletions src/lib/OpenEXR/ImfAcesFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
#ifndef INCLUDED_IMF_ACES_FILE_H
#define INCLUDED_IMF_ACES_FILE_H

// TODO: The text below needs to be updated to reflect the actual
// SMPTE ST 2065-4 standard.

//-----------------------------------------------------------------------------
//
// ACES image file I/O.
Expand Down Expand Up @@ -67,6 +70,13 @@ OPENEXR_IMF_INTERNAL_NAMESPACE_HEADER_ENTER

const Chromaticities& acesChromaticities ();

//
// The colorInteropID value for ACES2065-1, per the
// Color Interop Forum recommendation for OpenEXR files.
//

const std::string& acesColorInteropID ();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a file has acesContainerFlag and Chromaticities set to ACES, but the colorInteropID set to something else? Should the input side error, or change the colorInteropID on reading? Arguably, SMPTE 2065-4 would say that correct behavior is to ignore the colorInteropID, so it "doesn't exist" in ACES files

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImfAcesFile does not seem to do any validation of the other inputs, so I did not add any for colorInteropID either. AcesOutputFile seems to ignore the chromaticities of the provided header and write the ACES ones, so I did the same with colorInteropID.

AcesInputFile is more confusing than I first realized. When I looked at ImfAcesFile.cpp as I was doing the PR, I mistakenly thought the color conversion was part of the AcesOutpuFile because it converts from the file header color space to ACES2065-1, which would be reasonable on output. However, it's actually part of AcesInputFile, and converts from the file's color space to ACES2065-1.

I had thought that ImfAcesFile was intended to read and write ST 2065-4 files, but maybe I'm wrong about what the input side is intended for. It seems that AcesInputFile is actually reading OpenEXR files with arbitrary chromaticities and converting the image buffer to the ACES2065-1 color space. Is that really what it's supposed to do? I would guess that almost no one would want that! The more typical workflow is to use ACES2065-1 as the storage space and ACEScg as the working/image processing space.

Please let me know the actual intent of this class and I will update the PR accordingly. Thank you for your review Peter!


//
// ACES output file.
//
Expand Down
30 changes: 30 additions & 0 deletions src/lib/OpenEXR/ImfMultiPartOutputFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "ImfOutputPartData.h"
#include "ImfOutputStreamMutex.h"
#include "ImfPartType.h"
#include "ImfStandardAttributes.h"
#include "ImfStdIO.h"
#include "ImfThreading.h"
#include "ImfTiledOutputFile.h"
Expand Down Expand Up @@ -377,6 +378,18 @@ MultiPartOutputFile::Data::overrideSharedAttributesValues (
dst.insert ("chromaticities", *chromaticities);
else
dst.erase ("chromaticities");

//
// ColorInteropID -- leave a part's existing "data" tag alone, since
// it marks image data that is intentionally not color managed.
//
if (!hasColorInteropID (dst) || colorInteropID (dst) != "data")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this enforce that if the first part is data, then all parts must be data, but not the other way round? Is that intentional? (or should the clause also be skipped if src has a colorInteropID set to data, and the checkSharedAttributesvalues function have a similar clause?)

In any case, if parts are written with different colorinteropIDs, this function will delete that information from other parts, causing the reader to treat them as being in the wrong space. I would prefer either that the library always checked for colorInteropID consistency on MultiPartOutputFile with no option to bypass test or automatically "fix" attribute, or else all the colorInteropID logic was dropped from the SharedAttribute code, making the multipart rules of the Recommendation less strict.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it should not try to modify an existing value. Is it okay if I just handle it the same as chromaticities then?

{
if (hasColorInteropID (src))
addColorInteropID (dst, colorInteropID (src));
else
dst.erase ("colorInteropID");
}
}

bool
Expand Down Expand Up @@ -447,6 +460,23 @@ MultiPartOutputFile::Data::checkSharedAttributesValues (
}
}

//
// ColorInteropID -- a part after the first may omit it (inheriting
// the first part's value), or set it to "data" to mark image data
// that should not be color managed. Any other value that differs
// from the first part's is a conflict.
//
if (hasColorInteropID (dst) && colorInteropID (dst) != "data")
{
if ((hasColorInteropID (src) &&
colorInteropID (src) != colorInteropID (dst)) ||
(!hasColorInteropID (src)))
{
conflict = true;
conflictingAttributes.push_back ("colorInteropID");
}
}

return conflict;
}

Expand Down
6 changes: 5 additions & 1 deletion src/lib/OpenEXR/ImfMultiPartOutputFile.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ OPENEXR_IMF_INTERNAL_NAMESPACE_HEADER_ENTER
// * Pixel Aspect Ratio
// * Time Code
// * Chromaticities
// * ColorInteropID
// The first header forms the basis for the set of attributes that are shared
// across the constituent parts.
// across the constituent parts. A part after the first may omit
// colorInteropID (inheriting the first part's value), or set it to "data"
// to mark image data that should not be color managed; any other value
// that differs from the first part's is a conflict.
//
// Parameters
// headers - pointer to array of headers; one for each part of the image file
Expand Down
2 changes: 2 additions & 0 deletions src/lib/OpenEXR/ImfRgbaFile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ prefixFromLayerName (const string& layerName, const Header& header)
V3f
ywFromHeader (const Header& header)
{
// TODO: Add support for colorInteropID.

Chromaticities cr;

if (hasChromaticities (header)) cr = chromaticities (header);
Expand Down
41 changes: 20 additions & 21 deletions src/lib/OpenEXR/ImfStandardAttributes.h
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,26 @@ IMF_STD_ATTRIBUTE_DEF (reelName, ReelName, std::string)

IMF_STD_ATTRIBUTE_DEF (chromaticities, Chromaticities, Chromaticities)

//
// ColorInteropID -- the Color Interop Forum ID that describes the color space of the RGB images.
//
// This is a specially designed string used to communicate the color space of
// the image in an interoperable manner. The contents of the string is described
// in the specification "An ID for Color Interop":
// <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/03_ColorInteropID/ColorInteropID.md>
//
// Guidance to application developers is provided in "Identifying the Color Space of OpenEXR Files":
// <https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/04_OpenEXRFiles/OpenEXRFiles.md>
//
// Please adhere to the following usage guidance:
// 1. If the color space of an image is modified, do not forward the stale attribute value.
// 2. Do not set this to a default color space. If the actual color space is not known, omit
// the attribute or set it to "unknown".
//
// New in OpenEXR v3.4

IMF_STD_ATTRIBUTE_DEF (colorInteropID, ColorInteropID, std::string)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the links and moved this up to be next to chromaticities.

//
// whiteLuminance -- for RGB images, defines the luminance, in Nits
// (candelas per square meter) of the RGB value (1.0, 1.0, 1.0).
Expand Down Expand Up @@ -724,25 +744,4 @@ IMF_STD_ATTRIBUTE_DEF_DEPRECATED (

IMF_STD_ATTRIBUTE_DEF (idManifest, IDManifest, CompressedIDManifest)

//
// ColorInteropID -- the Color Interop Forum ID that describes the color space of the RGB images.
//
// This is a specially designed string used to communicate the color space of
// the image in an interoperable manner. The contents of the string is described
// in the specification "An ID for Color Interop":
// <https://docs.google.com/document/d/1T94lYbis9uCskL_ZEMxGBF2JryLfZnjxlEoNgRHZzBE/edit?usp=sharing>
//
// Guidance to application developers is provided in "Identifying the Color Space of OpenEXR Files":
// <https://docs.google.com/document/d/1MTH1bq2L67ifvdDf64Amhzg4AbkIM5LG6yPHrB96Vwo/edit?usp=sharing>
//
// Please adhere to the following usage guidance:
// 1. If the color space of an image is modified, do not forward the stale attribute value.
// 2. Do not set this to a default color space. If the actual color space is not known, omit
// the attribute or set it to "unknown".
//
// New in OpenEXR v3.4


IMF_STD_ATTRIBUTE_DEF (colorInteropID, ColorInteropID, std::string)

#endif
8 changes: 5 additions & 3 deletions src/lib/OpenEXRCore/parse_header.c
Original file line number Diff line number Diff line change
Expand Up @@ -2857,7 +2857,7 @@ internal_exr_parse_header (exr_context_t ctxt)
{
for ( int p = 1; p < ctxt->num_parts; ++p )
{
const char *mismatch[4] = { NULL, NULL, NULL, NULL };
const char *mismatch[5] = { NULL, NULL, NULL, NULL, NULL };
int mismatchcount = 0;
exr_priv_part_t curp = ctxt->parts[p];

Expand All @@ -2872,15 +2872,17 @@ internal_exr_parse_header (exr_context_t ctxt)
rv = ctxt->print_error (
ctxt,
rv,
"Part %d (%s) has non-conforming shared attributes: %s%s%s%s%s%s%s",
"Part %d (%s) has non-conforming shared attributes: %s%s%s%s%s%s%s%s%s",
p, curp->name ? curp->name->string->str : "<missing name>",
mismatch[0] ? mismatch[0] : "",
mismatch[0] ? " " : "",
mismatch[1] ? mismatch[1] : "",
mismatch[1] ? " " : "",
mismatch[2] ? mismatch[2] : "",
mismatch[2] ? " " : "",
mismatch[3] ? mismatch[3] : "");
mismatch[3] ? mismatch[3] : "",
mismatch[3] ? " " : "",
mismatch[4] ? mismatch[4] : "");

// MultiPartInputFile would fail unconditionally
break;
Expand Down
41 changes: 41 additions & 0 deletions src/lib/OpenEXRCore/validation.c
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,47 @@ internal_exr_validate_shared_attrs (exr_context_t ctxt,
if (rv != EXR_ERR_SUCCESS)
mismatchattr[misidx++] = "chromaticities";

/*
* colorInteropID only needs to be set on the first part; later parts
* may omit it (inheriting the first part's color space) or set it to
* "data" to mark images that should not be color managed. Any other
* value on a later part that differs from the first part's is a
* mismatch.
*/
rv = exr_get_attribute_by_name (ctxt, 0, "colorInteropID", &battr);
rv1 = exr_get_attribute_by_name (ctxt, curpartidx, "colorInteropID", &cattr);
if (EXR_ERR_SUCCESS == rv1)
{
if (cattr->type != EXR_ATTR_STRING)
{
rv = EXR_ERR_ATTR_TYPE_MISMATCH;
}
else if (
cattr->string->length == 4 &&
0 == memcmp (cattr->string->str, "data", 4))
{
rv = EXR_ERR_SUCCESS;
}
else if (
EXR_ERR_SUCCESS == rv && battr->type == EXR_ATTR_STRING &&
battr->string->length == cattr->string->length &&
0 == memcmp (
battr->string->str,
cattr->string->str,
(size_t) cattr->string->length))
{
rv = EXR_ERR_SUCCESS;
}
else
{
rv = EXR_ERR_ATTR_TYPE_MISMATCH;
}
}
else
rv = EXR_ERR_SUCCESS; // this part has none, inherits from part 0
if (rv != EXR_ERR_SUCCESS)
mismatchattr[misidx++] = "colorInteropID";

*mismatchcount = misidx;
return misidx == 0 ? EXR_ERR_SUCCESS : EXR_ERR_ATTR_TYPE_MISMATCH;
}
Expand Down
Loading
Loading