317748 - Merge BlackenFrame and SetMaskVisibility into ClearFrame

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alfred Kayser]

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

Comment 1

[Alfred Kayser]

Attachment: Combine BlackenFrame and SetMaskVisibility into ClearFrame

Comment 2

[Alfred Kayser]

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 3

[Stuart Parmenter]

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.

Comment 4

[Alfred Kayser]

Attachment: V2: Update to current trunk with Cairo

Also remove the SetMaskVisibility and BuildCompositeMask stuff, which is not needed for Cairo

Comment 5

[Alfred Kayser]

This then also 'fixes' bug 192790.

Comment 6

[Alfred Kayser]

Attachment: Little tweak to ClearFrame on multiple rows instead of looping

Comment 7

[Alfred Kayser]

Attachment: V4: updated to current tree (after Cairo cleanup)

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!

Comment 8

[Alfred Kayser]

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.

Comment 9

[Stuart Parmenter]

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.

Comment 10

[Stuart Parmenter]

ok, APNG is in the tree.  Can you put together an updated patch?

Comment 11

[Alfred Kayser]

Attachment: v5: Patch imgContainer

Note, also tested with APNG's from http://littlesvr.ca/apng/

Comment 12

[Stuart Parmenter]

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.

Comment 13

[Alfred Kayser]

Attachment: v6: Use Cairo and OPERATOR_CLEAR to clear the frame

Comment 14

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

Comment 15

[Alfred Kayser]

Attachment: V7: Removed SetColor after OPERATION_CLEAR

Comment 16

[Alfred Kayser]

Who can do the checkin for me? Thanks in advance, Alfred

Comment 17

[Nickolay_Ponomarev]

mozilla/modules/libpr0n/src/imgContainer.cpp  1.44
mozilla/modules/libpr0n/src/imgContainer.h    1.22

Comment 18

[Nickolay_Ponomarev]

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.

Comment 19

[Alfred Kayser]

Attachment: V8: Also fix nsGIFDecoder

Comment 20

[Nickolay_Ponomarev]

Does this need a re-review or can it be landed?

Comment 21

[Alfred Kayser]

Comment on attachment 262889 [details] [diff] [review]
V8: Also fix nsGIFDecoder

Tor, can you sr the part for nsGIFDecoder2.cpp?

Comment 22

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

Comment 23

[Alfred Kayser]

Attachment: V9: The right patch to nsGIFDecoder2.cpp

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

Comment 24

[Alfred Kayser]

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/

Comment 25

[Ryan VanderMeulen [:RyanVM]]

alfred, did you mean to actually request sr from tor on your v9 patch? :-)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 27

[Alfred Kayser]

Confirmed with my local build with this patch applied that bug 382764 is fixed by this patch.

Comment 28

[tor]

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

Comment 29

[Alfred Kayser]

Attachment: v10: heap based gfxContext

Note, there are great many other places with gfxContext could be placed on the stack...

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Sounds like a followup bug to me

Comment 31

[Alfred Kayser]

Created bug 383166 for the followup on stack allocation of gfxContext.

Comment 32

[Alfred Kayser]

Who can do the checkin for me?

Comment 33

[tor]

Checked in for Alfred.

Comment 34

[Alfred Kayser]

Thanks!

Tinderboxes all green:
 firefox-bin
  	Total:	       -944 (+448/-1392)
  	Code:	       -941 (+0/+0)
  	Data:	         -3 (+448/-1392)