Closed Bug 317748 Opened 16 years ago Closed 15 years ago

Merge BlackenFrame and SetMaskVisibility into ClearFrame

Categories

(Core :: ImageLib, defect)

defect
Not set
minor

Tracking

()

VERIFIED FIXED
mozilla1.9alpha6

People

(Reporter: alfredkayser, Assigned: alfredkayser)

References

Details

(Keywords: memory-footprint)

Attachments

(1 file, 9 obsolete files)

Minor callpath optimisation, for less code and more clarity:
Replace the two functions:
   'BlackenFrame(aFrame);SetMaskVisibility(aFrame, PR_FALSE);'
with one function:
   'ClearFrame(aFrame);'

Before:
  16.448 imgContainerGIF.obj
  39.630 imggif_s.lib
After:
  16.181 imgContainerGIF.obj
  39.230 imggif_s.lib
Saving: about 400 bytes
Attachment #204150 - Flags: review? → review?(pavlov)
When in Cairo, a little more that can be achieved, so a new patch is in order.
(all SetMask/CombineMask/etc stuff is not needed for Cairo).
Comment on attachment 204150 [details] [diff] [review]
Combine BlackenFrame and SetMaskVisibility into ClearFrame

this should be updated to the trunk.  sorry for taking so long to look at this.
Attachment #204150 - Attachment is obsolete: true
Attachment #204150 - Flags: review?(pavlov)
Also remove the SetMaskVisibility and BuildCompositeMask stuff, which is not needed for Cairo
Assignee: pavlov → alfredkayser
Status: NEW → ASSIGNED
Attachment #253225 - Flags: review?(pavlov)
This then also 'fixes' bug 192790.
Blocks: 192790
Attachment #253225 - Attachment is obsolete: true
Attachment #253225 - Flags: review?(pavlov)
Attachment #253370 - Flags: review?(pavlov)
Some statistics:
Directory of C:\mozilla\suite-opt\modules\libpr0n\decoders\gif
28-02-2007  22:22            72.024 imggif_s.lib

Directory of C:\mozilla\suite-opt\modules\libpr0n\decoders\gif\old
28-02-2007  22:16            84.272 imggif_s.lib

So more than 12K code savings, with a simple patch!
Attachment #253370 - Attachment is obsolete: true
Attachment #256837 - Flags: review?(pavlov)
Attachment #253370 - Flags: review?(pavlov)
Comment on attachment 256837 [details] [diff] [review]
V4: updated to current tree (after Cairo cleanup)

This is a patch!

Some explanation:
Make ClearFrame also clear the alpha byte. 
Remove BuildCompositeMask as it is no longer needed because of Cairo.
Attachment #256837 - Attachment is patch: true
Attachment #256837 - Attachment mime type: application/octet-stream → text/plain
Keywords: footprint
I think this is probably fine but i'd like to wait until apng is in so andrew doesn't have any more merge conflicts he has to deal with.
ok, APNG is in the tree.  Can you put together an updated patch?
Attached patch v5: Patch imgContainer (obsolete) — Splinter Review
Note, also tested with APNG's from http://littlesvr.ca/apng/
Attachment #256837 - Attachment is obsolete: true
Attachment #260575 - Flags: review?(pavlov)
Attachment #256837 - Flags: review?(pavlov)
Comment on attachment 260575 [details] [diff] [review]
v5: Patch imgContainer

Rather than doing a memset() I think we'd be better off using the gfxContext code that was there before.  Probably worth also setting the operator to OPERATOR_CLEAR.  This allows is potentially much faster for some surfaces and is probably a bit slower for image surfaces.
Attachment #260575 - Flags: review?(pavlov) → review-
Attachment #260575 - Attachment is obsolete: true
Attachment #261117 - Flags: review?(pavlov)
Attachment #261117 - Flags: review?(pavlov) → review+
Attachment #261117 - Flags: superreview?(tor)
Comment on attachment 261117 [details] [diff] [review]
v6: Use Cairo and OPERATOR_CLEAR to clear the frame

>+void imgContainer::ClearFrame(gfxIImageFrame *aFrame, nsIntRect &aRect)
...
>+  // Erase the destination rectangle to transparent
>   nsRefPtr<gfxContext> ctx = new gfxContext(surf);
>-  ctx->SetColor(gfxRGBA(0, 0, 0));
>-  ctx->Rectangle(gfxRect(aX, aY, aWidth, aHeight));
>+  ctx->SetOperator(gfxContext::OPERATOR_CLEAR);
>+  ctx->SetColor(gfxRGBA(0, 0, 0, 0));
>+  ctx->Rectangle(gfxRect(aRect.x, aRect.y, aRect.width, aRect.height));
>   ctx->Fill();

We don't need SetColor for OPERATOR_CLEAR, do we?  With that change, sr=tor.
Attachment #261117 - Flags: superreview?(tor) → superreview+
Attachment #261117 - Attachment is obsolete: true
Who can do the checkin for me? Thanks in advance, Alfred
Whiteboard: [checkin needed]
mozilla/modules/libpr0n/src/imgContainer.cpp  1.44
mozilla/modules/libpr0n/src/imgContainer.h    1.22
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha4
Backed out due to tree bustage:

nsGIFDecoder2.cpp
/builds/slave/trunk_osx/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp: In static member function 'static int nsGIFDecoder2::HaveDecodedRow(void*, PRUint8*, int, int, int)':
/builds/slave/trunk_osx/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp:479: error: 'BlackenFrame' is not a member of 'imgContainer'

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp&rev=1.74&mark=479#479
this got added before your last patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch V8: Also fix nsGIFDecoder (obsolete) — Splinter Review
Attachment #261361 - Attachment is obsolete: true
Does this need a re-review or can it be landed?
Comment on attachment 262889 [details] [diff] [review]
V8: Also fix nsGIFDecoder

Tor, can you sr the part for nsGIFDecoder2.cpp?
Attachment #262889 - Flags: superreview?(tor)
Comment on attachment 262889 [details] [diff] [review]
V8: Also fix nsGIFDecoder

>--- modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp	20 Mar 2007 23:56:49 -0000	1.74
>+++ modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp	26 Apr 2007 14:12:07 -0000
>@@ -470,19 +470,17 @@ int nsGIFDecoder2::HaveDecodedRow(
...
>     if (!cmap) { // cmap could have null value if the global color table flag is 0
>-      for (int i = 0; i < aDuplicateCount; ++i) {
>-        imgContainer::BlackenFrame(decoder->mImageFrame, 0, aRowNumber+i, width, 1);
>-      }
>+      imgContainer::ClearFrame(decoder->mImageFrame, 0, aRowNumber+i, width, aDuplicateCount);

This won't build, due to "i" being undefined and the only ClearFrame methods in imgContainer accepting one or two arguments.
Attachment #262889 - Flags: superreview?(tor) → superreview-
This one should be right.
Compiled and tested against my library of test agif's.

Note, bug 376446 seems to fixed by the patch as well.
My guess would be that the 'BuildCompositeMask' function doesn't work well in Cairo...
Attachment #262889 - Attachment is obsolete: true
Attachment #263343 - Flags: superreview?
Blocks: 376446
Note, the following image is not animated right in current builds.
http://littlesvr.ca/apng/images/lion-rgb-3frame.png
With this patch applied it works ok.

(The second frame should blend with the first, but it stripes the old frame, indicating a faulty BuildCompositeMask).

More testcases are at: http://littlesvr.ca/apng/
alfred, did you mean to actually request sr from tor on your v9 patch? :-)
Also, I should note that I've had the v9 patch in my local tree since it was posted and haven't noticed any visible regressions in the near month it's been there.
Blocks: 382764
Attachment #263343 - Flags: superreview? → superreview?(tor)
Confirmed with my local build with this patch applied that bug 382764 is fixed by this patch.
(In reply to comment #23)
> Created an attachment (id=263343) [details]
> V9: The right patch to nsGIFDecoder2.cpp

One small note: in imgContainer::ClearFrame and imgContainerGIF::BlackenFrame,  you can stack allocate the gfxContext rather than using nsRefPtr/new.
Note, there are great many other places with gfxContext could be placed on the stack...
Attachment #263343 - Attachment is obsolete: true
Attachment #267161 - Flags: superreview?(tor)
Attachment #263343 - Flags: superreview?(tor)
Sounds like a followup bug to me
Created bug 383166 for the followup on stack allocation of gfxContext.
Attachment #267161 - Flags: superreview?(tor) → superreview+
Who can do the checkin for me?
Whiteboard: [checkin needed]
Blocks: 196295
Checked in for Alfred.

Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: mozilla1.9alpha4 → mozilla1.9alpha6
Flags: in-testsuite-
Thanks!

Tinderboxes all green:
 firefox-bin
  	Total:	       -944 (+448/-1392)
  	Code:	       -941 (+0/+0)
  	Data:	         -3 (+448/-1392)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.