As a music fan, I want audio to fade in gently so my immersion is increased


Submitter:	Jonathan Yap	Reviewers
Branch:	v3.2.1	Groups:	viewer
Bugs:	STORM-591	People:
Change Number:	None	Repository:	viewer-development

Description:

Audio fading in has been added for when a music stream starts. Audio fading out has been added for when there is a change in the music stream that is playing.

Two dead files have been eliminated.

An existing bug in how music was not being paused correctly has been fixed.

When you are teleporting you will hear the music stream from the place you are leaving. This is a change in behavior; previously the music stream was stopped when the teleport progress bar was being displayed.

This code change affects several areas where music is started or stopped. The new code has evolved significantly, so please look for things that might not "make sense."

Testing Done:

See the massive test plan in the jira.

Expand All

A couple of minor suggestions, but generally very clear and clean code.

Kudos for good use of the singleton pattern.

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

Please fix (and in the else below) to match the coding standard; add the braces.
https://wiki.secondlife.com/wiki/Coding_Standard#Braces

Jonathan Yap Dec. 2, 2011, 7:52 a.m. (Dec. 2, 2011, 7:52 a.m.)
```
Done.
```

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

I think this would be clearer if it were written as a switch statement without the early returns.

Jonathan Yap Dec. 2, 2011, 7:52 a.m. (Dec. 2, 2011, 7:52 a.m.)
```
Done.
```

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

If you add an 'else' here, you can replace all the early returns in this method with assignments to a 'bool fadeIsFinished = false;' variable and return that at the bottom.

Jonathan Yap Dec. 2, 2011, 7:52 a.m. (Dec. 2, 2011, 7:52 a.m.)
```
Done, but not by adding an else.
```

Quick comment regarding behavior...we really should not play any audio during the teleport blackout as it is disconcerting to feel like you are still in an area (hearing the audio) but not be able to see or interact with anything.  Everything else sounds great.

Jonathan Yap Nov. 23, 2011, 8:41 a.m. (Nov. 23, 2011, 8:41 a.m.)

The reason I changed the behavior when the teleport progress bar was showing was to keep the state engine logic simple -- it activates when there is change in your current parcel music URL.

Otherwise you have to deal with various cases as exceptions to how the state engine processes stream transitions.

If you force-fade the current stream when the teleport progress bar appears and the teleport fails you'll have to fade the stream back in.  I am not sure if there are other edge cases that might appear.

I suggest trying Oz's PO test viewer (there is a download link in a jira comment).  Personally I felt having the music keep playing and then fade out and in when you arrive was more enjoyable (you can consider it in-flight entertainment).  This also might tie in well with any improvements to the progress bar screen no longer being a blank page as was discussed in one of Oz's user group meetings.

Oz Linden Nov. 23, 2011, 9:32 a.m. (Nov. 23, 2011, 9:32 a.m.)

On the other hand, testing in the current Development viewer [Second Life 3.2.4 (245662)], when I started from a parcel with a stream and TP'ed to a new region&parcel without one, I got a sudden cutoff when the TP screen came up but the stream restarted when the screen dropped and then cut off quickly (presumably because new parcel properties had arrived).  That's arguably not as good as a fade after teleport.

Richard Nelson Nov. 28, 2011, 1:26 p.m. (Nov. 28, 2011, 1:26 p.m.)

One of the in-progress changes to the teleport/log in screen would involve playing video+audio on that login screen, which should not be fighting with background audio.

Looks good. Nice job overall bringing the pieces together and excellent test plan.

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

Minor point - maybe a warning where you define the fade in/out times that they should not be zero, even for testing.

Jonathan Yap Dec. 2, 2011, 2:36 a.m. (Dec. 2, 2011, 2:36 a.m.)

Good catch!  In code that is now removed I was not letting this value go below zero.  I've added a bit of code to prevent it from dropping below 0.01 as a preventative measure against future changes or TPVs who pick this up and add back the two debug settings.

Ship It!

Diff
- added Diff r2

Description:

New diff based on requested code review changes.

Diff
- added Diff r3

Description:

Made changes per code review requests:

Took care of case where division by zero is possible
Removed early returns
Music does not play when the teleport progress bar is displayed

Also made a change so that the fade in timer during login is reset until STATE_STARTED is reached.  This helps fix not hearing a full fade in during login.

status changed from pending to submitted

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.