[opensource-dev] Review Request: VWR-24321: Validate textures starting with 00 too.

Twisted Laws twisted_laws at hotmail.com
Wed Jan 19 10:32:45 PST 2011



> On Jan. 18, 2011, 12:09 p.m., Merov Linden wrote:
> > indra/newview/lltexturecache.cpp, line 1595
> > <http://codereview.secondlife.com/r/90/diff/1/?file=413#file413line1595>
> >
> >     validate_idx being used in a test later, it's not just for (validate_idx == 0) that the behavior will be different. I need to understand better what that idx is all about or you need to give a bit more explanation before I approve this diff.
> 
> Aleric Inglewood wrote:
>     The debug setting CacheValidateCounter is set to 'next_id', which makes clear what it's meaning is: namely, the id that we will check next time. next_idx is a very local variable that is simply set to the value of CacheValidateCounter plus 1, and then that value is stored to CacheValidateCounter again for next time.
>     
>     validate_idx is the ID that is actually being tested this time. Hence, it should be the value of CacheValidateCounter before we increase that.
>     
>     Obviously, those ID's should be in the range 0...255, which is garanteed by doing a modulo 256 on next_id before writing it to CacheValidateCounter.
>     
>     The old code also increases validate_idx *before* it is used. That means that it will be in the range 1...256, and 0 is never tested (note that validate_idx is just increased, there is no modulo applied, so it's obvious that it shouldn't be increased). Even the wording ("next"_id) suggests that validate_idx should just be the value of CacheValidateCounter, which is the value of the previous next_id...
>     
>     So, after this patch, we get to the following code with validate_idx in the range 0...255, as it should be.
>
> 
> Cron Stardust wrote:
>     Just double checking, as switching from pre-increment to addition can change behavior: In both cases next_idx will have the same value after this line is executed, however validate_idx will not.
>     
>     Prediff start state: validate_idx == 0;
>     Prediff stop state: validate_idx == 1; next_idx == 1;
>     
>     Postdiff start state: validate_idx == 0;
>     Postdiff stop state: validate_idx == 0; next_idx == 1;
>     
>     And another round over at the other end:
>     
>     Prediff start state: validate_idx == 255;
>     Prediff stop state: validate_idx == 256; next_idx == 0;
>     
>     Postdiff start state: validate_idx == 255;
>     Postdiff stop state: validate_idx == 255; next_idx == 0;
>     
>     So, yes, validate_idx will only have a 255 in it in this last case, however the over-all behavior has changed: validate_idx isn't being incremented at all in this line.
>     
>     WARNING: I have NOT looked at the rest of the diff, only at this one line.  Nor do I know if validate_idx should or shouldn't be incremented by this line of code.

Given the way this seems to work to me without changing the sequence things are checked...  
I'd change line 1619 to  if (uuididx == (validate_idx % 256))
Otherwise it was checking for 1 thru 256 and never 0...  this does not change what appears
to be incorrect coding where Aleric pointed out and thus won't change the current
logic that it starts by checking 1 thru 255 before checking 0.  To retain the current sequence
that things are checked, you would have to compare uuididx to next_idx along with the change
Aleric provided.

However it seems to me that all is ok with using Aleric's correction and leaving the remaining
code untouched.  (I can't see where changing the sequence of checking makes a difference.)


- Twisted


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/90/#review191
-----------------------------------------------------------


On Jan. 14, 2011, 1:02 p.m., Aleric Inglewood wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/90/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2011, 1:02 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> -------
> 
> Trivial patch, just removes stupidity.
> 
> 
> This addresses bug VWR-24321.
>     http://jira.secondlife.com/browse/VWR-24321
> 
> 
> Diffs
> -----
> 
>   doc/contributions.txt b0bd26c5638a 
>   indra/newview/lltexturecache.cpp b0bd26c5638a 
> 
> Diff: http://codereview.secondlife.com/r/90/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Aleric
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110119/c26c2890/attachment-0001.htm 


More information about the opensource-dev mailing list