Merge pull request #6050 from johnhaddon/imageGadgetCancellationFix

ImageGadget fixes

Changes.md

@@ -1,7 +1,13 @@
 1.3.16.x (relative to 1.3.16.8)
 ========
 
+Fixes
+-----
 
+- Viewer, ImageGadget :
+  - Fixed partial image updates when an unrelated InteractiveRender was running (#6043).
+  - Fixed "colour tearing", where updates to some image channels became visible before updates to others.
+  - Fixed unnecessary texture updates when specific image tiles don't change.
 
 1.3.16.8 (relative to 1.3.16.7)
 ========

include/GafferImageUI/ImageGadget.h

@@ -292,6 +292,7 @@ class GAFFERIMAGEUI_API ImageGadget : public GafferUI::Gadget
 			// Applies previously computed updates for several tiles
 			// such that they become visible to the UI thread together.
 			static void applyUpdates( const std::vector<Update> &updates );
+			void resetActive();
 
 			// Called from the UI thread.
 			const IECoreGL::Texture *texture( bool &active );

python/GafferImageUITest/ImageGadgetTest.py

@@ -34,6 +34,7 @@
 #
 ##########################################################################
 
+import time
 import unittest
 import imath
 
@@ -120,5 +121,29 @@ def testStateChangedSignal( self ) :
 		self.assertEqual( len( cs ), 2 )
 		self.assertNotEqual( gadget.state(), gadget.State.Paused )
 
+	def testNoUnecessaryUpdates( self ) :
+
+		script = Gaffer.ScriptNode()
+		script["image"] = GafferImage.Checkerboard()
+		script["image"]["format"].setValue( GafferImage.Format( GafferImage.ImagePlug.tileSize(), GafferImage.ImagePlug.tileSize() ) )
+
+		gadget = GafferImageUI.ImageGadget()
+		gadget.setImage( script["image"]["out"] )
+		gadget.setContext( script.context() )
+
+		with GafferUI.Window() as window :
+			GafferUI.GadgetWidget( gadget )
+
+		GafferImageUI.ImageGadget.resetTileUpdateCount()
+		window.setVisible( True )
+		while GafferImageUI.ImageGadget.tileUpdateCount() < 4 :
+			self.waitForIdle()
+
+		for frame in range( 2, 4 ) :
+			script.context().setFrame( frame )
+			time.sleep( 0.5 )
+			self.waitForIdle()
+			self.assertEqual( GafferImageUI.ImageGadget.tileUpdateCount(), 4 )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferImageUI/ImageGadget.cpp

@@ -53,6 +53,7 @@
 #include "Gaffer/BackgroundTask.h"
 #include "Gaffer/Context.h"
 #include "Gaffer/Node.h"
+#include "Gaffer/Process.h"
 #include "Gaffer/ScriptNode.h"
 
 #include "IECore/MessageHandler.h"
@@ -717,25 +718,18 @@ ImageGadget::Tile::Tile( const Tile &other )
 
 ImageGadget::Tile::Update ImageGadget::Tile::computeUpdate( const GafferImage::ImagePlug *image )
 {
-	try
-	{
-		const IECore::MurmurHash h = image->channelDataPlug()->hash();
-		Mutex::scoped_lock lock( m_mutex );
-		if( m_channelDataHash != MurmurHash() && m_channelDataHash == h )
-		{
-			return Update{ this, nullptr, MurmurHash() };
-		}
-
-		m_active = true;
-		m_activeStartTime = std::chrono::steady_clock::now();
-		lock.release(); // Release while doing expensive calculation so UI thread doesn't wait.
-		ConstFloatVectorDataPtr channelData = image->channelDataPlug()->getValue( &h );
-		return Update{ this, channelData, h };
-	}
-	catch( ... )
+	const IECore::MurmurHash h = image->channelDataPlug()->hash();
+	Mutex::scoped_lock lock( m_mutex );
+	if( m_channelDataHash != MurmurHash() && m_channelDataHash == h )
 	{
 		return Update{ this, nullptr, MurmurHash() };
 	}
+
+	m_active = true;
+	m_activeStartTime = std::chrono::steady_clock::now();
+	lock.release(); // Release while doing expensive calculation so UI thread doesn't wait.
+	ConstFloatVectorDataPtr channelData = image->channelDataPlug()->getValue( &h );
+	return Update{ this, channelData, h };
 }
 
 void ImageGadget::Tile::applyUpdates( const std::vector<Update> &updates )
@@ -747,7 +741,7 @@ void ImageGadget::Tile::applyUpdates( const std::vector<Update> &updates )
 
 	for( const auto &u : updates )
 	{
-		if( u.tile )
+		if( u.channelData )
 		{
 			u.tile->m_channelDataToConvert = u.channelData;
 			u.tile->m_channelDataHash = u.channelDataHash;
@@ -761,6 +755,12 @@ void ImageGadget::Tile::applyUpdates( const std::vector<Update> &updates )
 	}
 }
 
+void ImageGadget::Tile::resetActive()
+{
+	Mutex::scoped_lock lock( m_mutex );
+	m_active = false;
+}
+
 const IECoreGL::Texture *ImageGadget::Tile::texture( bool &active )
 {
 	const auto now = std::chrono::steady_clock::now();
@@ -858,47 +858,70 @@ void ImageGadget::updateTiles()
 
 	auto tileFunctor = [this, channelsToCompute] ( const ImagePlug *image, const V2i &tileOrigin ) {
 
-		vector<Tile::Update> updates;
-		ImagePlug::ChannelDataScope channelScope( Context::current() );
-		for( auto &channelName : channelsToCompute )
+		try
 		{
-			channelScope.setChannelName( &channelName );
-			Tile &tile = m_tiles[TileIndex(tileOrigin, channelName)];
-			updates.push_back( tile.computeUpdate( image ) );
-		}
+			vector<Tile::Update> updates;
+			ImagePlug::ChannelDataScope channelScope( Context::current() );
+			for( auto &channelName : channelsToCompute )
+			{
+				channelScope.setChannelName( &channelName );
+				Tile &tile = m_tiles[TileIndex(tileOrigin, channelName)];
+				updates.push_back( tile.computeUpdate( image ) );
+			}
 
-		Tile::applyUpdates( updates );
+			Tile::applyUpdates( updates );
 
-		if( refCount() && !m_renderRequestPending.exchange( true ) )
+			if( refCount() && !m_renderRequestPending.exchange( true ) )
+			{
+				// Must hold a reference to stop us dying before our UI thread call is scheduled.
+				ImageGadgetPtr thisRef = this;
+				ParallelAlgo::callOnUIThread(
+					[thisRef] {
+						thisRef->m_renderRequestPending = false;
+						thisRef->Gadget::dirty( DirtyType::Render );
+					}
+				);
+			}
+		}
+		catch( ... )
 		{
-			// Must hold a reference to stop us dying before our UI thread call is scheduled.
-			ImageGadgetPtr thisRef = this;
-			ParallelAlgo::callOnUIThread(
-				[thisRef] {
-					thisRef->m_renderRequestPending = false;
-					thisRef->Gadget::dirty( DirtyType::Render );
-				}
-			);
+			// We don't want to call `Tile::applyUpdates()` because we won't have
+			// a complete set of updates for all channels. But we do need to turn off
+			// the active flag for each tile.
+			for( auto &channelName : channelsToCompute )
+			{
+				m_tiles[TileIndex(tileOrigin, channelName)].resetActive();
+			}
+			throw;
 		}
-	};
 
+	};
 
-	// callOnBackgroundThread requires a "subject" that will trigger task cancellation
-	// when dirtied.  This subject usually needs to be in a script, but there's a special
-	// case in BackgroundTask::scriptNode for nodes that are in a GafferUI::View.  We
-	// can work with this by passing in m_image, which is passed to us by ImageView.
-	// This means that any internal nodes of ImageGadget are not part of the automatic
-	// task cancellation and we must ensure that we never modify internal nodes while
-	// the background task is running ( this is easier now that there are no internal nodes ).
 	Context::Scope scopedContext( m_context.get() );
 	m_tilesTask = ParallelAlgo::callOnBackgroundThread(
 		// Subject
 		m_image.get(),
 		// OK to capture `this` via raw pointer, because ~ImageGadget waits for
 		// the background process to complete.
 		[ this, channelsToCompute, dataWindow, tileFunctor ] {
-			ImageAlgo::parallelProcessTiles( m_image.get(), tileFunctor, dataWindow );
-			m_dirtyFlags &= ~TilesDirty;
+
+			try
+			{
+				ImageAlgo::parallelProcessTiles( m_image.get(), tileFunctor, dataWindow );
+				m_dirtyFlags &= ~TilesDirty;
+			}
+			catch( const Gaffer::ProcessException & )
+			{
+				// No point starting a new compute if it's just
+				// going to error again.
+				m_dirtyFlags &= ~TilesDirty;
+			}
+			catch( const IECore::Cancelled & )
+			{
+				// Don't clear dirty flag, so that we restart
+				// on the next redraw.
+			}
+
 			if( refCount() )
 			{
 				ImageGadgetPtr thisRef = this;
@@ -908,6 +931,7 @@ void ImageGadget::updateTiles()
 					}
 				);
 			}
+
 		}
 	);