Changes to OSL ownership of color systems #1405

lgritz · 2021-09-14T06:51:59Z

Patrick Hodoul pointed out that we were forcing OpenColorIO to
initialize and read configs as soon as the ShadingSystem was
created. In this patch, we're changing to a lazy creation approach.

We remove the OCIOColorSystem m_ocio_system member and the
ocio_transform() method from ShadingSystem. Put them in the
ShadingContext instead. This allows lock-free access because each
context is used by only one thread at a time.

Within the OCIOColorSystem, make the OIIO::ColorConfig be a
shared_ptr rather than a member, so it can be created lazily. If the
Context's OCIOColorSystem needs a config and doesn't have its own yet,
it asks the ShadingSystem, which allocates one the first time (and
only that one time).

So, in short, SS allocates one shared ColorConfig, the first time
any context asks for one. The Context only asks the SS for this once
per context (i.e., once per thread). Individual shades just use the
ColorConfig of their context, not needing to lock for it (except for
that brief lock the first time any context asks for the very first
ColorConfig that's needed).

Some things were guarded by OIIO_HAS_COLORPROCESSOR, which is silly
because we no longer support OIIO versions so old that it's not
defined. But then OSL undefed it for Cuda. I replaced these guards
by a more straightforward direct test of whether it was Cuda code.

I added a sample OCIO config to testsuite so we can use it for tests.
In particular, the new testsuite/ocio -- test using OCIO for color
transforms. We never actually tested that in the test suite before!

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

Patrick Hodoul pointed out that we were forcing OpenColorIO to initialize and read configs as soon as the ShadingSystem was created. In this patch, we're changing to a lazy creation approach. We remove the `OCIOColorSystem m_ocio_system` member and the ocio_transform() method from ShadingSystem. Put them in the ShadingContext instead. This allows lock-free access because each context is used by only one thread at a time. Within the OCIOColorSystem, make the `OIIO::ColorConfig` be a shared_ptr rather than a member, so it can be created lazily. If the Context's OCIOColorSystem needs a config and doesn't have its own yet, it asks the ShadingSystem, which allocates one the first time (and only that one time). So, in short, SS allocates *one* shared ColorConfig, the first time any context asks for one. The Context only asks the SS for this once per context (i.e., once per thread). Individual shades just use the ColorConfig of their context, not needing to lock for it (except for that brief lock the first time any context asks for the very first ColorConfig that's needed). Some things were guarded by `OIIO_HAS_COLORPROCESSOR`, which is silly because we no longer support OIIO versions so old that it's not defined. But then OSL `undef`ed it for Cuda. I replaced these guards by a more straightforward direct test of whether it was Cuda code. I added a sample OCIO config to testsuite so we can use it for tests. In particular, the new testsuite/ocio -- test using OCIO for color transforms. Signed-off-by: Larry Gritz <[email protected]>

lgritz · 2021-09-29T22:47:47Z

Any objections and/or positive reviews?

AlexMWells · 2021-09-29T23:50:18Z

src/liboslexec/oslexec_pvt.h

@@ -835,7 +833,7 @@ class ShadingSystemImpl

    ustring m_colorspace;                 ///< What RGB colors mean
    ColorSystem m_colorsystem;            ///< Data for current colorspace
-    OCIOColorSystem m_ocio_system;        ///< OCIO color system (when OIIO_HAS_COLORPROCESSOR=1)
+    std::shared_ptr<OIIO::ColorConfig> m_colorconfig;  ///< OIIO/OCIO color configuration


Just checking... is any CUDA_ARCH needed around this?

I don't think so. Builds fine for me.

AlexMWells · 2021-09-29T23:50:57Z

src/liboslexec/oslexec_pvt.h

@@ -2081,6 +2083,8 @@ class OSLEXECPUBLIC ShadingContext {

    Dictionary *m_dictionary;

+    OCIOColorSystem m_ocio_system;


Just checking... is any CUDA_ARCH needed around this?

Builds fine for me.

AlexMWells · 2021-09-29T23:52:55Z

src/liboslexec/shadingsys.cpp

+std::shared_ptr<OIIO::ColorConfig>
+ShadingSystemImpl::colorconfig()
+{
+    lock_guard lock(m_mutex);


Is the guard only needed when it's not initialized? Won't this lock a mutex everytime colorconfig() is called?

The mutex is needed every time ShadingSystemImpl::colorconfig() is called, but it will only be called the first time a ColorConfig is needed by each ShadingContext (that is, each thread), after which a copy is retained in the context, where it does not need a lock because it's only used by one thread.

So if there are n threads in your renderer, this lock will be engaged at most n times during the whole render.

sounds good, just the name of the method wouldn't make me think it has the potential cost of a lock involved

sfriedmapixar · 2021-09-29T23:41:08Z

src/liboslexec/opcolor.h

@@ -18,9 +18,6 @@

 #include <OpenImageIO/color.h>

-#ifdef __CUDACC__
-  #undef OIIO_HAS_COLORPROCESSOR
-#endif



Changing all of these will make it difficult for someone to use OSL with an OIIO that doesn't have OCIO. I know I used that at one point just to get things up and running quickly, but if you feel that's an extra config worth not supporting, that's fine. Just wanted to call it out.

You're misunderstanding what this means. OIIO_HAS_COLORPROCESSOR never meant "OIIO was built with OCIO." Rather, it meant "you are using an OIIO that is new enough to have the ColorProcessor class exposed in the public API", which was added between 1.9 and 2.0. Current OSL doesn't support OIIO versions old enough to predate this, so the guard is no longer needed.

OIIO's ColorProcessor exists even if the OIIO was built without OCIO support, or even if no OCIO config is found. In those cases, it reverts to a basic implementation that knows a few general color transforms like converting between sRGB and a linear transfer function.

Got it, then LGTM.

lgritz · 2021-10-05T18:59:27Z

Did I answer everybody's questions on this adequately, or are there outstanding concerns?

AlexMWells reviewed Sep 29, 2021

View reviewed changes

sfriedmapixar approved these changes Sep 30, 2021

View reviewed changes

lgritz merged commit bee949c into AcademySoftwareFoundation:master Oct 5, 2021

lgritz deleted the lg-lazyocio branch October 8, 2021 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to OSL ownership of color systems #1405

Changes to OSL ownership of color systems #1405

lgritz commented Sep 14, 2021

lgritz commented Sep 29, 2021

AlexMWells Sep 29, 2021

lgritz Sep 30, 2021

AlexMWells Sep 29, 2021

lgritz Sep 30, 2021

AlexMWells Sep 29, 2021 •

edited

Loading

lgritz Sep 30, 2021 •

edited

Loading

AlexMWells Sep 30, 2021

sfriedmapixar Sep 29, 2021

lgritz Sep 30, 2021

sfriedmapixar Oct 5, 2021

lgritz commented Oct 5, 2021

		@@ -2081,6 +2083,8 @@ class OSLEXECPUBLIC ShadingContext {

		Dictionary *m_dictionary;

		OCIOColorSystem m_ocio_system;

Changes to OSL ownership of color systems #1405

Changes to OSL ownership of color systems #1405

Conversation

lgritz commented Sep 14, 2021

lgritz commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexMWells Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

lgritz Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgritz commented Oct 5, 2021

AlexMWells Sep 29, 2021 •

edited

Loading

lgritz Sep 30, 2021 •

edited

Loading