[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