Skip to content

Bugfix: RGB/Hue Curve Performance with MSL #2342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
carolalynn wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −68
Open

Bugfix: RGB/Hue Curve Performance with MSL #2342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 35 additions & 31 deletions src/OpenColorIO/GpuShaderClassWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,36 +181,10 @@ std::string MetalShaderClassWrapper::generateClassWrapperHeader(GpuShaderText& k
kw.indent();
for(const auto& param : m_functionParameters)
{
size_t openAngledBracketPos = param.m_name.find('[');
if(!param.m_isArray)
{
kw.newLine() << "this->" << param.m_name << " = " << param.m_name << ";";
}
else
{
size_t closeAngledBracketPos = param.m_name.find(']');
std::string variableName = param.m_name.substr(0, openAngledBracketPos);

kw.newLine() << "for(int i = 0; i < "
<< GetArrayLengthVariableName(variableName)
<< "; ++i)";
kw.newLine() << "{";
kw.indent();
kw.newLine() << "this->" << variableName << "[i] = " << variableName << "[i];";
kw.dedent();
kw.newLine() << "}";

kw.newLine() << "for(int i = "
<< GetArrayLengthVariableName(variableName)
<< "; i < "
<< param.m_name.substr(openAngledBracketPos+1, closeAngledBracketPos-openAngledBracketPos-1)
<< "; ++i)";
kw.newLine() << "{";
kw.indent();
kw.newLine() << "this->" << variableName << "[i] = 0;";
kw.dedent();
kw.newLine() << "}";
}
// Array members are pointers into constant memory (see rewriteArrayDeclarations) so they
// are bound rather than copied, using the parameter name without its array size.
std::string variableName = param.m_name.substr(0, param.m_name.find('['));
kw.newLine() << "this->" << variableName << " = " << variableName << ";";
}
kw.dedent();
kw.newLine() <<"}";
Expand Down Expand Up @@ -378,6 +352,36 @@ void MetalShaderClassWrapper::prepareClassWrapper(const std::string& resourcePre
extractFunctionParameters(originalHeader);
}

std::string MetalShaderClassWrapper::rewriteArrayDeclarations(const std::string& declarations) const
{
// The uniform declarations are shared with the other GPU languages so arrays arrive here as
// fixed-size members i.e. 'float name[120];'. Owning them would make the constructor copy the
// whole array out of constant memory into per-thread memory, which is prohibitive as the
// struct is typically instantiated once per pixel. Hold a pointer to the constant memory
// instead; every read site is unchanged as the indexing syntax is the same.

@doug-walker doug-walker Sep 11, 2026

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.

Is it true that "the struct is typically instantiated once per pixel"? Isn't it more typical that a group of pixels is processed in a batch, to amortize cost like this?

I'm all in favor of the change to improve performance for the single-pixel case, but I suspect it's not the "typical" situation.

//
// Note that nothing zero-fills the members between the array length and the declared capacity
// any more. An op declaring an array uniform must therefore keep its reads below the length it
// reports, as only that many elements are uploaded: a read beyond it now runs past the
// uploaded data instead of returning zero.
std::string rewritten = declarations;
for(const auto& param : m_functionParameters)
{
if(!param.m_isArray)
continue;

const std::string declaration = param.m_type + " " + param.m_name + ";";
const size_t pos = rewritten.find(declaration);
if(pos != std::string::npos)
{
rewritten.replace(pos, declaration.size(),
"constant " + param.m_type + "* "
+ param.m_name.substr(0, param.m_name.find('[')) + ";");
}
}
return rewritten;
}

std::string MetalShaderClassWrapper::getClassWrapperHeader(const std::string& originalHeader)
{
GpuShaderText st(GPU_LANGUAGE_MSL_2_0);
Expand All @@ -388,7 +392,7 @@ std::string MetalShaderClassWrapper::getClassWrapperHeader(const std::string& or
std::string classWrapHeader = "\n// Declaration of class wrapper\n\n";
classWrapHeader += st.string();

return classWrapHeader + originalHeader;
return classWrapHeader + rewriteArrayDeclarations(originalHeader);
}

std::string MetalShaderClassWrapper::getClassWrapperFooter(const std::string& originalFooter)
Expand Down
1 change: 1 addition & 0 deletions src/OpenColorIO/GpuShaderClassWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class MetalShaderClassWrapper : public GpuShaderClassWrapper

static std::string getClassWrapperName(const std::string &resourcePrefix, const std::string &functionName);
void extractFunctionParameters(const std::string& declaration);
std::string rewriteArrayDeclarations(const std::string& declarations) const;
std::string generateClassWrapperHeader(GpuShaderText& st) const;
std::string generateClassWrapperFooter(GpuShaderText& st, const std::string &ocioFunctionName) const;

Expand Down
59 changes: 22 additions & 37 deletions tests/cpu/GpuShader_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1215,48 +1215,20 @@ ocioOCIOMain(
, bool ocio_grading_rgbcurve_localBypass
)
{
for(int i = 0; i < ocio_grading_rgbcurve_knotsOffsets_count; ++i)
{
this->ocio_grading_rgbcurve_knotsOffsets[i] = ocio_grading_rgbcurve_knotsOffsets[i];
}
for(int i = ocio_grading_rgbcurve_knotsOffsets_count; i < 8; ++i)
{
this->ocio_grading_rgbcurve_knotsOffsets[i] = 0;
}
for(int i = 0; i < ocio_grading_rgbcurve_knots_count; ++i)
{
this->ocio_grading_rgbcurve_knots[i] = ocio_grading_rgbcurve_knots[i];
}
for(int i = ocio_grading_rgbcurve_knots_count; i < 120; ++i)
{
this->ocio_grading_rgbcurve_knots[i] = 0;
}
for(int i = 0; i < ocio_grading_rgbcurve_coefsOffsets_count; ++i)
{
this->ocio_grading_rgbcurve_coefsOffsets[i] = ocio_grading_rgbcurve_coefsOffsets[i];
}
for(int i = ocio_grading_rgbcurve_coefsOffsets_count; i < 8; ++i)
{
this->ocio_grading_rgbcurve_coefsOffsets[i] = 0;
}
for(int i = 0; i < ocio_grading_rgbcurve_coefs_count; ++i)
{
this->ocio_grading_rgbcurve_coefs[i] = ocio_grading_rgbcurve_coefs[i];
}
for(int i = ocio_grading_rgbcurve_coefs_count; i < 360; ++i)
{
this->ocio_grading_rgbcurve_coefs[i] = 0;
}
this->ocio_grading_rgbcurve_knotsOffsets = ocio_grading_rgbcurve_knotsOffsets;
this->ocio_grading_rgbcurve_knots = ocio_grading_rgbcurve_knots;
this->ocio_grading_rgbcurve_coefsOffsets = ocio_grading_rgbcurve_coefsOffsets;
this->ocio_grading_rgbcurve_coefs = ocio_grading_rgbcurve_coefs;
this->ocio_grading_rgbcurve_localBypass = ocio_grading_rgbcurve_localBypass;
}


// Declaration of all variables

int ocio_grading_rgbcurve_knotsOffsets[8];
float ocio_grading_rgbcurve_knots[120];
int ocio_grading_rgbcurve_coefsOffsets[8];
float ocio_grading_rgbcurve_coefs[360];
constant int* ocio_grading_rgbcurve_knotsOffsets;
constant float* ocio_grading_rgbcurve_knots;
constant int* ocio_grading_rgbcurve_coefsOffsets;
constant float* ocio_grading_rgbcurve_coefs;
bool ocio_grading_rgbcurve_localBypass;


Expand Down Expand Up @@ -1361,8 +1333,21 @@ float4 OCIOMain(
).OCIOMain(inPixel);
}
)" };

OCIO_CHECK_EQUAL(expected, text);

// The struct is instantiated once per invocation, which for a compute shader
// dispatched one thread per pixel means once per pixel. Owning the array
// uniforms rather than pointing at them therefore copies the whole declared
// capacity into per-thread memory for every pixel, which dominated the cost
// of any curve op. These checks state that invariant on its own so a future
// update of the expected text above cannot quietly restore the copy.
OCIO_CHECK_ASSERT(text.find("constant float* ocio_grading_rgbcurve_knots;")
!= std::string::npos);
OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots = "
"ocio_grading_rgbcurve_knots;") != std::string::npos);
OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots[i]")
== std::string::npos);

@doug-walker doug-walker Sep 11, 2026

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.

Maybe this is a nit pick but it seems like kind of an odd test. The first two asserts are redundant after the comparison of the shader program above. And for the third, if we don't want people to change the code back in the future, perhaps just a comment where the original code is should suffice? If someone did try to return to the original approach, it would have to be written exactly the same in order to fail this test (e.g., maybe someone uses j rather than i for their loop)?

}

OCIO_ADD_TEST(GpuShader, VulkanSupport)
Expand Down