Optimisations to OCIOIPNode#670
Merged
bernie-laberge merged 7 commits intoAcademySoftwareFoundation:mainfrom Mar 27, 2025
Merged
Optimisations to OCIOIPNode#670bernie-laberge merged 7 commits intoAcademySoftwareFoundation:mainfrom
bernie-laberge merged 7 commits intoAcademySoftwareFoundation:mainfrom
Conversation
kenmcgaugh
requested review from
bernie-laberge,
cedrik-fuoco-adsk and
eloisebrosseau
as code owners
kenmcgaugh
force-pushed
the
ocio-optimise
branch
from
df149f6 to
f2aaca7
Compare
Contributor
|
Hello @kenmcgaugh, I just want to let you know that we are very excited about this PR of yours ! Thank you for contributing to the Open RV cause ! |
|
Nice work Ken! |
bernie-laberge
approved these changes
Mar 21, 2025
Contributor
There was a problem hiding this comment.
LGTM
Great idea to separate the OCIO Gpu processor generation business from the OCIOIpNode::evaluate() even though I believe it was supposed to be cached at the OCIO level. Something we had never revisited since this node was introduced 12 years ago (I checked).
I have tested it and it really makes a difference.
Thanks you @kenmcgaugh !
And I apologize for the time it took for the review: it is crunch time here for us.
Contributor
Author
|
Thanks for your feedback. Interestingly enough, just yesterday I discovered a couple of small issues when reading .rv session files. I’ll commit my update fixing them. One is to add a call to updateFunction() in the readCompleted() method. The other is to return early from updateFunction() if m_state->linear.empty(). |
…uate() method and into its own method that is called during during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk>
### Add .git-blame-ignore-revs ### Summarize your change. Add a .git-blame-ignore-revs file and update README.md to explain how to use it. ### Describe the reason for the change. A .git-blame-ignore-revs file lists commits to ignore when running `git blame`, such as formatting commits. This allows you to use `git blame` without these commits cluttering the Git history. For now, this file only contains the formatting of all the C++ codebase in Open RV based on the new .clang-format. Other refactoring changes that are not modifying the code logic can be added to this file as needed. --------- Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk>
…o main branch) (AcademySoftwareFoundation#672) ### Adding some variable to prepare for Qt6 in openrv-sub ### Linked issues n/a ### Summarize your change. Adding some Cmake variables to identify if the Qt package needed is Qt5 or Qt6. ### Describe the reason for the change. This is needed for the future merge of the Qt6 branch but also to push the Qt6 changes to openrv-sub without affecting the current build. ### Describe what you have tested and on which operating system. All Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk>
### Add missing rvSession.py ### Linked issues NA ### Describe the reason for the change. This python file module was unfortunately forgotten when RV was open sourced. ### Summarize your change. I removed the duplicated gtoContainer.py file module (duplication is bad) and made the necessary adjustments to the CMake build system to install it when building Open RV. ### Describe what you have tested and on which operating system. Successfully testes on macOS. ### Add a list of changes, and note any that might need special attention during the review. ### If possible, provide screenshots. Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk>
Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk>
kenmcgaugh
force-pushed
the
ocio-optimise
branch
from
6ae597e to
bd6480f
Compare
Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk>
eloisebrosseau
approved these changes
Mar 24, 2025
bernie-laberge
merged commit 59cee81
into
AcademySoftwareFoundation:main
10 checks passed
cedrik-fuoco-adsk
added a commit
to cedrik-fuoco-adsk/OpenRV
that referenced
this pull request
Apr 8, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com>
cedrik-fuoco-adsk
added a commit
to cedrik-fuoco-adsk/OpenRV
that referenced
this pull request
Apr 8, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
cedrik-fuoco-adsk
added a commit
to cedrik-fuoco-adsk/OpenRV
that referenced
this pull request
Apr 25, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
cedrik-fuoco-adsk
added a commit
to cedrik-fuoco-adsk/OpenRV
that referenced
this pull request
Apr 28, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
cedrik-fuoco-adsk
added a commit
to cedrik-fuoco-adsk/OpenRV
that referenced
this pull request
Apr 28, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
richardssam
pushed a commit
to richardssam/OpenRV
that referenced
this pull request
Jun 17, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com> Signed-off-by: Sam.Richards@taurich.org <Sam.Richards@taurich.org>
richardssam
pushed a commit
to richardssam/OpenRV
that referenced
this pull request
Jun 17, 2025
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead. Removed all mutex handling as the code being protected now only ever executes in the main thread (I think). Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now matches the existing behavior of "ocio_look.outColorSpace". Changed behavior where an empty "ocio_display.display" property uses the config's default display. Changed behavior where an empty "ocio_display.view" property uses the config's default view. The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set. Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through. When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong. In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance. --------- Signed-off-by: Ken McGaugh <ken@mcgaugh.co.uk> Signed-off-by: Éloïse Brosseau <eloise.brosseau@autodesk.com> Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Bernard Laberge <bernard.laberge@autodesk.com> Signed-off-by: kenmcgaugh <ken@mcgaugh.co.uk> Co-authored-by: Éloïse Brosseau <54746458+eloisebrosseau@users.noreply.github.com> Co-authored-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Bernard Laberge <117092886+bernie-laberge@users.noreply.github.com> Signed-off-by: Sam.Richards@taurich.org <Sam.Richards@taurich.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactored to move all the OCIO shader building code out of the evaluate() method and into its own method that is called during propertyChanged() instead.
Removed all mutex handling as the code being protected now only ever executes in the main thread (I think).
Changed behavior where an empty "ocio_color.outColorSpace" property will default to scene_linear. This now
matches the existing behavior of "ocio_look.outColorSpace".
Changed behavior where an empty "ocio_display.display" property uses the config's default display.
Changed behavior where an empty "ocio_display.view" property uses the config's default view.
The above changes to these properties prevent error messages when OCIOIPNode's are instantiated since the shader building now executes before there is a chance for the properties to be set.
Added check for gpuProcessor->isNoOp() so that the node becomes a pass-through.
When the Shader::Function is constructed, passed numTextures+num3Dtextures for the fetches argument rather than 1. Felt like the right thing to do but I may be completely wrong.
In my tests on a MacBook Pro (Apple M1 Pro), I'm getting almost 2x speed up in playback performance.