[sldev] Bug in LLTextureCacheRemoteWorker::doRead ?
Philippe Bossut (Merov Linden)
merov at lindenlab.com
Wed Mar 25 22:31:37 PDT 2009
Hi Carlo,
How uncanny, it just happens that I'm starting to work on documenting
that piece of code. I'm still trying to get my bearing through the
various threads and mutex involved so I might be missing something but
I'll be giving my best shot at an explanation.
On Mar 25, 2009, at 8:48 PM, Carlo Wood wrote:
> I'm trying to understand the texture pipeline by
> reading the code (unfortunately there is no documentation).
Yup, this is coming (see above).
> Doing so, I ran into the following that appears to
> be a bug.
>
> The class LLTextureCacheRemoteWorker has an
> element called mOffset.
>
> Because of code like this:
>
> mState = mOffset < TEXTURE_CACHE_ENTRY_SIZE ? HEADER : BODY;
>
> it seems that mOffset is the data that was read so far.
mOffset is actually the data that's reserved for the header in the
formatted image buffer at creation *before* the readFromCache() is
invoked (see LLTextureFetchWorker::doWork() in lltexturefetch.cpp).
This quantity is fixed and never changed. What we've read so far is
*at most* TEXTURE_CACHE_ENTRY_SIZE from the header cache.
The cache system writes all the cached files "headers" in a special
file so to access the basic image info rapidly. Each entry to that
file is *fixed* and stores TEXTURE_CACHE_ENTRY_SIZE of data, header
*and all*. So, sometimes that entry actually stores real data (pixels
so to speak...) that needs to be read before we read the body. This is
what the mState == HEADER section will actually do. Sometimes, the
header is actually bigger than TEXTURE_CACHE_ENTRY_SIZE and part of
the header of the file will leak into the body section and will have
to be accounted for there. This is the reason of the mind boggling
code you encountered...
So, the idea in the line of code you mentioned here above is that, if
the header (mOffset) is smaller than TEXTURE_CACHE_ENTRY_SIZE, then
there is some data we'll have to get from here and that'll be done in
the mState == HEADER section of the doWork(). Otherwise, we go to the
BODY section directly.
Note that, in truth, the (mState == HEADER) section actually doesn't
read the header (this is done in getHeaderCacheEntry()) but rather
deals with the left over of body data in that buffer...
> Note that there are two sizes: data size (the whole image)
> and file size. Those two differ because the first
> TEXTURE_CACHE_ENTRY_SIZE bytes (600) are written to the
> texture.cache file and the remaining bytes are written
> to the cache/textures/?/* files.
>
> So again, I think that mOffset is the *data* that was read
> previously.
getHeaderCacheEntry() read TEXTURE_CACHE_ENTRY_SIZE bytes *at most*
that do truly belonged to the original file (and are not duplicated in
the other part of the cache) so it reads min(mOffset,
TEXTURE_CACHE_ENTRY_SIZE). When you realize that, the algorithm here
under makes sense (though it's really acrobatic I agree...)
> A bit further in LLTextureCacheRemoteWorker::doRead in
> we see the following code however:
>
> if (mState == BODY)
> {
> std::string filename = mCache->getTextureFileName(mID);
> S32 filesize = ll_apr_file_size(filename, mCache-
> >getFileAPRPool());
> S32 bytes_read = 0;
> if (filesize > mOffset)
> {
> S32 datasize = TEXTURE_CACHE_ENTRY_SIZE +
> filesize;
> mDataSize = llmin(datasize, mDataSize);
> S32 data_offset = TEXTURE_CACHE_ENTRY_SIZE -
> mOffset;
> data_offset = llmax(data_offset, 0);
> S32 file_size = mDataSize - data_offset;
> S32 file_offset = mOffset -
> TEXTURE_CACHE_ENTRY_SIZE;
> file_offset = llmax(file_offset, 0);
> ...
>
> Clearly, 'filesize' is the size of the "cache/textures/?/*" file
> (the BODY), and thus 'datasize' is TEXTURE_CACHE_ENTRY_SIZE +
> filesize.
>
> data_offset has mOffset subtracted, again indicating that mOffset is
> a DATA offset, not a file offset.
>
> The (remaining) file_size is then indeed mDataSize - data_offset,
> and 'file_offset' is set to 'mOffset - TEXTURE_CACHE_ENTRY_SIZE',
> again very clearly indicating that mOffset is the total (data) offset.
>
> Therefore... the ' if (filesize > mOffset) ' must be a bug!
Well, if (filesize <= mOffset), the file was all contained in the
header cache to start with and there's nothing else left to read. It
was all read in the getHeaderCacheEntry() call and transfered in the
relevant buffer in the (mState == HEADER) section.
If you execute the computation here above for the 2 cases (mOffset <=
TEXTURE_CACHE_ENTRY_SIZE) and (mOffset > TEXTURE_CACHE_ENTRY_SIZE) you
get:
if (mOffset <= TEXTURE_CACHE_ENTRY_SIZE)
file_size = filesize + mOffset
file_offset = 0
That's the "normal" case, the one where the header is small. We'll
read the whole body (we'll add the extra bytes coming from the header
cache in a memcpy() later), there's no offset to worry about and the
whole file is the addition of both header and data.
if (mOffset > TEXTURE_CACHE_ENTRY_SIZE)
file_size = filesize + TEXTURE_CACHE_ENTRY_SIZE
file_offset = mOffset - TEXTURE_CACHE_ENTRY_SIZE // a negative
value, allowing the reading of the remainder of the header
In that case, the header overflown the header cache so, in reality,
the file size is one max buffer (TEXTURE_CACHE_ENTRY_SIZE) plus the
body but the data is really starting at (header size -
TEXTURE_CACHE_ENTRY_SIZE).
If you ask me, that piece of code could be rewritten without loss of
performance or generality by simply spelling out the 2 cases as I just
did. At least, it's intent would be made plain and it'd be easier to
read... It might even make the absence of documentation less painful.
That being said, I might have missed something in my interpretation of
that code so may be someone else could jump in and show us what to
think of all this.
Cheers,
- Merov
More information about the SLDev
mailing list