In imgContainerGif::BlackFrame(nsRect...) the following happens: PRUint8* tmpRow = NS_STATIC_CAST(PRUint8*, nsMemory::Alloc(bprToWrite)); if (!tmpRow) { aFrame->UnlockImageData(); return; } memset(tmpRow, 0, bprToWrite); for (PRInt32 y = 0; y < height; y++) { aFrame->SetImageData(tmpRow, bprToWrite, ((y + aY) * bpr) + xOffset); } nsMemory::Free(tmpRow); But when you pass 'nsnull' as first arg, to SetImageData, it also zero's the image data, so the above can be replaced with: for (PRInt32 y = 0; y < height; y++) { aFrame->SetImageData(tmpRow, bprToWrite, ((y + aY) * bpr) + xOffset); } Same applies in the other BlackenFrame() for the whole frame, the following does the whole trick of blacken the frame: + aFrame->GetImageDataLength(&aDataLength); + aFrame->SetImageData(nsnull, aDataLength, 0); Thirdly, by first checking width and height before locking the pixels, they don't have to be unlocked in case of width,heigth<0. Attaching patch to show how this works. This patch compiles, runs and is tested with a wide range of animated GIF's. In summary, less code, less MALLOC's, and cleaner use of SetImageData (no more GetImageData and 'ImageUpdated' stuff) in BlackenFrame. P.s. CopyImageData should move to gfxFrame, because it is just like DrawTo (but without the nsRect arg.)

Note, as SetImageData does also the pixel locking, it is not needed here

Comment on attachment 190031 [details] [diff] [review] v1: Cleanup use of Get/SetImageData in BlackenFrame Small patch to remove memory allocation and redundant code, just like bug 300936.

pav, ping? Is that patch ok? Is it wanted?

Comment on attachment 190031 [details] [diff] [review] v1: Cleanup use of Get/SetImageData in BlackenFrame yes its fine

Thanks! Who can check this in the trunk for me?

I could if I could get it to apply... it doesn't seem to, to current trunk. Care to update it?

Fresh patch coming up...

Updated version to be committed to the trunk (one change in V1 was allready committed in another bug)

Checked in. Is that it for this bug, then?

Tinderbox reports: libimglib2.so Total: -336 (+11/-347) Code: -347 (+0/+0) Data: +11 (+11/-347) -336 (+11/-347) text (DATA) -336 (+11/-347) UNDEF:libimglib2.so:text +11 .nosyms.text -16 imgContainerGIF::SetMaskVisibility(gfxIImageFrame*, int, int, int, int, int) -107 imgContainerGIF::BlackenFrame(gfxIImageFrame*, int, int, int, int) -224 imgContainerGIF::BlackenFrame(gfxIImageFrame*) With this report, I declare this bug as FIXED

This caused bug 315966.

Verified!