SL-17483: Recast LLImageDecodeThread as a facade for ThreadPool.

Remove all references to LLQueuedThread (but emulate a couple bits of its API
such as handle_t and getPending()).

Migrate ImageRequest into llimageworker.cpp. It has never been part of
LLImageDecodeThread's public API. Remove ImageRequest tests.

Remove all references to LLImageDecodeThread::pause(). The idea of pausing
another thread is bizarre to me, and LLThreadPool has no such operation. Nor
does it have an abortRequest().

8 files changed, 91 insertions, 157 deletions

diff --git a/indra/llimage/llimageworker.cpp b/indra/llimage/llimageworker.cpp
index d8503396d7..3c21499673 100644
--- a/indra/llimage/llimageworker.cpp
+++ b/indra/llimage/llimageworker.cpp
@@ -28,44 +28,83 @@
 
 #include "llimageworker.h"
 #include "llimagedxt.h"
+#include "threadpool.h"
+
+/*--------------------------------------------------------------------------*/
+class ImageRequest
+{
+public:
+	ImageRequest(LLPointer<LLImageFormatted> image,
+				 S32 discard, BOOL needs_aux,
+				 LLPointer<LLImageDecodeThread::Responder> responder);
+	virtual ~ImageRequest();
+
+	/*virtual*/ bool processRequest();
+	/*virtual*/ void finishRequest(bool completed);
+
+private:
+	// input
+	LLPointer<LLImageFormatted> mFormattedImage;
+	S32 mDiscardLevel;
+	BOOL mNeedsAux;
+	// output
+	LLPointer<LLImageRaw> mDecodedImageRaw;
+	LLPointer<LLImageRaw> mDecodedImageAux;
+	BOOL mDecodedRaw;
+	BOOL mDecodedAux;
+	LLPointer<LLImageDecodeThread::Responder> mResponder;
+};
+
 
 //----------------------------------------------------------------------------
 
 // MAIN THREAD
-LLImageDecodeThread::LLImageDecodeThread(bool threaded)
-	: LLQueuedThread("imagedecode", threaded)
+LLImageDecodeThread::LLImageDecodeThread(bool /*threaded*/)
 {
-	mCreationMutex = new LLMutex();
+    mThreadPool.reset(new LL::ThreadPool("ImageDecode", 8));
+    mThreadPool->start();
 }
 
 //virtual 
 LLImageDecodeThread::~LLImageDecodeThread()
-{
-	delete mCreationMutex ;
-}
+{}
 
 // MAIN THREAD
 // virtual
 S32 LLImageDecodeThread::update(F32 max_time_ms)
 {
     LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
-	S32 res = LLQueuedThread::update(max_time_ms);
-	return res;
+    return getPending();
 }
 
-LLImageDecodeThread::handle_t LLImageDecodeThread::decodeImage(LLImageFormatted* image, 
-	S32 discard, BOOL needs_aux, Responder* responder)
+S32 LLImageDecodeThread::getPending()
 {
-    LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
-	handle_t handle = generateHandle();
+    return mThreadPool->getQueue().size();
+}
 
-    ImageRequest* req = new ImageRequest(handle, image,
-        discard, needs_aux,
-        responder);
+LLImageDecodeThread::handle_t LLImageDecodeThread::decodeImage(LLPointer<LLImageFormatted> image, 
+    S32 discard, BOOL needs_aux, LLPointer<LLImageDecodeThread::Responder> responder)
+{
+    LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
 
-    addRequest(req);
+    // Instantiate the ImageRequest right in the lambda, why not?
+    mThreadPool->getQueue().post(
+        [req = ImageRequest(image, discard, needs_aux, responder)]
+        () mutable
+        {
+            auto done = req.processRequest();
+            req.finishRequest(done);
+        });
+
+    // It's important to our consumer (LLTextureFetchWorker) that we return a
+    // nonzero handle. It is NOT important that the nonzero handle be unique:
+    // nothing is ever done with it except to compare it to zero, or zero it.
+    return 17;
+}
 
-	return handle;
+void LLImageDecodeThread::shutdown()
+{
+    mThreadPool->close();
 }
 
 LLImageDecodeThread::Responder::~Responder()
@@ -74,11 +113,10 @@ LLImageDecodeThread::Responder::~Responder()
 
 //----------------------------------------------------------------------------
 
-LLImageDecodeThread::ImageRequest::ImageRequest(handle_t handle, LLImageFormatted* image, 
-												S32 discard, BOOL needs_aux,
-												LLImageDecodeThread::Responder* responder)
-	: LLQueuedThread::QueuedRequest(handle, FLAG_AUTO_COMPLETE),
-	  mFormattedImage(image),
+ImageRequest::ImageRequest(LLPointer<LLImageFormatted> image, 
+							S32 discard, BOOL needs_aux,
+							LLPointer<LLImageDecodeThread::Responder> responder)
+	: mFormattedImage(image),
 	  mDiscardLevel(discard),
 	  mNeedsAux(needs_aux),
 	  mDecodedRaw(FALSE),
@@ -87,7 +125,7 @@ LLImageDecodeThread::ImageRequest::ImageRequest(handle_t handle, LLImageFormatte
 {
 }
 
-LLImageDecodeThread::ImageRequest::~ImageRequest()
+ImageRequest::~ImageRequest()
 {
 	mDecodedImageRaw = NULL;
 	mDecodedImageAux = NULL;
@@ -98,7 +136,7 @@ LLImageDecodeThread::ImageRequest::~ImageRequest()
 
 
 // Returns true when done, whether or not decode was successful.
-bool LLImageDecodeThread::ImageRequest::processRequest()
+bool ImageRequest::processRequest()
 {
     LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
 	const F32 decode_time_slice = 0.f; //disable time slicing
@@ -125,7 +163,7 @@ bool LLImageDecodeThread::ImageRequest::processRequest()
 											  mFormattedImage->getHeight(),
 											  mFormattedImage->getComponents());
 		}
-		done = mFormattedImage->decode(mDecodedImageRaw, decode_time_slice); // 1ms
+		done = mFormattedImage->decode(mDecodedImageRaw, decode_time_slice);
 		// some decoders are removing data when task is complete and there were errors
 		mDecodedRaw = done && mDecodedImageRaw->getData();
 	}
@@ -138,14 +176,14 @@ bool LLImageDecodeThread::ImageRequest::processRequest()
 											  mFormattedImage->getHeight(),
 											  1);
 		}
-		done = mFormattedImage->decodeChannels(mDecodedImageAux, decode_time_slice, 4, 4); // 1ms
+		done = mFormattedImage->decodeChannels(mDecodedImageAux, decode_time_slice, 4, 4);
 		mDecodedAux = done && mDecodedImageAux->getData();
 	}
 
 	return done;
 }
 
-void LLImageDecodeThread::ImageRequest::finishRequest(bool completed)
+void ImageRequest::finishRequest(bool completed)
 {
     LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE;
 	if (mResponder.notNull())
@@ -155,10 +193,3 @@ void LLImageDecodeThread::ImageRequest::finishRequest(bool completed)
 	}
 	// Will automatically be deleted
 }
-
-// Used by unit test only
-// Checks that a responder exists for this instance so that something can happen when completion is reached
-bool LLImageDecodeThread::ImageRequest::tut_isOK()
-{
-	return mResponder.notNull();
-}
diff --git a/indra/llimage/llimageworker.h b/indra/llimage/llimageworker.h
index e0a94d2841..6a0b2b4681 100644
--- a/indra/llimage/llimageworker.h
+++ b/indra/llimage/llimageworker.h
@@ -29,9 +29,13 @@
 
 #include "llimage.h"
 #include "llpointer.h"
-#include "llworkerthread.h"
 
-class LLImageDecodeThread : public LLQueuedThread
+namespace LL
+{
+    class ThreadPool;
+} // namespace LL
+
+class LLImageDecodeThread
 {
 public:
 	class Responder : public LLThreadSafeRefCount
@@ -42,57 +46,24 @@ public:
 		virtual void completed(bool success, LLImageRaw* raw, LLImageRaw* aux) = 0;
 	};
 
-	class ImageRequest : public LLQueuedThread::QueuedRequest
-	{
-	protected:
-		virtual ~ImageRequest(); // use deleteRequest()
-		
-	public:
-		ImageRequest(handle_t handle, LLImageFormatted* image,
-					 S32 discard, BOOL needs_aux,
-					 LLImageDecodeThread::Responder* responder);
-
-		/*virtual*/ bool processRequest();
-		/*virtual*/ void finishRequest(bool completed);
-
-		// Used by unit tests to check the consitency of the request instance
-		bool tut_isOK();
-		
-	private:
-		// input
-		LLPointer<LLImageFormatted> mFormattedImage;
-		S32 mDiscardLevel;
-		BOOL mNeedsAux;
-		// output
-		LLPointer<LLImageRaw> mDecodedImageRaw;
-		LLPointer<LLImageRaw> mDecodedImageAux;
-		BOOL mDecodedRaw;
-		BOOL mDecodedAux;
-		LLPointer<LLImageDecodeThread::Responder> mResponder;
-	};
-	
 public:
 	LLImageDecodeThread(bool threaded = true);
 	virtual ~LLImageDecodeThread();
 
-	handle_t decodeImage(LLImageFormatted* image,
+	// meant to resemble LLQueuedThread::handle_t
+	typedef U32 handle_t;
+	handle_t decodeImage(LLPointer<LLImageFormatted> image,
 						 S32 discard, BOOL needs_aux,
-						 Responder* responder);
+						 LLPointer<Responder> responder);
+	S32 getPending();
 	S32 update(F32 max_time_ms);
+	void shutdown();
 
 private:
-	struct creation_info
-	{
-		handle_t handle;
-		LLPointer<LLImageFormatted> image;
-		S32 discard;
-		BOOL needs_aux;
-		LLPointer<Responder> responder;
-		creation_info(handle_t h, LLImageFormatted* i, U32 p, S32 d, BOOL aux, Responder* r)
-			: handle(h), image(i), discard(d), needs_aux(aux), responder(r)
-		{}
-	};
-	LLMutex* mCreationMutex;
+	// As of SL-17483, LLImageDecodeThread is no longer itself an
+	// LLQueuedThread - instead this is the API by which we submit work to the
+	// "ImageDecode" ThreadPool.
+	std::unique_ptr<LL::ThreadPool> mThreadPool;
 };
 
 #endif
diff --git a/indra/llimage/tests/llimageworker_test.cpp b/indra/llimage/tests/llimageworker_test.cpp
index d36d35aba4..0a97b739b0 100644
--- a/indra/llimage/tests/llimageworker_test.cpp
+++ b/indra/llimage/tests/llimageworker_test.cpp
@@ -125,42 +125,11 @@ namespace tut
 		}
 	};
 
-	// Test wrapper declaration : image worker
-	// Note: this class is not meant to be instantiated outside an LLImageDecodeThread instance
-	// but it's not a bad idea to get its public API a good shake as part of a thorough unit test set.
-	// Some gotcha with the destructor though (see below).
-	struct imagerequest_test
-	{
-		// Instance to be tested
-		LLImageDecodeThread::ImageRequest* mRequest;
-		bool done;
-
-		// Constructor and destructor of the test wrapper
-		imagerequest_test()
-		{
-			done = false;
-
-			mRequest = new LLImageDecodeThread::ImageRequest(0, 0,
-											 0, FALSE,
-											 new responder_test(&done));
-		}
-		~imagerequest_test()
-		{
-			// We should delete the object *but*, because its destructor is protected, that cannot be
-			// done from outside an LLImageDecodeThread instance... So we leak memory here... It's fine...
-			//delete mRequest;
-		}
-	};
-
 	// Tut templating thingamagic: test group, object and test instance
 	typedef test_group<imagedecodethread_test> imagedecodethread_t;
 	typedef imagedecodethread_t::object imagedecodethread_object_t;
 	tut::imagedecodethread_t tut_imagedecodethread("LLImageDecodeThread");
 
-	typedef test_group<imagerequest_test> imagerequest_t;
-	typedef imagerequest_t::object imagerequest_object_t;
-	tut::imagerequest_t tut_imagerequest("LLImageRequest");
-
 	// ---------------------------------------------------------------------------------------
 	// Test functions
 	// Notes:
@@ -172,21 +141,6 @@ namespace tut
 	// ---------------------------------------------------------------------------------------
 	// Test the LLImageDecodeThread interface
 	// ---------------------------------------------------------------------------------------
-	//
-	// Note on Unit Testing Queued Thread Classes
-	//
-	// Since methods on such a class are called on a separate loop and that we can't insert tut
-	// ensure() calls in there, we exercise the class with 2 sets of tests:
-	// - 1: Test as a single threaded instance: We declare the class but ask for no thread
-	//   to be spawned (easy with LLThreads since there's a boolean argument on the constructor
-	//   just for that). We can then unit test each public method like we do on a normal class.
-	// - 2: Test as a threaded instance: We let the thread launch and check that its external 
-	//   behavior is as expected (i.e. it runs, can accept a work order and processes
-	//   it). Typically though there's no guarantee that this exercises all the methods of the
-	//   class which is why we also need the previous "non threaded" set of unit tests for
-	//   complete coverage.
-	//
-	// ---------------------------------------------------------------------------------------
 
 	template<> template<>
 	void imagedecodethread_object_t::test<1>()
@@ -211,24 +165,4 @@ namespace tut
 		// Verifies that the responder has now been called
 		ensure("LLImageDecodeThread: threaded work unit not processed", done == true);
 	}
-
-	// ---------------------------------------------------------------------------------------
-	// Test the LLImageDecodeThread::ImageRequest interface
-	// ---------------------------------------------------------------------------------------
-	
-	template<> template<>
-	void imagerequest_object_t::test<1>()
-	{
-		// Test that we start with a correct request at creation
-		ensure("LLImageDecodeThread::ImageRequest::ImageRequest() constructor test failed", mRequest->tut_isOK());
-		bool res = mRequest->processRequest();
-		// Verifies that we processed the request successfully
-		ensure("LLImageDecodeThread::ImageRequest::processRequest() processing request test failed", res == true);
-		// Check that we can call the finishing call safely
-		try {
-			mRequest->finishRequest(false);
-		} catch (...) {
-			fail("LLImageDecodeThread::ImageRequest::finishRequest() test failed");
-		}
-	}
 }
diff --git a/indra/newview/app_settings/settings.xml b/indra/newview/app_settings/settings.xml
index 3201848f38..bc4945eca5 100644
--- a/indra/newview/app_settings/settings.xml
+++ b/indra/newview/app_settings/settings.xml
@@ -12646,6 +12646,8 @@
       <map>
         <key>General</key>
         <integer>1</integer>
+        <key>ImageDecode</key>
+        <integer>9</integer>
       </map>
     </map>
     <key>ThrottleBandwidthKBPS</key>
diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp
index bc38a96ef2..ae0c9d4dd6 100644
--- a/indra/newview/llappviewer.cpp
+++ b/indra/newview/llappviewer.cpp
@@ -1548,7 +1548,6 @@ bool LLAppViewer::doFrame()
 			{
 				S32 non_interactive_ms_sleep_time = 100;
 				LLAppViewer::getTextureCache()->pause();
-				LLAppViewer::getImageDecodeThread()->pause();
 				ms_sleep(non_interactive_ms_sleep_time);
 			}
 
@@ -1568,7 +1567,6 @@ bool LLAppViewer::doFrame()
 					ms_sleep(milliseconds_to_sleep);
 					// also pause worker threads during this wait period
 					LLAppViewer::getTextureCache()->pause();
-					LLAppViewer::getImageDecodeThread()->pause();
 				}
 			}
 
@@ -1617,7 +1615,6 @@ bool LLAppViewer::doFrame()
 			{
 				LL_PROFILE_ZONE_NAMED_CATEGORY_APP( "df getTextureCache" )
 				LLAppViewer::getTextureCache()->pause();
-				LLAppViewer::getImageDecodeThread()->pause();
 				LLAppViewer::getTextureFetch()->pause();
 			}
 			if(!total_io_pending) //pause file threads if nothing to process.
@@ -2051,10 +2048,10 @@ bool LLAppViewer::cleanup()
 	sTextureCache->shutdown();
 	sImageDecodeThread->shutdown();
 	sPurgeDiskCacheThread->shutdown();
-    if (mGeneralThreadPool)
-    {
-        mGeneralThreadPool->close();
-    }
+	if (mGeneralThreadPool)
+	{
+		mGeneralThreadPool->close();
+	}
 
 	sTextureFetch->shutDownTextureCacheThread() ;
 	sTextureFetch->shutDownImageDecodeThread() ;
diff --git a/indra/newview/lltexturefetch.cpp b/indra/newview/lltexturefetch.cpp
index 64ccbf5e4a..539e2454ea 100644
--- a/indra/newview/lltexturefetch.cpp
+++ b/indra/newview/lltexturefetch.cpp
@@ -2113,10 +2113,10 @@ void LLTextureFetchWorker::onCompleted(LLCore::HttpHandle handle, LLCore::HttpRe
 // Threads:  Tmain
 void LLTextureFetchWorker::endWork(S32 param, bool aborted)
 {
-    LL_PROFILE_ZONE_SCOPED;
+	LL_PROFILE_ZONE_SCOPED;
 	if (mDecodeHandle != 0)
 	{
-		mFetcher->mImageDecodeThread->abortRequest(mDecodeHandle, false);
+		// LL::ThreadPool has no operation to cancel a particular work item
 		mDecodeHandle = 0;
 	}
 	mFormattedImage = NULL;
@@ -3169,7 +3169,7 @@ void LLTextureFetch::shutDownImageDecodeThread()
 {
 	if(mImageDecodeThread)
 	{
-		llassert_always(mImageDecodeThread->isQuitting() || mImageDecodeThread->isStopped()) ;
+		delete mImageDecodeThread;
 		mImageDecodeThread = NULL ;
 	}
 }
diff --git a/indra/newview/llviewerwindow.cpp b/indra/newview/llviewerwindow.cpp
index e27b5caab7..15f20d1d34 100644
--- a/indra/newview/llviewerwindow.cpp
+++ b/indra/newview/llviewerwindow.cpp
@@ -5476,7 +5476,6 @@ void LLViewerWindow::stopGL(BOOL save_state)
 
 		// Pause texture decode threads (will get unpaused during main loop)
 		LLAppViewer::getTextureCache()->pause();
-		LLAppViewer::getImageDecodeThread()->pause();
 		LLAppViewer::getTextureFetch()->pause();
 				
 		gSky.destroyGL();
diff --git a/indra/test/test.cpp b/indra/test/test.cpp
index bb48216b2b..28f25087ac 100644
--- a/indra/test/test.cpp
+++ b/indra/test/test.cpp
@@ -401,7 +401,7 @@ public:
 	{
 		// Per http://confluence.jetbrains.net/display/TCD65/Build+Script+Interaction+with+TeamCity#BuildScriptInteractionwithTeamCity-ServiceMessages
 		std::string result;
-		BOOST_FOREACH(char c, str)
+		for (char c : str)
 		{
 			switch (c)
 			{