VWR-25923 Unnecessary capability request spam


Submitter:	Jonathan Yap	Reviewers
Branch:	2.7	Groups:	viewer
Bugs:	VWR-25923	People:
Change Number:	None	Repository:	viewer-development

Description:

This is a patch by ArminWeatherHax.  I am creating the request to help speed this fix along in the system.

----

Ways to reproduce: log into a simulator.
Reproduces: always
Affects: any version supported, probably all 3rd party viewers but Kokua (and Imprudence, soon).

What happens:
In each idle cycle the voice client requests the "ParcelVoiceInfoRequest" capability, thats each time a HTTP GET.
See LLVivoxVoiceClient::stateMachine() after comment // Check for parcel boundary crossing

Expected:
On parcel/region change request the capability once. It's not the region that rezzes in, but the avatar, so do the request for the capability not earlier than the agents region signals capabilitiesReceived() true. After that you are sure if the region returns an empty url you can give up for that region.

Not sure about the impact on lag - requesting and returning an url is not much data transmitted, though its a pretty big number of people doing it over and over per second (no matter if they have voice on or off).


----

going once again through llviewerregion I see its fortunately not each time a HTTP GET, just once when the agent connects to the region. Though the patch still saves all the lookup if the cap is there while it can't be possibly.

Testing Done:

Expand All

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

I think is better put a comment (generic too) here about the meaning and usage of mRegionHasVoice

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

not sure is a nice choice remove the warn, a resident lose the only way to know if all work fine

ArminWeatherHax Resident June 21, 2011, 2:51 a.m. (June 21, 2011, 2:51 a.m.)

I'll add a comment and rename mRegionHasVoice to mRegionHasVoiceCap to be more clear, and add a warning when the cap response arrived with empty ParcelVoiceInfoRequest.

Seems to work for me, although I'd appreciate if you used a more straightforward (=less error prone) logic.

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

Inconsistent spacing around "if" conditions.

Please use "if (cond)" with no extra or missing spaces.

This also applies to other changes in the patch.

ArminWeatherHax Resident June 21, 2011, 10:10 a.m. (June 21, 2011, 10:10 a.m.)
```
"if[space](cond)" - is that right?
```
Vadim ProductEngine June 21, 2011, 10:38 a.m. (June 21, 2011, 10:38 a.m.)
```
Yes.
```

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

It's unclear to me why you set voice to enabled here.
I think it should be set to enabled only when we're sure the parcel supports it.

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

What if region caps never arrive?
The string may grow infinitely.

Also I don't like using region name as a marked that region caps haven't arrived yet -- that's a hack.

ArminWeatherHax Resident June 21, 2011, 2:29 a.m. (June 21, 2011, 2:29 a.m.)

The state engine resets the string if it was changed, but you are right, it's not nice. I'll go for adding LLVivoxVoiceClient::parcelChanged() to LLViewerParcelMgr::addAgentParcelChangedCallback which will make the whole patch more straightforward.

Vadim ProductEngine June 21, 2011, 8:22 a.m. (June 21, 2011, 8:22 a.m.)

Sounds good.

BTW, while working on Windlight Region Settings I added a "capabilities received" signal in LLViewerRegion, which might be of use here as well. That code hasn't been integrated yet, but if you want I can send you the patch.

ArminWeatherHax Resident June 21, 2011, 9:44 a.m. (June 21, 2011, 9:44 a.m.)

Currently I have a new state which rechecks capabilitiesReceived until true,  and was about to add a comment to it that a callback would be much better - so yes please about sending that patch :)

Vadim ProductEngine June 21, 2011, 10:49 a.m. (June 21, 2011, 10:49 a.m.)
```
Sent.
```

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

This could be written shorter:
mRegionHasVoice = enabled;

I pushed a new version of the patch to https://bitbucket.org/ArminW/vwr-25923
Its 2 commits: 
* first Vadims patch
* then my patch addressing the reviews here

Diff
- added Diff r2

indra/newview/llvoicevivox.h (Diff revisions 1 - 2)

Very good idea to explain the semantics of this field in detail, as the name alone doesn't give it away fully. :-)

indra/newview/llvoicevivox.cpp (Diff revisions 1 - 2)

These are two sentences. Use a period instead of a comma.

indra/newview/llvoicevivox.cpp (Diff revisions 1 - 2)

requestParcelInfo() might be a more natural name than using 'do'.

indra/newview/llvoicevivox.h (Diff revision 2)

Not a native speaker myself, but I don't think 'keep s.th. staying in <a state>' is proper English. Maybe either write
	// Setting mRegionHasVoiceCaps to false makes the voice client stay in stateDisabled.
or simply
	// Setting mRegionHasVoiceCaps to false keeps the voice client in stateDisabled.

indra/newview/llvoicevivox.h (Diff revision 2)

This sentence confused me for some reason. I guess 'allows to leave' would be clearer than 'allows escaping'.

indra/newview/llvoicevivox.cpp (Diff revision 2)

I think this should be "changed region before the region cap response for the previous region arrived" to be clear.

indra/newview/llvoicevivox.cpp (Diff revision 2)

https://wiki.secondlife.com/wiki/Coding_standard#Naming_Convention says local variables should be lower_case, not camelCase so make this parcel_local_ID.

indra/newview/llvoicevivox.cpp (Diff revision 2)

What does the first 'it' refer to? "ID with" what?

indra/newview/llvoicevivox.cpp (Diff revision 2)

Why not directly
		mCurrentRegion = gAgent.getRegion();
and then use mCurrentRegion in the rest of this method?

indra/newview/llvoicevivox.cpp (Diff revision 2)

Here, too, either just 'keeps' without 'staying' or 'makes [...] stay', I think.

No major objections now, thanks.

When I applied this latest patch to firestorm, which is based off of LL 2.5.2, I kept getting timed out when TPing from a mainland area and a DD of 256 to other regions.  I tried multiple times TPing from the area around help island public to magnum sandbox 4 or to help people island.  In every case I timed and was logged off after the TP was completed, as the viewer was cleaning up and closing connections to the old regions.  I don't have this problem with the first iteration of this patch.  I feel this scenario needs to be tested with this latest patch on v-dev tip to make sure this doesn't happen there as well.

The account I used only has 4 friends, and around 24 items in the inventory in addition to the library.  It also only has 2 attachments (a hud and the firestorm bridge) and is one of the noob outfits.  I also am running on an i7 2600k, 16GB ram, ATI 6970, and 20/5mbit FIOS connection.

Vadim ProductEngine June 28, 2011, 6:27 a.m. (June 28, 2011, 6:27 a.m.)

Could you please provide exact steps to reproduce the issue?
Have you tried reverting the patch and repeating the teleport attempt?

Tankmaster Finesmith June 28, 2011, 10:55 a.m. (June 28, 2011, 10:55 a.m.)

I TPed to Help Island Public, which is on maind land, while running from Visual Studio.  Draw distance set to 256.  Flew over orentation island and proceeded onto another neighboring sim.  My viewer is now connected to several different regions do to normal viewer behavior.  I then TP to magnum sandbox 4 by tyeping that into the nav bar up top and pressing enter.  

First tried with the first patch, which we have had for several weeks now in our repository:
-TP was successful, then the viewer became rather unresponsive and the logs say things related to "(ip address) time out exceeded, disconnecting" but eventually recovers
-repeated 3 times, last time I TPed to Help People Island instead and all times got the same result

Second I tried with the updated patch:
-TP was successful, then the viewer was completely unresponsive, and I eventually got the ding from getting logged off and in the loges I see a mass of "invalid packet from (IP address)"
-repeated 3 times like and to the same regions (twice to magnum, once to HPI) every time I got logged off

Third I reverted back to the first patch:
Same experience as the first:
-TP was successful, then the viewer became rather unresponsive and the logs say things related to "(ip address) time out exceeded, disconnecting" but eventually recovers
-repeated 3 times, last time I TPed to HPI as I have in the other tests, and all times got the same result 


In talking with armin, he got similer results but thinks its the magnum sandbox region that may be having problems.  I am not as sure on that as I get it when TPing to HPI as well which is release server channle and not a RC channle.

Vadim ProductEngine June 29, 2011, 5:20 a.m. (June 29, 2011, 5:20 a.m.)

Thanks, Tankmaster. I tend to think that these errors are not related to the patch.

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.