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

VERIFIED FIXED

Status

()

VERIFIED FIXED
14 years ago
9 years ago

People

(Reporter: alfredkayser, Assigned: alfredkayser)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

14 years ago
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.)
(Assignee)

Comment 1

14 years ago
Created attachment 190031 [details] [diff] [review]
v1: Cleanup use of Get/SetImageData in BlackenFrame

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

Updated

14 years ago
Attachment #190031 - Flags: review?(pavlov)
(Assignee)

Comment 2

14 years ago
Created attachment 190230 [details]
Testdata: animated gif which uses BlackenFrame a lot
(Assignee)

Updated

14 years ago
Attachment #190031 - Flags: review?(pavlov) → review?(tor)

Updated

14 years ago
Attachment #190031 - Flags: review?(tor) → review+
(Assignee)

Comment 3

13 years ago
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.
Attachment #190031 - Flags: superreview?(dveditz)
(Assignee)

Updated

13 years ago
Attachment #190031 - Flags: superreview?(dveditz) → superreview?(stuart)
pav, ping?  Is that patch ok?  Is it wanted?

Comment 5

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

yes its fine
Attachment #190031 - Flags: superreview?(pavlov) → superreview+
(Assignee)

Comment 6

13 years ago
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?
(Assignee)

Comment 8

13 years ago
Fresh patch coming up...
(Assignee)

Comment 9

13 years ago
Created attachment 202469 [details] [diff] [review]
V2: updated to current trunk

Updated version to be committed to the trunk
(one change in V1 was allready committed in another bug)
Attachment #190031 - Attachment is obsolete: true
Assignee: pavlov → alfredkayser
Checked in.  Is that it for this bug, then?
(Assignee)

Comment 11

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Depends on: 315966
(Assignee)

Comment 13

13 years ago
Verified!
Status: RESOLVED → VERIFIED
(Assignee)

Updated

13 years ago
Blocks: 318699

Updated

9 years ago
QA Contact: imagelib
You need to log in before you can comment on or make changes to this bug.