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

[Aleric Inglewood]

> 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.
> 
> Twisted Laws wrote:
>     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.)
> 
> Boroondas Gupte 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.
>     
>     That's the intention. Without the change suggested here, validate_idx ends up in the wrong range. See my more detailed review below.

@Cron Stardust : That is correct. The bug was that validate_idx was 1 too large. Nor incrementing it is the correct thing.
Re the 'WARNING': this line is the only line in the diff :p


- Aleric


-----------------------------------------------------------
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/42008055/attachment.htm