From 18f4cf4e36cdc22921954018215b7a3c0147eba7 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 24 May 2024 10:54:53 +0100 Subject: [PATCH 1/2] Cryptomatte : Fix `__matteChannelData` hash - Remove stray hashing of the current input channel. - Check cryptomatte channels exists before hashing them. - Include cryptomatte alpha channel in hash. This fixes one of the problems noted in #5866. --- Changes.md | 3 +++ python/GafferSceneTest/CryptomatteTest.py | 30 +++++++++++++++++++++++ src/GafferScene/Cryptomatte.cpp | 23 +++++++++++++---- 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/Changes.md b/Changes.md index e058c16cefe..f49f9a92787 100644 --- a/Changes.md +++ b/Changes.md @@ -20,6 +20,9 @@ Fixes - Fixed behaviour of `editingFinishedSignal()` to match TextWidget : it is now also emitted when the text is activated (see `activatedSignal()`). - MultiLineStringMetadataWidget : The Ctrl+Return shortcut now updates the metadata value immediately. - UIEditor : The Ctrl+Return shortcut now updates the button code immediately. +- Cryptomatte : + - Fixed errors when the input image didn't contain the main `RGBA` channels. + - Fixed inaccurate hash. API --- diff --git a/python/GafferSceneTest/CryptomatteTest.py b/python/GafferSceneTest/CryptomatteTest.py index 74f9484e8e1..ddb678b2854 100644 --- a/python/GafferSceneTest/CryptomatteTest.py +++ b/python/GafferSceneTest/CryptomatteTest.py @@ -222,6 +222,36 @@ def testPreviewChannels( self ) : self.assertEqual( s["color"].getValue(), imath.Color4f(-7.18267809e-20, 0.448745549, 0.928857431, 1) ) + def testPreviewChannelsWithoutRGBAInput( self ) : + + # As for `testPreviewChannels` but with some additional + # protection against bad accesses to non-existent RGBA + # input channels. + + reader = GafferImage.ImageReader() + reader["fileName"].setValue( self.testImage ) + + # ImageReader doesn't check for the existence of channels + # in `hash()`, but Shuffle does. So we insert a shuffle + # as additional protection against bugs in Cryptomatte. + + shuffle = GafferImage.Shuffle() + shuffle["in"].setInput( reader["out"] ) + + cryptomatte = GafferScene.Cryptomatte() + cryptomatte["in"].setInput( shuffle["out"] ) + cryptomatte["layer"].setValue( "crypto_object" ) + + sampler = GafferImage.ImageSampler() + sampler["image"].setInput( cryptomatte["out"] ) + sampler["pixel"].setValue( imath.V2f( 32, 108 ) ) + + self.assertEqual( sampler["color"].getValue(), imath.Color4f(-7.18267809e-20, 0.198745549, 0.178857431, 0) ) + + cryptomatte["matteNames"].setValue( IECore.StringVectorData( [ "/cow1" ] ) ) + + self.assertEqual( sampler["color"].getValue(), imath.Color4f(-7.18267809e-20, 0.448745549, 0.928857431, 1) ) + def testWildcardMatch( self ) : r = GafferImage.ImageReader() diff --git a/src/GafferScene/Cryptomatte.cpp b/src/GafferScene/Cryptomatte.cpp index 8252290f116..ee609656f62 100644 --- a/src/GafferScene/Cryptomatte.cpp +++ b/src/GafferScene/Cryptomatte.cpp @@ -563,26 +563,39 @@ void Cryptomatte::hash( const Gaffer::ValuePlug *output, const Gaffer::Context * } else if( output == matteChannelDataPlug() ) { - inPlug()->channelDataPlug()->hash( h ); - std::string cryptomatteLayer; ConstStringVectorDataPtr channelNamesData; { GafferImage::ImagePlug::GlobalScope globalScope( context ); - cryptomatteLayer = layerPlug()->getValue(); channelNamesData = inPlug()->channelNamesPlug()->getValue(); - + cryptomatteLayer = layerPlug()->getValue(); matteValuesPlug()->hash( h ); } + const std::vector &channelNames = channelNamesData->readable(); + boost::regex channelNameRegex( fmt::format( g_cryptomatteChannelPattern, cryptomatteLayer ) ); GafferImage::ImagePlug::ChannelDataScope channelDataScope( context ); - for( const auto &c : channelNamesData->readable() ) + for( const auto &c : channelNames ) { if( boost::regex_match( c, channelNameRegex ) ) { + ChannelMap::const_iterator cIt = g_channelMap.find( GafferImage::ImageAlgo::baseName( c ) ); + if( cIt == g_channelMap.end() ) + { + continue; + } + + const std::string &alphaChannel = GafferImage::ImageAlgo::channelName( GafferImage::ImageAlgo::layerName( c ), cIt->second ); + if( !GafferImage::ImageAlgo::channelExists( channelNames, alphaChannel ) ) + { + continue; + } + channelDataScope.setChannelName( &c ); inPlug()->channelDataPlug()->hash( h ); + channelDataScope.setChannelName( &alphaChannel ); + inPlug()->channelDataPlug()->hash( h ); } } } From 804c92c10b0f96d9cb303601c18566d9f9895517 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 24 May 2024 11:04:59 +0100 Subject: [PATCH 2/2] Cryptomatte : Match `hash/computeChannelData()` to each other The second half of the hash function bore little resemblance to the compute, and had at least one bug : the call to the base class was missing. Conforming the two was simplified by restructuring the compute to reduce nesting by using as many early-outs as possible. --- src/GafferScene/Cryptomatte.cpp | 125 ++++++++++++++++---------------- 1 file changed, 62 insertions(+), 63 deletions(-) diff --git a/src/GafferScene/Cryptomatte.cpp b/src/GafferScene/Cryptomatte.cpp index ee609656f62..11509049b48 100644 --- a/src/GafferScene/Cryptomatte.cpp +++ b/src/GafferScene/Cryptomatte.cpp @@ -877,13 +877,15 @@ IECore::ConstStringVectorDataPtr Cryptomatte::computeChannelNames( const Gaffer: void Cryptomatte::hashChannelData( const GafferImage::ImagePlug *output, const Gaffer::Context *context, IECore::MurmurHash &h ) const { + const std::string &channelName = context->get( GafferImage::ImagePlug::channelNameContextName ); + const Imath::V2i &tileOrigin = context->get( GafferImage::ImagePlug::tileOriginContextName ); + std::string alphaChannel; { GafferImage::ImagePlug::GlobalScope globalScope( context ); alphaChannel = outputChannelPlug()->getValue(); } - const std::string channelName = context->get( GafferImage::ImagePlug::channelNameContextName ); if( channelName != "R" && channelName != "G" && channelName != "B" && channelName != alphaChannel ) { h = inPlug()->channelDataPlug()->hash(); @@ -912,46 +914,42 @@ void Cryptomatte::hashChannelData( const GafferImage::ImagePlug *output, const G } } - const std::string &firstDataChannel = cryptomatteLayer + g_firstDataChannelSuffix; - if( !GafferImage::ImageAlgo::channelExists( channelNamesData->readable(), firstDataChannel ) ) + if( channelName == alphaChannel ) { - h = GafferImage::ImagePlug::blackTile()->Object::hash(); + h = matteChannelDataPlug()->hash(); return; } - h.append( channelName ); + // `channelName` is in RGB + const std::string firstDataChannel = cryptomatteLayer + g_firstDataChannelSuffix; + if( !GafferImage::ImageAlgo::channelExists( channelNamesData->readable(), firstDataChannel ) ) { - GafferImage::ImagePlug::GlobalScope globalScope( context ); - layerPlug()->hash( h ); - matteValuesPlug()->hash( h ); - inPlug()->metadataPlug()->hash( h ); - - if( channelName == alphaChannel ) - { - outputChannelPlug()->hash( h ); - } + h = GafferImage::ImagePlug::blackTile()->Object::hash(); + return; } - if( channelName != "R" ) + if( channelName == "R" ) { - matteChannelDataPlug()->hash( h ); + h = inPlug()->channelDataHash( firstDataChannel, tileOrigin ); + return; } - GafferImage::ImagePlug::ChannelDataScope channelDataScope( context ); - channelDataScope.setChannelName( &firstDataChannel ); - inPlug()->channelDataPlug()->hash( h ); + // `channelName` is in GB + + FlatImageProcessor::hashChannelData( output, context, h ); + + h.append( inPlug()->channelDataHash( firstDataChannel, tileOrigin ) ); + matteChannelDataPlug()->hash( h ); + h.append( channelName ); + } IECore::ConstFloatVectorDataPtr Cryptomatte::computeChannelData( const std::string &channelName, const Imath::V2i &tileOrigin, const Gaffer::Context *context, const GafferImage::ImagePlug *parent ) const { - std::string cryptomatteLayer; std::string alphaChannel; - ConstStringVectorDataPtr channelNamesData; { GafferImage::ImagePlug::GlobalScope globalScope( context ); - channelNamesData = inPlug()->channelNamesPlug()->getValue(); - cryptomatteLayer = layerPlug()->getValue(); alphaChannel = outputChannelPlug()->getValue(); } @@ -960,11 +958,17 @@ IECore::ConstFloatVectorDataPtr Cryptomatte::computeChannelData( const std::stri return inPlug()->channelDataPlug()->getValue(); } - const std::vector &channelNames = channelNamesData->readable(); + std::string cryptomatteLayer; + ConstStringVectorDataPtr channelNamesData; + { + GafferImage::ImagePlug::GlobalScope globalScope( context ); + channelNamesData = inPlug()->channelNamesPlug()->getValue(); + cryptomatteLayer = layerPlug()->getValue(); + } if( cryptomatteLayer == "" ) { - if( GafferImage::ImageAlgo::channelExists( channelNames, channelName ) ) + if( GafferImage::ImageAlgo::channelExists( channelNamesData->readable(), channelName ) ) { return inPlug()->channelDataPlug()->getValue(); } @@ -974,55 +978,50 @@ IECore::ConstFloatVectorDataPtr Cryptomatte::computeChannelData( const std::stri } } - if( channelName == "R" || channelName == "G" || channelName == "B" ) + if( channelName == alphaChannel ) { - const std::string &firstDataChannel = cryptomatteLayer + g_firstDataChannelSuffix; - if( !GafferImage::ImageAlgo::channelExists( channelNames, firstDataChannel ) ) - { - return GafferImage::ImagePlug::blackTile(); - } + return matteChannelDataPlug()->getValue(); + } - GafferImage::ImagePlug::ChannelDataScope channelDataScope( context ); - channelDataScope.setChannelName( &firstDataChannel ); + // `channelName` is in RGB - if( channelName == "R" ) - { - return inPlug()->channelDataPlug()->getValue(); - } - else if( channelName == "G" || channelName == "B" ) - { - FloatVectorDataPtr resultData = new FloatVectorData; - std::vector &result = resultData->writable(); - result.resize( GafferImage::ImagePlug::tilePixels(), 0.0f ); + const std::string firstDataChannel = cryptomatteLayer + g_firstDataChannelSuffix; + if( !GafferImage::ImageAlgo::channelExists( channelNamesData->readable(), firstDataChannel ) ) + { + return GafferImage::ImagePlug::blackTile(); + } - ConstFloatVectorDataPtr valueData = inPlug()->channelDataPlug()->getValue(); - const std::vector &values = valueData->readable(); + if( channelName == "R" ) + { + return inPlug()->channelData( firstDataChannel, tileOrigin ); + } - ConstFloatVectorDataPtr alphaData = matteChannelDataPlug()->getValue(); - const std::vector &alphas = alphaData->readable(); + // `channelName` is in GB - const size_t shift = channelName == "G" ? 8 : 16; - const float mult = channelName == "G" ? 0.25f : 0.75f; - uint32_t h; + FloatVectorDataPtr resultData = new FloatVectorData; + std::vector &result = resultData->writable(); + result.resize( GafferImage::ImagePlug::tilePixels(), 0.0f ); - std::vector::const_iterator vIt = values.begin(); - std::vector::const_iterator aIt = alphas.begin(); - for( std::vector::iterator it = result.begin(), eIt = result.end(); it != eIt; ++it, ++vIt, ++aIt ) - { - // Adapted from the Cryptomatte specification - std::memcpy( &h, &(*vIt), sizeof( uint32_t ) ); - *it = (float)(h << shift) / (float)UINT32_MAX * 0.3f + *aIt * mult; - } + ConstFloatVectorDataPtr valueData = inPlug()->channelData( firstDataChannel, tileOrigin ); + const std::vector &values = valueData->readable(); - return resultData; - } - } - else if( channelName == alphaChannel ) + ConstFloatVectorDataPtr alphaData = matteChannelDataPlug()->getValue(); + const std::vector &alphas = alphaData->readable(); + + const size_t shift = channelName == "G" ? 8 : 16; + const float mult = channelName == "G" ? 0.25f : 0.75f; + uint32_t h; + + std::vector::const_iterator vIt = values.begin(); + std::vector::const_iterator aIt = alphas.begin(); + for( std::vector::iterator it = result.begin(), eIt = result.end(); it != eIt; ++it, ++vIt, ++aIt ) { - return matteChannelDataPlug()->getValue(); + // Adapted from the Cryptomatte specification + std::memcpy( &h, &(*vIt), sizeof( uint32_t ) ); + *it = (float)(h << shift) / (float)UINT32_MAX * 0.3f + *aIt * mult; } - return GafferImage::ImagePlug::blackTile(); + return resultData; } } // namespace GafferScene