301594 - Remove unneeded row allocation and such from 'BlackenFrame' in imgContainerGif

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

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.)

Comment 1

[Alfred Kayser]

Attachment: v1: Cleanup use of Get/SetImageData in BlackenFrame

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

Comment 2

[Alfred Kayser]

Attachment: Testdata: animated gif which uses BlackenFrame a lot

Comment 3

[Alfred Kayser]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Stuart Parmenter]

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

yes its fine

Comment 6

[Alfred Kayser]

Thanks!

Who can check this in the trunk for me?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Alfred Kayser]

Fresh patch coming up...

Comment 9

[Alfred Kayser]

Attachment: V2: updated to current trunk

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Alfred Kayser]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

This caused bug 315966.

Comment 13

[Alfred Kayser]

Verified!