148637 - Clean-up & Optimization of imgContainerGIF

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, enhancement)

Description

[ArronM (:paper)]

imgContainerGIF needs some cleaning up and general optimization.  I'll try to
create seperate bugs where I can, but a bulk of the changes are "all or none"

As per comments in Bug 85595, this bug will contain at a minumum changing the
code that is commented to be "essentially UnionRect" to actually use UnionRect.

Some of the optimizations which will be included here or in a new bug if I can
seperate it:

- Timer Creation Clean-up.  Timers only need to be created in one place.
- Skip building a composite for frames that really don't need it (ie. one where
the previous frame is a full frame and it disposed of itself by clearing itself
to background)
- Reduce the need for mCompositing frame by copying composited image back to
mFrames if certain criteria is met
- Free memory used by mCompositing frame when no longer in use.
- Build a nsRect during first loop (appendframe) to determine what area of the
first frame really needs updating.

Comment 1

[ArronM (:paper)]

Attachment: What I have

This is what I have so far.  I'm not seeking a r= on it.  I'll be splitting it
up into smaller patches and shoving them off into seperate bugs.  I'm attaching
the patch in case anyone wants to play.

Plus there's a lot of junk in the patch, like trailing spaces, debug code, and
silly comments.

Comment 2

[ArronM (:paper)]

Attachment: What I have, unbitrotted to Oct 12

Patch smaller now that Bug 152756 is checked in.

Comment 3

[ArronM (:paper)]

Attachment: Cleanup #1

Mostly reformatting.  Some minor code changes, the biggest one being
replacement of "switch (format)" with a check&exit if alpha layer isn't 1 bit.

Comment 4

[ArronM (:paper)]

Attachment: Cleanup #1: diff -uw version

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107303 [details] [diff] [review]
Cleanup #1: diff -uw version

why did you remove the second part of the BuildCompositeMask comment?

-  aOverlayFrame->LockAlphaData();
er, can you remove that just like that?

-/// Windows and OS/2 have the funky bottom up data storage we need to account
for
any reason you're removing the comment here? or just because you mentioned it
100 lines above?

+  nsWeakPtr		mObserver;
maybe you should add a comemnt to this member as well, esp. what type the
observer is?

+  //! All the frames of the GIF
+  nsSupportsArray	mFrames;
maybe mention what types the frames are? (gfxIImageFrame I guess?)

rest looks good

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107302 [details] [diff] [review]
Cleanup #1

marking review- to get it off my queue

Comment 7

[ArronM (:paper)]

Attachment: Cleanup #1

-  aOverlayFrame->LockAlphaData();
Look 10 lines above.. I replaced
"NS_FAILED(aOverlayFrame->GetTransparentColor.." with
"NS_FAILED(aOverlayFrame->LockAlphaData..".  Both fail if there's not Alpha
Data.

-/// Windows and OS/2 
Yeah, mentioning it once per function is enough, I feel.  Anymore than that and
it gets annoying.

The other two changes have been done! :)

Comment 8

[ArronM (:paper)]

Attachment: Cleanup #1: diff -uw version

Comment 9

[ArronM (:paper)]

Attachment: Cleanup #1: diff -uw version

Comment 10

[ArronM (:paper)]

Attachment: Cleanup #1

I changed the logic of BuildCompositeMask to exit out of the overlay is beyond
the area of the composite.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107497 [details] [diff] [review]
Cleanup #1: diff -uw version

	 const PRUint32 width  =
PR_MIN(widthOverlay,(widthComposite-overlayXOffset));
this could do with a few more spaces, same for the next line

Comment 12

[ArronM (:paper)]

Attachment: Cleanup #1

I forgot for a moment there that I was doing a code cleanup patch, and went for
not modifying more lines than I needed.  Silly me.  Spaced, and split over two
lines :)

Comment 13

[ArronM (:paper)]

Comment on attachment 107503 [details] [diff] [review]
Cleanup #1

I've also moved Line 41 ("mSize(0,0),") down a line to shut up biesi's compiler
warning.

I'm leaving the unsigned mismatch compiler warning, because the WIP patch in
this bug will is rewriting that function (and two other functions that I did
not clean up)

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 107503 [details] [diff] [review]
Cleanup #1

r=biesi with the move of mSize initialization

Comment 15

[tor]

Comment on attachment 107503 [details] [diff] [review]
Cleanup #1

sr=tor with warning change

Comment 16

[ArronM (:paper)]

Comment on attachment 107503 [details] [diff] [review]
Cleanup #1

Checking in imgContainerGIF.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.cpp,v  <-- 
imgContainerGIF.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in imgContainerGIF.h;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.h,v  <-- 
imgContainerGIF.h
new revision: 1.3; previous revision: 1.2
done

Leaving Bug open for Optimization part

Comment 17

[ArronM (:paper)]

Attachment: Optimizations

Rewrite of AppendFrame, Notify, and DoComposite

Probably best to just apply the patch and review those 3 functions (and compare
it to the pre-patched functions if need be).  Trying to read the diff will be a
nightmare.  Even a -uw is ugly, but I'll attach one if requested.

Comment 18

[ArronM (:paper)]

Comment on attachment 114132 [details] [diff] [review]
Optimizations

ack, there's a bunch of trailing spaces.  Consider them removed. 
I'll wait until biesi finds something wrong before I post a new patch though.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 114132 [details] [diff] [review]
Optimizations

+   *			 (aNextFrame,mCompositingFrame, or
mCompositingPrevFrame)
nit: space missing after aNextFrame

+  void CopyFrameImage(gfxIImageFrame *aSrcFrame, gfxIImageFrame *aDstFrame);
hm, can this be a static method?

as mentioned on irc, please remove NewFrameData, given that it's empty.

+  // aFrameNum is 1 based, mCurrentDecodingFrameIndex is not.
maybe mention what mCurrentDecodingFrameIndex _is_ based on (presumably 0)

+void imgContainerGIF::CopyFrameImage(gfxIImageFrame *aSrcFrame,
what if an error happens in this function? does the caller not care?

more comments later...

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 114132 [details] [diff] [review]
Optimizations

  NS_ASSERTION(item, "imgContainerGIF::AppendFrame: item is null");
  if (!item)
    return NS_ERROR_NULL_POINTER;

NS_ENSURE_ARG_POINTER(item);

instead of this->some_member_function()
just use some_member_function
(mostly, maybe only, affects StopAnimation)

not sure if I mentioned it already, but |if NS_FAILED(...)| should have an
additional pair of (), several times.


more tomorrow

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 114132 [details] [diff] [review]
Optimizations

please also add an assertion in ::DoComposite for aFrameToUse

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 114132 [details] [diff] [review]
Optimizations

    mCompositingFrame = do_CreateInstance("@mozilla.org/gfx/image/frame;2");
    if (!mCompositingFrame)
      return NS_ERROR_OUT_OF_MEMORY;

might want to use this isntead:
    mCompositingFrame = do_CreateInstance("@mozilla.org/gfx/image/frame;2",
&rv);
    if (NS_FAILED(rv))
      return rv;

  nsCOMPtr<nsIInterfaceRequestor> ireq(do_QueryInterface(aDstFrame));

Given that you commented the other two code blocks in this (CopyFrameImage)
function, please also add a comment to this one, like:
      // Tell the image that it's data has been updated
(copied from the original code)

And it would be a good idea to null-check ireq and img before using them.

Marking review- because I'd like to see a new patch with the changes I
mentioned.

Comment 23

[ArronM (:paper)]

Attachment: Optimizations

Everything except the NS_ENSURE_ARG_POINTER changes, as per discussion on IRC.
A bit more documentation too.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 114411 [details] [diff] [review]
Optimizations

+static void CopyFrameImage(gfxIImageFrame *aSrcFrame, gfxIImageFrame
*aDstFrame);

um, I really meant a static method on the class... but this is also ok, I
guess.

+      // XXX - This could be done it multiple framechanged calls
do you mean "with" instead of "it"?

thanks for the patch, looks good.

Comment 25

[ArronM (:paper)]

Attachment: Optimizations

Changes from last patch:
- Changed CopyFrameImage() to return PRBool, and fail if size is different,
etc.

- The CopyFrameImage that copies a composited frame back into mFrames now
checks the results and skips optimization.  The other 2 calls to CopyFrameImage
won't break on failure, so they don't check the result.

- Made CopyFrameImage static as originally intended by biesi's request (I hope)


- Fixed that "it" == "with" typo

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 114850 [details] [diff] [review]
Optimizations

+  if (NS_SUCCEEDED(aDstFrame->LockImageData())) {
and
+  if (NS_SUCCEEDED(aDstFrame->LockAlphaData())) {

shouldn't you return PR_FALSE if this fails?

looks good otherwise,
r=biesi

Comment 27

[tor]

Comment on attachment 114850 [details] [diff] [review]
Optimizations

Remove the extra prototype of CopyFrameImage in the .cpp, follow
biesi's last comment, and attach a patch for future reference.
sr=tor

Comment 28

[ArronM (:paper)]

Attachment: Optimizations: Patch Checked In

Comment 29

[ArronM (:paper)]

checked in!!  Let there be rejoicing and lots of dancing, but please, no more
sites with dancing GIFs. :)

Checking in imgContainerGIF.h;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.h,v  <-- 
imgContainerGIF.h
new revision: 1.6; previous revision: 1.5
done
Checking in imgContainerGIF.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.cpp,v  <-- 
imgContainerGIF.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in nsGIFDecoder2.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp,v  <-- 
nsGIFDecoder2.cpp
new revision: 1.47; previous revision: 1.46
done

Comment 30

[ArronM (:paper)]

*** Bug 177948 has been marked as a duplicate of this bug. ***