(STORM-380) There is a little delay in sound when gesture first time played


Submitter:	Seth ProductEngine	Reviewers
Branch:		Groups:	viewer
Bugs:	STORM-380	People:
Change Number:	None	Repository:	viewer-development

Description:

Added syncing animations and sounds before the gesture starts playing.
The actual playing of animations and sounds of a gesture starts only when all needed animations and sound files are loaded into viewer cache. This reduces the delay between animations and sounds meant to be played simultaneously but may increase the delay between the moment a gesture is triggered and the moment it starts playing.

Fixed calling assets callback to clean up the void pointer in getAssetData() and avoid potential memory leaks.

Testing Done:

Expand All

Ah, a good issue to tackle! Thanks for looking into it.

indra/newview/llgesturemgr.h (Diff revision 1)

Please use tabs for indentation and spaces for alignment. (Here, that'd be only one tab followed by 32 spaces.) Whatever you did here looks wrong whether I set tab display length to 4 or 8.

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)
```
Corrected indentation here and for the method declared above.
```

indra/newview/llgesturemgr.cpp (Diff revision 1)

If the iterator will not be used outside the loop, please declare it with the assignment in the for's braces.

Oz Linden March 28, 2011, 3:58 a.m. (March 28, 2011, 3:58 a.m.)
```
I don't see any reason to make that distinction.
```

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)

Seems like it may look confusing if the iterator is not used outside the loop, so moved its declaration inside braces.

indra/newview/llgesturemgr.cpp (Diff revision 1)

I'm not sure what you mean by "if it stops". Maybe that the currently handled gesture step stops the animation? If so, maybe write

// Don't request the animation if this step stops it or if it is already in Static VFS

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)
```
Corrected.
```

indra/newview/llgesturemgr.cpp (Diff revision 1)

Why are STEP_CHAT and STEP_WAIT mentioned when they don't do anything and we do have a default step that also doesn't do anything?

If all cases are supposed to be explicitly handled, we might want to make sure of that with:
		// [...]
		case STEP_CHAT:
		case STEP_WAIT:
			{
				break;
			}
		default:
			{
				// We should never get here.
				llassert(false);
			}
		}

(... or maybe just some terminal output.)

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)

Added the STEP_EOF for a complete list of step types and a warning for the default case.

indra/newview/llgesturemgr.cpp (Diff revision 1)

s/is/if/

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)
```
Changed "is: to "if". Is that correct?
```
Boroondas Gupte March 28, 2011, 12:57 p.m. (March 28, 2011, 12:57 p.m.)
```
Correct.
```

indra/newview/llgesturemgr.cpp (Diff revision 1)

Hmm ... so with the current change, a gesture's playback could potentially be delayed forever due to assets for other gestures being loaded?

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)

For now starting each new gesture resets the list of pending assets downloads so if the last started gesture's asset are loaded successfully the playback will continue. This should probably need a better way to handle the cases of some assets not being loaded due to some kind of error. The current behavior is not suppressing the gesture playback due to any data loading delays. Should we follow this behavior?

Boroondas Gupte March 28, 2011, 3:50 p.m. (March 28, 2011, 3:50 p.m.)

Ah, I thought assets would only be removed from the list when they finished loading. If it were so, someone always triggering a new gesture (with new assets) before all previously queued assets have been loaded could thus prevent the list from becoming empty and thereby delay playback of all gestures forever, even although assets for most gestures would already be available. This could happen even in the case of no assets failing to load.

Emptying the list (is that line 532 in the new indra/newview/llgesturemgr.cpp ?) when a new gesture comes in avoids this. Off course it isn't really the Right Thing To Do(TM), but if the behavior will in no case be worse than the current code, we can go for this until pending downloads are checked separately for each queued gesture. The latter can probably be achieved by maintaining a separate list of pending assets for each gesture and start playing a gesture when its own list has become empty.

Seth ProductEngine March 29, 2011, 10:38 a.m. (March 29, 2011, 10:38 a.m.)

Yes, it's line 532 but probably there should be a loading assets map that holds assets for each gesture.

indra/newview/llgesturemgr.cpp (Diff revision 1)

Can a gesture reference other asset types than animations and sounds? And if it can, shouldn't those be removed from mLoadingAssets, too, once loaded?

If it can but shouldn't maybe some terminal output would be appropriate here.

Seth ProductEngine March 28, 2011, 12:09 p.m. (March 28, 2011, 12:09 p.m.)

It shouldn't reference other asset types and there are specific callback procedures to handle animation and sound assets so added an assert for the default case.

Boroondas Gupte March 28, 2011, 3:57 p.m. (March 28, 2011, 3:57 p.m.)

Is this already checked somewhere before here, or could a bogus gesture trigger that assert? If the latter, a clean-up step and some terminal output would be friendlier than an assert, as we don't want the ability to 'shoot down' a Viewer by providing invalid data from remote.

Seth ProductEngine March 29, 2011, 10:38 a.m. (March 29, 2011, 10:38 a.m.)

That assert could only be triggered by requesting an asset type other that animation or sound with onAssetLoadComplete() callback, not by the invalid data from server. This callback can only handle those two types, for other types we don't know how to clean up a void pointer to user_data arg.

indra/newview/llgesturemgr.cpp (Diff revision 1)

I think that there's a race condition here.  

Wouldn't it be better to insert the id into the mLoadingAssets before requesting it, so that it is sure to be in the list if the onAssetLoadComplete is called immediately?

indra/newview/llgesturemgr.cpp (Diff revision 1)

Same possible race as above

Seth ProductEngine March 28, 2011, 12:15 p.m. (March 28, 2011, 12:15 p.m.)

Moved inserting the id into the mLoadingAssets prior to requesting asset data.

The getAssetData() request could potentially lead to memory leaks in some cases. Should this be a separate jira?

Diff
- added Diff r2

Description:

Fixes according to the feedback on revision 1.

Should be fine then, as far as I can tell.

Diff
- added Diff r3

Description

Changed from:

First pass implementation of syncing the animations and sounds before the gesture starts playing.
The actual playing of animations and sounds of a gesture starts only when all needed animations and sound files are loaded into viewer cache. This reduces the delay between animations and sounds meant to be played simultaneously but may increase the delay between the moment a gesture is triggered and the moment it starts playing.

Changed to:

Added syncing animations and sounds before the gesture starts playing.
The actual playing of animations and sounds of a gesture starts only when all needed animations and sound files are loaded into viewer cache. This reduces the delay between animations and sounds meant to be played simultaneously but may increase the delay between the moment a gesture is triggered and the moment it starts playing.

Fixed calling assets callback to clean up the void pointer in getAssetData() and avoid potential memory leaks.

Description:

- Fixed checking the pending downloads for each playing gesture separately.
- Fixed calling assets callback to clean up the void pointer in getAssetData() and avoid potential memory leaks.

I'm a bit worried about the loop-nesting. E.g.

indra/newview/llgesturemgr.cpp (Diff revisions 2 - 3)

in update, stepGesture is called in a loop

indra/newview/llgesturemgr.cpp (Diff revisions 2 - 3)

stepGesture calls hasLoadingAssets

indra/newview/llgesturemgr.cpp (Diff revisions 2 - 3)

hasLoadingAssets calls set::find in a loop (which is logarithmic in the set's size)

so we have N * M * log(L) behavior for update(). This might not be too bad, as the number of queued gestures N, the number of steps per gesture M and the number of loading assets L are probably all small most of the time, and the inner loop is exited via return once the first match has been found. But I still wonder whether it might pay off to e.g. make the gestures observers of their pending assets.

Seth ProductEngine April 21, 2011, 3:07 p.m. (April 21, 2011, 3:07 p.m.)

Looks like making gestures observers of their pending assets will not allow to remove the nested loops completely. When an asset is loaded we still have to iterate through all the gestures for which this asset had been requested and remove it from each gesture's pending asset list. This also increases the memory usage because we store separate lists of assets which may have duplicates.

Also making gesture an observer adds the need in tracking its lifetime. Deriving LLMultiGesture from boost::signals2::trackable for that purpose will require LLAssetStorage refactoring to use boost::signals2 for connecting the callbacks.

indra/newview/llgesturemgr.h (Diff revision 3)

Shouldn't this be a (non-static) method of LLMulitGesture taking no argument, rather than a static member of LLGestureMgr taking a pointer to a gesture?

Also, the gesture should not be changed by that, should it? If possible, make the argument type pointer-to-const (or, if you make it a method of LLMultiGesture as suggested, make it a const method).

Seth ProductEngine April 21, 2011, 3:07 p.m. (April 21, 2011, 3:07 p.m.)

It could be a non-static method of LLMulitGesture if a gesture would be an observer of its own pending assets, or if a gesture would check pending assets from the list stored at LLGestureMgr. In this case the LLMulitGesture class becomes more complicated without any gain in performance because this is the same check that LLGestureMgr::hasLoadingAssets() does.

Boroondas Gupte April 22, 2011, 2:04 a.m. (April 22, 2011, 2:04 a.m.)
```
OK. Could you still consider to pass a pointer-to-const?
```

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.