Miscellaneous cleanup in and around llmediadataclient.

Added tags to some media-related logging in LLVOVolume.

Made LLMediaDataClient::Responder do most of its work in tick() instead of its destructor.

Added a comment to llmediadataclient.cpp that explains the idea behind the two-queue system.

Made LLMediaDataClient::sortQueue() remove requests from the queue that hold references to dead items.  This should make teleporting away solve many of the pathological queueing cases.

Updated llmediadataclient test cases to reflect the change in behavior in sortQueue().

Removed some unnecessary const-ness in LLMediaDataClient::enqueue, which caused it to have to use const_cast.

1 files changed, 100 insertions, 37 deletions

diff --git a/indra/newview/llmediadataclient.cpp b/indra/newview/llmediadataclient.cpp
index b8da368bd7..8f5290bac2 100755
--- a/indra/newview/llmediadataclient.cpp
+++ b/indra/newview/llmediadataclient.cpp
@@ -58,6 +58,32 @@
 // - Any request that gets a 503 still goes through the retry logic
 //
 
+/***************************************************************************************************************
+	What's up with this queueing code?
+
+	First, a bit of background:
+
+	Media on a prim was added into the system in the Viewer 2.0 timeframe.  In order to avoid changing the 
+	network format of objects, an unused field in the object (the "MediaURL" string) was repurposed to 
+	indicate that the object had media data, and also hold a sequence number and the UUID of the agent 
+	who last updated the data.  The actual media data for objects is accessed via the "ObjectMedia" capability.  
+	Due to concerns about sim performance, requests to this capability are rate-limited to 5 requests every 
+	5 seconds per agent.
+
+	The initial implementation of LLMediaDataClient used a single queue to manage requests to the "ObjectMedia" cap.  
+	Requests to the cap were queued so that objects closer to the avatar were loaded in first, since they were most 
+	likely to be the ones the media performance manager would load.
+
+	This worked in some cases, but we found that it was possible for a scripted object that constantly updated its 
+	media data to starve other objects, since the same queue contained both requests to load previously unseen media 
+	data and requests to fetch media data in response to object updates.
+
+	The solution for this we came up with was to have two queues.  The sorted queue contains requests to fetch media 
+	data for objects that don't have it yet, and the round-robin queue contains requests to update media data for 
+	objects that have already completed their initial load.  When both queues are non-empty, the code ping-pongs 
+	between them so that updates can't completely block initial load-in.
+**************************************************************************************************************/
+
 //
 // Forward decls
 //
@@ -149,7 +175,7 @@ void LLMediaDataClient::request(const LLMediaDataClientObject::ptr_t &object, co
 	enqueue(new Request(getCapabilityName(), payload, object, this));
 }
 
-void LLMediaDataClient::enqueue(const Request *request)
+void LLMediaDataClient::enqueue(Request *request)
 {
 	if (request->isNew())
 	{		
@@ -161,8 +187,7 @@ void LLMediaDataClient::enqueue(const Request *request)
 		
 		LL_DEBUGS("LLMediaDataClient") << "Queuing SORTED request for " << *request << LL_ENDL;
 		
-		// Sadly, we have to const-cast because items put into the queue are not const
-		mSortedQueue.push_back(const_cast<LLMediaDataClient::Request*>(request));
+		mSortedQueue.push_back(request);
 		
 		LL_DEBUGS("LLMediaDataClientQueue") << "SORTED queue:" << mSortedQueue << LL_ENDL;
 	}
@@ -184,8 +209,7 @@ void LLMediaDataClient::enqueue(const Request *request)
 		{
 			LL_DEBUGS("LLMediaDataClient") << "Queuing RR request for " << *request << LL_ENDL;
 			// Push the request on the pending queue
-			// Sadly, we have to const-cast because items put into the queue are not const
-			mRoundRobinQueue.push_front(const_cast<LLMediaDataClient::Request*>(request));
+			mRoundRobinQueue.push_front(request);
 			
 			LL_DEBUGS("LLMediaDataClientQueue") << "RR queue:" << mRoundRobinQueue << LL_ENDL;			
 		}
@@ -240,17 +264,64 @@ bool LLMediaDataClient::processQueueTimer()
 	return isEmpty();
 }
 
+bool LLMediaDataClient::request_needs_purge(LLMediaDataClient::request_ptr_t request)
+{
+	// Check for conditions that should cause a request to get removed from the queue
+	if(request.isNull())
+	{
+		LL_WARNS("LLMediaDataClient") << "removing NULL request" << LL_ENDL;
+		return true;
+	}
+
+	// For some reason, the existing code put sent items onto the back of the round-robin queue and let them come through again
+	// before stripping them off the front.  Was there a good reason for this?
+//	if(request->isMarkedSent())
+//	{
+//		LL_INFOS("LLMediaDataClient") << "removing : " << *request << " request is marked sent" << LL_ENDL;
+//		return true;
+//	}
+
+	const LLMediaDataClientObject *object = request->getObject();
+	
+	if(object == NULL)
+	{
+		LL_INFOS("LLMediaDataClient") << "removing : " << *request << " object is NULL" << LL_ENDL;
+		return true;
+	}
+	
+	if(object->isDead())
+	{
+		LL_INFOS("LLMediaDataClient") << "removing : " << *request << " object is dead" << LL_ENDL;
+		return true;
+	}
+	
+	if(!object->hasMedia())
+	{
+		LL_INFOS("LLMediaDataClient") << "removing : " << *request << " object has no media" << LL_ENDL;
+		return true;
+	}
+
+	return false;	
+}
+
 void LLMediaDataClient::sortQueue()
 {
 	if(!mSortedQueue.empty())
 	{
-		// Score all items first
+		// Cull dead objects and score all remaining items first
 		request_queue_t::iterator iter = mSortedQueue.begin();
 		request_queue_t::iterator end = mSortedQueue.end();
 		while (iter != end)
 		{
-			(*iter)->updateScore();
-			iter++;
+			if(request_needs_purge(*iter))
+			{
+				iter = mSortedQueue.erase(iter);
+			}
+			else
+			{
+				(*iter)->updateScore();
+				iter++;
+			}
 		}
 		
 		// Re-sort the list...
@@ -270,6 +341,19 @@ void LLMediaDataClient::sortQueue()
 			}
 		}
 	}
+	
+	// Cull dead items from the round robin queue as well.
+	for(request_queue_t::iterator iter = mRoundRobinQueue.begin(); iter != mRoundRobinQueue.end();)
+	{
+		if(request_needs_purge(*iter))
+		{
+			iter = mRoundRobinQueue.erase(iter);
+		}
+		else
+		{
+			iter++;
+		}
+	}
 }
 
 // static
@@ -298,26 +382,14 @@ void LLMediaDataClient::serviceQueue()
 		llassert(!request.isNull());
 		const LLMediaDataClientObject *object = (request.isNull()) ? NULL : request->getObject();
 		llassert(NULL != object);
-		
+
 		// Check for conditions that would make us just pop and rapidly loop through
 		// the queue.
-		if(request.isNull() ||
-		   request->isMarkedSent() ||
-		   NULL == object ||
-		   object->isDead() ||
-		   !object->hasMedia())
+		if(request->isMarkedSent())
 		{
-			if (request.isNull()) 
-			{
-				LL_WARNS("LLMediaDataClient") << "Skipping NULL request" << LL_ENDL;
-			}
-			else {
-				LL_INFOS("LLMediaDataClient") << "Skipping : " << *request << " " 
-				<< ((request->isMarkedSent()) ? " request is marked sent" :
-					((NULL == object) ? " object is NULL " :
-					 ((object->isDead()) ? "object is dead" : 
-					  ((!object->hasMedia()) ? "object has no media!" : "BADNESS!")))) << LL_ENDL;
-			}
+			// Sent items that make it back to the head of the queue need to be skipped.
+			LL_INFOS("LLMediaDataClient") << "removing : " << *request << " request is marked sent" << LL_ENDL;
+			
 			queue_p->pop_front();
 			continue;	// jump back to the start of the quick retry loop
 		}
@@ -448,24 +520,15 @@ LLMediaDataClient::Responder::RetryTimer::RetryTimer(F32 time, Responder *mdr)
 {
 }
 
-// virtual 
-LLMediaDataClient::Responder::RetryTimer::~RetryTimer() 
+// virtual
+BOOL LLMediaDataClient::Responder::RetryTimer::tick()
 {
-	LL_DEBUGS("LLMediaDataClient") << "~RetryTimer" << *(mResponder->getRequest()) << LL_ENDL;
-	
-	// XXX This is weird: Instead of doing the work in tick()  (which re-schedules
-	// a timer, which might be risky), do it here, in the destructor.  Yes, it is very odd.
-	// Instead of retrying, we just put the request back onto the queue
 	LL_INFOS("LLMediaDataClient") << "RetryTimer fired for: " << *(mResponder->getRequest()) << " retrying" << LL_ENDL;
 	mResponder->getRequest()->reEnqueue();
 	
 	// Release the ref to the responder.
 	mResponder = NULL;
-}
 
-// virtual
-BOOL LLMediaDataClient::Responder::RetryTimer::tick()
-{
 	// Don't fire again
 	return TRUE;
 }
@@ -552,7 +615,7 @@ const char *LLMediaDataClient::Request::getTypeAsString() const
 }
 
 
-void LLMediaDataClient::Request::reEnqueue() const
+void LLMediaDataClient::Request::reEnqueue()
 {
 	// I sure hope this doesn't deref a bad pointer:
 	mMDC->enqueue(this);