Merge pull request #4690 from danieldresser-ie/mixFix

Fix GafferImage::Mix

Changes.md

@@ -1,3 +1,12 @@
+0.61.x.x (relative to 0.61.11.0)
+=========
+
+Fixes
+-----
+
+- Image Node Mix : Fixed incorrect results outside mask data window, and incorrect results when changing inputs.
+
+
 0.61.11.0 (relative to 0.61.10.0)
 =========
 

python/GafferImageTest/ImageTestCase.py

@@ -107,7 +107,7 @@ def assertImagesEqual( self, imageA, imageB, maxDifference = 0.0, ignoreMetadata
 
 			stats = GafferImage.ImageStats()
 			stats["in"].setInput( difference["out"] )
-			stats["area"].setValue( imageA["format"].getValue().getDisplayWindow() )
+			stats["area"].setValue( imageA["dataWindow"].getValue() )
 
 			for channelName in imageA["channelNames"].getValue() :
 

python/GafferImageTest/ImageTransformTest.py

@@ -575,7 +575,7 @@ def testConcatenation( self ) :
 			# show that it is legitimate : differences in filtering are that great.
 			# The threshold is still significantly lower than the differences between
 			# checker tiles, so does guarantee that tiles aren't getting out of alignment.
-			self.assertImagesEqual( tc2["out"], t2["out"], maxDifference = 0.11, ignoreDataWindow = True )
+			self.assertImagesEqual( tc2["out"], t2["out"], maxDifference = 0.17, ignoreDataWindow = True )
 
 	def testDisabledAndNonConcatenating( self ) :
 

python/GafferImageTest/MixTest.py

@@ -51,6 +51,7 @@ class MixTest( GafferImageTest.ImageTestCase ) :
 	rPath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/redWithDataWindow.100x100.exr" )
 	gPath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/greenWithDataWindow.100x100.exr" )
 	checkerPath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/checkerboard.100x100.exr" )
+	checkerNegativeDataWindowPath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/checkerWithNegativeDataWindow.200x150.exr" )
 	checkerMixPath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/checkerMix.100x100.exr" )
 	representativeDeepImagePath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/representativeDeepImage.exr" )
 	radialPath = os.path.expandvars( "$GAFFER_ROOT/python/GafferImageTest/images/radial.exr" )
@@ -601,5 +602,94 @@ def testMismatchThrows( self ) :
 		mix["in"][0].setInput( deep["out"] )
 		GafferImage.ImageAlgo.tiles( mix["out"] )
 
+	def testFuzzDataWindows( self ):
+
+		# A bunch of different test images with varying data windows
+		file1 = GafferImage.ImageReader()
+		file1["fileName"].setValue( self.checkerNegativeDataWindowPath )
+
+		file1Shuffle = GafferImage.Shuffle()
+		file1Shuffle["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "A", "R" ) )
+		file1Shuffle['in'].setInput( file1['out'] )
+
+		file2 = GafferImage.ImageReader()
+		file2["fileName"].setValue( self.checkerPath )
+
+		file2Shuffle = GafferImage.Shuffle()
+		file2Shuffle["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "A", "R" ) )
+		file2Shuffle['in'].setInput( file2['out'] )
+
+		largeConstant = GafferImage.Constant()
+		largeConstant["format"].setValue( GafferImage.Format( imath.Box2i( imath.V2i( 0 ), imath.V2i( 1920, 1080 ) ), 1 ) )
+		largeConstant["color"].setValue( imath.Color4f( 0.7, 0.8, 0.9, 0.65 ) )
+
+		smallConstant = GafferImage.Constant()
+		smallConstant["format"].setValue( GafferImage.Format( imath.Box2i( imath.V2i( 554, 210 ), imath.V2i( 1265, 869 ) ), 1 ) )
+		smallConstant["color"].setValue( imath.Color4f( 0.4, 0.5, 0.6, 0.45 ) )
+
+		leftConstant = GafferImage.Constant()
+		leftConstant["format"].setValue( GafferImage.Format( imath.Box2i( imath.V2i( -353, -561 ), imath.V2i( 915, 500 ) ), 1 ) )
+		leftConstant["color"].setValue( imath.Color4f( 0.1, 0.2, 0.3, 0.75 ) )
+
+		rightConstant = GafferImage.Constant()
+		rightConstant["format"].setValue( GafferImage.Format( imath.Box2i( imath.V2i( 945, 557 ), imath.V2i( 2007, 1117 ) ), 1 ) )
+		rightConstant["color"].setValue( imath.Color4f( 0.61, 0.62, 0.63, 0.35 ) )
+
+		Mix = GafferImage.Mix( "Mix" )
+
+		# Create a network using Merge that should match the result of Mix
+		MaskPromote = GafferImage.Shuffle( "MaskPromote" )
+		MaskPromote["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "R", "A" ) )
+		MaskPromote["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "G", "A" ) )
+		MaskPromote["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "B", "A" ) )
+		MaskPromote["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "A", "A" ) )
+		MaskPromote['in'].setInput( Mix['mask'] )
+
+		Input0Mult = GafferImage.Merge( "Input0Mult" )
+		Input0Mult["operation"].setValue( GafferImage.Merge.Operation.Multiply )
+		Input0Mult['in'][0].setInput( Mix['in'][0] )
+		Input0Mult["in"][1].setInput( MaskPromote["out"] )
+
+		Input0Subtract = GafferImage.Merge( "Input0Subtract" )
+		Input0Subtract["operation"].setValue( GafferImage.Merge.Operation.Subtract )
+		Input0Subtract["in"][0].setInput( Input0Mult["out"] )
+		Input0Subtract['in'][1].setInput( Mix['in'][0] )
+
+		Input1Mult = GafferImage.Merge( "Input1Mult" )
+		Input1Mult["operation"].setValue( GafferImage.Merge.Operation.Multiply )
+		Input1Mult['in'][0].setInput( Mix['in'][1] )
+		Input1Mult["in"][1].setInput( MaskPromote["out"] )
+
+		ReferenceMerge = GafferImage.Merge( "ReferenceMerge" )
+		ReferenceMerge["operation"].setValue( GafferImage.Merge.Operation.Add )
+		ReferenceMerge["in"][0].setInput( Input0Subtract["out"] )
+		ReferenceMerge["in"][1].setInput( Input1Mult["out"] )
+
+		# Expand the data window of the reference to match the Mix, so we can compare
+		BlackBackground = GafferImage.Shuffle( "BlackBackground" )
+		BlackBackground["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "R", "__black" ) )
+		BlackBackground["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "G", "__black" ) )
+		BlackBackground["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "B", "__black" ) )
+		BlackBackground["channels"].addChild( GafferImage.Shuffle.ChannelPlug( "A", "__black" ) )
+		BlackBackground["in"].setInput( Mix["out"] )
+
+		ReferenceWithDataWindow = GafferImage.Merge( "ReferenceWithDataWindow" )
+		ReferenceWithDataWindow["operation"].setValue( GafferImage.Merge.Operation.Over )
+		ReferenceWithDataWindow["in"][0].setInput( BlackBackground["out"] )
+		ReferenceWithDataWindow["in"][1].setInput( ReferenceMerge["out"] )
+
+		# For a more thorough test, use the full list of test images - but restricting to just 3 catches
+		# most failures, and takes just 2 seconds to test
+		#testImages = [ largeConstant, smallConstant, leftConstant, rightConstant, file1Shuffle, file2Shuffle ]
+		testImages = [ largeConstant, smallConstant, leftConstant ]
+		for input0 in testImages:
+			for input1 in testImages:
+				for mask in testImages:
+					Mix["in"][0].setInput( input0["out"] )
+					Mix["in"][1].setInput( input1["out"] )
+					Mix["mask"].setInput( mask["out"] )
+
+					self.assertImagesEqual( Mix["out"], ReferenceWithDataWindow["out"], maxDifference = 1e-7 )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferImage/Mix.cpp

@@ -115,6 +115,11 @@ void Mix::affects( const Gaffer::Plug *input, AffectedPlugsContainer &outputs )
 		outputs.push_back( outPlug()->dataWindowPlug() );
 		outputs.push_back( outPlug()->channelNamesPlug() );
 	}
+	else if( input == maskPlug()->dataWindowPlug() )
+	{
+		outputs.push_back( outPlug()->dataWindowPlug() );
+		outputs.push_back( outPlug()->channelDataPlug() );
+	}
 	else if( input == maskPlug()->deepPlug() || input == maskPlug()->sampleOffsetsPlug() )
 	{
 		outputs.push_back( outPlug()->channelDataPlug() );
@@ -128,6 +133,10 @@ void Mix::affects( const Gaffer::Plug *input, AffectedPlugsContainer &outputs )
 		if( inputImage->parent<ArrayPlug>() == inPlugs() )
 		{
 			outputs.push_back( outPlug()->getChild<ValuePlug>( input->getName() ) );
+			if( input == inputImage->dataWindowPlug() )
+			{
+				outputs.push_back( outPlug()->channelDataPlug() );
+			}
 		}
 	}
 }
@@ -173,6 +182,7 @@ Imath::Box2i Mix::computeDataWindow( const Gaffer::Context *context, const Image
 		dataWindow.extendBy( (*it)->dataWindowPlug()->getValue() );
 	}
 
+
 	return dataWindow;
 }
 
@@ -279,7 +289,8 @@ void Mix::hashChannelData( const GafferImage::ImagePlug *output, const Gaffer::C
 		{
 			maskValidBound = boxIntersection( tileBound, maskPlug()->dataWindowPlug()->getValue() );
 		}
-		h.append( maskValidBound );
+		h.append( maskValidBound.min - tileOrigin );
+		h.append( maskValidBound.max - tileOrigin );
 
 		for( int i = 0; i < 2; i++ )
 		{
@@ -301,7 +312,12 @@ void Mix::hashChannelData( const GafferImage::ImagePlug *output, const Gaffer::C
 			// input data windows, we may be using/revealing the invalid parts of a tile. We
 			// deal with this in computeChannelData() by treating the invalid parts as black,
 			// and must therefore hash in the valid bound here to take that into account.
-			h.append( validBound[i] );
+			//
+			// Note that validBound only matters in relation to the channel data for this tile though,
+			// so we can hash it in relation to the tile origin, providing a small speedup by allowing
+			// reuse of constant tiles ( 20% speedup in testFuzzDataWindows )
+			h.append( validBound[i].min - tileOrigin );
+			h.append( validBound[i].max - tileOrigin );
 		}
 		outPlug()->deepPlug()->hash( h );
 		maskPlug()->deepPlug()->hash( h );
@@ -351,6 +367,7 @@ IECore::ConstFloatVectorDataPtr Mix::computeChannelData( const std::string &chan
 
 	std::string maskChannel;
 	Box2i maskValidBound;
+	bool hasMask = false;
 	bool maskDeep;
 	{
 		// Start by grabbing all the plug values we need that are global
@@ -380,10 +397,21 @@ IECore::ConstFloatVectorDataPtr Mix::computeChannelData( const std::string &chan
 		if( maskPlug()->getInput<ValuePlug>() &&
 			ImageAlgo::channelExists( maskPlug()->channelNamesPlug()->getValue()->readable(), maskChannel ) )
 		{
+			hasMask = true;
 			maskValidBound = boxIntersection( tileBound, maskPlug()->dataWindowPlug()->getValue() );
 			if( BufferAlgo::empty( maskValidBound ) )
 			{
-				return inputs[ 0 ]->channelData( channelName, tileOrigin );
+				if( BufferAlgo::empty( validBound[0] ) )
+				{
+					return ImagePlug::blackTile();
+				}
+				else if( validBound[0] == tileBound )
+				{
+					// If the whole tile is within the input data window, we can just pass through
+					// the input. Otherwise, we need to hit the slower path below to trim out the
+					// part of the input which is outside the input's data window.
+					return inputs[ 0 ]->channelData( channelName, tileOrigin );
+				}
 			}
 		}
 
@@ -525,16 +553,12 @@ IECore::ConstFloatVectorDataPtr Mix::computeChannelData( const std::string &chan
 					b = *B;
 				}
 
-				float m = mix;
+				float m = hasMask ? 0.0f : mix;
 				if( M )
 				{
 					if( yValidMask && x >= maskValidBound.min.x && x < maskValidBound.max.x )
 					{
-						m *= std::max( 0.0f, std::min( 1.0f, *M ) );
-					}
-					else
-					{
-						m = 0;
+						m = mix * std::max( 0.0f, std::min( 1.0f, *M ) );
 					}
 					++M;
 				}