Review: invalidation bug fix

Chasing some test failures that occurred after the recent mipmap/subimage changes, I found the following bug.  I think it's always been wrong, but only became symptomatic after that change.

The short story is that I was sloppy about sometimes using m_invalid, and other times m_subimages.size()>0 as tests for whether the specs were read and valid, but wasn't in fact ensuring that they always were in sync.  This caused some very subtle problems after files were invalidated, cascading into using a spec that was no longer valid memory.  This patch refactors a bit and fixes it up.  I believe it was a problem all along, but the memory was being reused "in place" before, so the values were still as expected.

The one trick to it all is that in eliminating the inconsistency, in invalidate() I no longer re-open and re-close the file.  I can't think of a good reason to do that now, but obviously at some point I thought it was necessary.  We pass all tests (including OSL and Arnold testsuites), so I don't think this breaks anything, but I want you reviewers to think hard about whether I'm forgetting something.

[ckulla, 2010-12-02 17:09:13]

http://codereview.appspot.com/3404041/diff/1/src/libtexture/imagecache.cpp
File src/libtexture/imagecache.cpp (right):

http://codereview.appspot.com/3404041/diff/1/src/libtexture/imagecache.cpp#ne...
src/libtexture/imagecache.cpp:364: // If m_subimages has already been filled
out, we've opened this file
Should this comment be updated?

http://codereview.appspot.com/3404041/diff/1/src/libtexture/imagecache.cpp#ne...
src/libtexture/imagecache.cpp:885: // FIXME -- why do we need to reopen here? 
Why reload the spec?
What if the file changes on disk? (new resolution for example?)

[larrygritz, 2010-12-02 17:47:36]

http://codereview.appspot.com/3404041/diff/1/src/libtexture/imagecache.cpp
File src/libtexture/imagecache.cpp (right):

http://codereview.appspot.com/3404041/diff/1/src/libtexture/imagecache.cpp#ne...
src/libtexture/imagecache.cpp:364: // If m_subimages has already been filled
out, we've opened this file
On 2010/12/02 17:09:14, ckulla wrote:
> Should this comment be updated?

Will do.

http://codereview.appspot.com/3404041/diff/1/src/libtexture/imagecache.cpp#ne...
src/libtexture/imagecache.cpp:885: // FIXME -- why do we need to reopen here? 
Why reload the spec?
On 2010/12/02 17:09:14, ckulla wrote:
> What if the file changes on disk? (new resolution for example?)

Yeah, I think that's the point.  Clearing validspec here is what ensures that
next time we need something from the file, we will read the header again.  It
will need to open() it at that point, which is how the spec gets filled in and
marked as valid.