Closed Bug 385883 Opened 17 years ago Closed 17 years ago

imgRequest wrongly calculates image size for the cache

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: alfredkayser, Unassigned)

References

Details

Attachments

(1 file)

In:
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/src/imgRequest.cpp#543
543     frame->GetImageDataLength(&imageSize);
544     frame->GetAlphaDataLength(&alphaSize);
545 
546     mCacheEntry->SetDataSize(cacheSize + imageSize + alphaSize);

The GetAlphaDataLength is wrong for Cairo, as the GetImageDataLength already includes the complete image size, including the alpha bytes.

The effect is that in about:cache the Data Size reported for each image twice the amount that it should be (width*height*8 instead of width*heigth*4).
This also means that the image cache is only half used.
Attachment #269816 - Flags: review?(pavlov)
Comment on attachment 269816 [details] [diff] [review]
Patch to remove the GetAlphaDatalength

mmm, good call.
Attachment #269816 - Flags: review?(pavlov) → review+
Attachment #269816 - Flags: superreview?(tor)
Severity: normal → major
Flags: blocking1.9?
Blocks: 367281
Attachment #269816 - Flags: superreview?(tor) → superreview+
Who can do the checkin?
Whiteboard: [checkin needed]
Checking in modules/libpr0n/src/imgRequest.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgRequest.cpp,v  <--  imgRequest.cpp
new revision: 1.93; previous revision: 1.92
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Looks like this caused about a 6% Tp win on Linux and a 4% Tp win on Mac! (Still waiting on Windows to cycle.)
Verified, in build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070628 Minefield/3.0a6pre, the sizes in about:cache are now correct for images cached in memory.
Status: RESOLVED → VERIFIED
Adam: What was the Tp gain on Windows?
There was about a 7% Tp win on Windows. However, after doing some investigation, it turns out that the win isn't as great as it sounds. See bug 296538, comment 22 and later.
These are tricky statistics. The overall summary is that with the same configuration settings the Tp is 7% improved. Before this patch the configuration says to use xxMB for cache, whilst only half of it was used. With this patch all of the specified xxMB's are used. Another way to improve the Tp is to increase the size of the cache, but then the configuration settings are not the same as before.
Note, that before Cairo was used, the cache was also fully used to the specified xxMB's, and that the Cairo change increased (initially) the Tp, of which we can now say that part of it was caused by the 'halving the image cache size'.
You need to log in before you can comment on or make changes to this bug.