ImageGadget : Fix "colour tearing" under cancellation

Because of the way we were suppressing exceptions in `Tile::computeUpdate()`, we could end up calling `Tile::applyUpdates()` with an incomplete set of updates (where some have null `channelData`). This could cause us to display a tile with a mix of updated and stale channels, causing the very "colour tearing" that `applyUpdates()` was intended to avoid.

We now handle exceptions one level higher up in the `tileFunctor`, allowing us to clear the active flags for all channels, but _not_ apply any channel data updates. This approach is much closer to the one originally proposed in #4011, which I replaced with a botched version in #5348.

Changes.md

@@ -9,7 +9,9 @@ Improvements
 Fixes
 -----
 
-- Viewer, ImageGadget : Fixed unnecessary texture updates when specific image tiles don't change.
+- Viewer, ImageGadget :
+  - 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.
 - Constraint : The `target` browser now shows locations from the `targetScene` if it has an input connection. Before it always showed locations from the main input.
 - ImageInspector : Fixed broken UI caused by double-clicking in the Image tab.
 

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 );

src/GafferImageUI/ImageGadget.cpp

@@ -717,25 +717,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 )
@@ -761,6 +754,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,28 +857,42 @@ 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();
+			}
 		}
+
 	};
 
 	Context::Scope scopedContext( m_context.get() );