77497 - imgContainer needs to have a GIF specific version

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[saari (gone)]

imgContainer has gotten too overloaded, needs to be spit up into a GIF specific
version and a general version.

Comment 1

[ArronM (:paper)]

I've been living and breathing imgContainer.cpp for the past 2+ weeks.  I'm
willing to make an imgContainerGIF and move relevent code to that module. 
That'll free up your time for other more important things.

The only question I have (so far) is: why is this bug marked as Mac Platform and
not All?

Comment 2

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

marking platform->all since that's the only sensible value for this bug

Comment 3

[ArronM (:paper)]

taking bug from saari & ccing people who love bugmail

I'll post a patch after Bug 85595 gets checked-in

Comment 4

[ArronM (:paper)]

Attachment: Patch 1of3: imgContainer stripped of gif stuff

I intend to have 3 patches:
1) imgContainer stripped of gif stuff (this one)
2) imgContainerGIF (basically a copy of imgContainer before the strip, along
with the required class name changes and compiler stuff)
3) Mac build file, which I will need someone with a Mac to do.

I intend to make no changes in imgContainerGIF other than the class name
changes and trailing white space removal.  This will make for a faster review. 
Any code standardizations and optimizations will be done in other bugs (See Bug
148637)

I anticipate that (1) will need the most "tender-lov'n reviewing" since after
stripping, I cleaned it up and put in null checking everywhere I could.

Comment 5

[ArronM (:paper)]

Comment on attachment 86834 [details] [diff] [review]
Patch 1of3: imgContainer stripped of gif stuff

ignore that patch, it's diff'd on the wrong files

Comment 6

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

>2) imgContainerGIF (basically a copy of imgContainer before the strip, along
>with the required class name changes and compiler stuff)

Hm, is ContainerGIF really so different? As in, might it be a good idea to make
imgContainerGIF inherit from imgContainer, so that common code could be shared?

Comment 7

[ArronM (:paper)]

Attachment: Patch 1of3: imgContainer stripped of gif stuff

See Comment #4 for info on this patch.	One question I anticipated from
reviewers is about the removal of code from DecodingComplete.  Only GIFs called
DecodingComplete, so it's safe.  (I may create a new bug to deal with removing
or making all decoders use DecodingComplete)


biesi, after the strip down, imgContainer contains hardly any code.  The only
common code that imgContainerGIF could use would be a few of the one-liner
functions.  The 2 biggest functions left in imgContainer, AppendFrame and
Notify will look very different after Bug 148637. 

Actually, I question leaving the animation code bits in there in the first
place.	Replacing the animation functions with NS_ERROR_NOT_IMPLEMENTED  and
removing the variables would save a few bytes and get rid of that
nsITimerCallback.  Perhaps a good idea for another RFE bug, but out of the
scope of this bug, IMO.

Comment 8

[ArronM (:paper)]

Attachment: Patch 2of3: imgContainerGIF

Addition of 2 files, plus changes to Makefile.in, Makefile.win,
nsGIFDecoder2.cpp, and nsGIFModule.cpp.  I think that's all I need to modify.

Comment 9

[ArronM (:paper)]

Attachment: diff -u0w imgContainer.cpp imgContainerGIF.cpp

This proves that imgContainerGIF.cpp is the same as imgContainer.cpp with the
exception of the class name.  (meaning there's no need to review the
imgContainerGIF.cpp code)

Comment 10

[ArronM (:paper)]

All my imgContainer patches seem to be in now.
I'll be un-bitrotting the patches in this bug within the next day or two.

Comment 11

[ArronM (:paper)]

Attachment: Patch 1of3: imgContainer stripped of gif stuff

diff'd against imgContainer.h v1.15 and imgContainer.cpp v1.32

Comment 12

[ArronM (:paper)]

Attachment: Patch 2of3: imgContainerGIF

only diff change is in imgContainerGIF.  Same diff for the rest of the files
(no new checkins in them)

Comment 13

[ArronM (:paper)]

Attachment: diff -u0w imgContainer.cpp imgContainerGIF.cpp

diff'd against imgContainer.cpp is v1.32 (latest) to prove that nothing was
changed.

Comment 14

[ArronM (:paper)]

pavlov, tor: a r= and sr= before someone patches
modules/libpr0n/decoders/src/imgContainer.* or modules/libpr0n/decoders/gif/*
again would be nice.

The sooner this one gets in, the sooner I can put up the patches for the bugs
that this block. :)

Comment 15

[ArronM (:paper)]

Attachment: Patch 2of3: imgContainerGIF

fixed spelling error in makefile.win.. imgConatiner -> imgContainer

Comment 16

[Stuart Parmenter]

Comment on attachment 90340 [details] [diff] [review]
Patch 2of3: imgContainerGIF

r=pavlov

Comment 17

[ArronM (:paper)]

futuring since I don't have time to hunt down r='s

Comment 18

[ArronM (:paper)]

Attachment: Patch 1of3: imgContainer stripped of gif stuff

Comment 19

[ArronM (:paper)]

Attachment: Patch 2of3: imgContainerGIF

Comment 20

[ArronM (:paper)]

Patches are now current.

Comment 21

[ArronM (:paper)]

Attachment: Patch 2of3: imgContainerGIF

Corrected diff so the new files are added to the right directory.

Comment 22

[ArronM (:paper)]

New checkins today, current patches won't apply. Another set of patches coming
up shortly.

Comment 23

[ArronM (:paper)]

Attachment: 1of3: imgContainer stripped of gif stuff

did a cvs diff from the root this time

Comment 24

[ArronM (:paper)]

Attachment: 2of3: imgContainerGIF

Comment 25

[ArronM (:paper)]

Attachment: 1of3: imgContainer stripped of multi-frames

As per discussion with tor, all animation code has been removed.  imgContainer
is now one frame only.

Comment 26

[tor]

Comment on attachment 101722 [details] [diff] [review]
2of3: imgContainerGIF

A couple of small argument checking things:

 * GetCurrentFrame should check for a null pointer
 * GetNumFrames should return with NS_ERROR_NULL_POINTER
   if the arg is null instead of trying to assign through it.

Comment 27

[tor]

Comment on attachment 101722 [details] [diff] [review]
2of3: imgContainerGIF

Ignore that comment - wrong attachment.  sr=tor

Comment 28

[tor]

Comment on attachment 101847 [details] [diff] [review]
1of3: imgContainer stripped of multi-frames

A couple of small argument checking things:

 * GetCurrentFrame should check for a null pointer
 * GetNumFrames should return with NS_ERROR_NULL_POINTER
   if the arg is null instead of trying to assign through it.

Comment 29

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

re attachment 101847 [details] [diff] [review] (patch 1 of 3):

+    mFrame = nsnull;
+    NS_RELEASE(mFrame);

doesn't look right to me :)
also, maybe you should use a COMPtr for it?

+  if (!mFrame)
+    return NS_ERROR_FAILURE;
+  *aCurrentFrame = mFrame;
+  NS_IF_ADDREF(*aCurrentFrame);

mFrame can not be null here. therefore, you can use NS_ADDREF rather than
NS_IF_ADDREF

+  *aNumFrames = (mFrame) ? 1 : 0;

no need for parentheses

+  NS_IF_ADDREF(*_retval);

as above, use NS_ADDREF.

+  mFrame = item;
+  NS_ADDREF(mFrame);

when using a COMPtr for mFrame as suggested above, you can remove the ADDREF here.

 NS_IMETHODIMP imgContainer::DecodingComplete(void)
 {
-  mDoneDecoding = PR_TRUE;
-
-  PRUint32 numFrames;
-  mFrames.Count(&numFrames);
-  if (numFrames == 1) {
-    nsCOMPtr<gfxIImageFrame> currentFrame;
-    inlinedGetFrameAt(0, getter_AddRefs(currentFrame));
-    currentFrame->SetMutable(PR_FALSE);
-  }
   return NS_OK;

um, shouldn't you still do the SetMutable call?

in StartAnimation and StopAnimation: I am not sure if NOT_IMPLEMENTED is the
right thing to return. The old implementation returned NS_OK even if there was
nothing to animate.

SetLoopCount: you are always returning NOT_IMPLEMENTED here. this is
inconsistent with SetAnimationMode, which returns OK but ignores the argument.


rest of this patch looks ok on first sight.

Comment 30

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

Comment on attachment 101722 [details] [diff] [review]
2of3: imgContainerGIF

r=biesi; I trust you that imgContainerGIF is indeed identical to the old
imgCotainer except the name.

Comment 31

[ArronM (:paper)]

Attachment: 1of3: imgContainer stripped

-changed everything that biesi (see notes) and tor mentioned
-removed NS_DECL_IMGICONTAINEROBSERVER
-removed imgIDecoderObserver

imgContainer is a wee thing now! :O

Biesi:
-DecodingComplete is only called by the GIF decoder.  So the SetMutable is
useless for anything else.  (JPGs, for example, SetMutable inside the decoder)

-Both [Get,Set]LoopCount now return NS_ERROR_NOT_IMPLEMENTED, and both
AnimationModes return NS_OK.  In other words, we are faking implementation of
AnimationMode, and not implementing LoopCount.

Comment 32

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

Comment on attachment 101950 [details] [diff] [review]
1of3: imgContainer stripped

thanks for the changes - I noticed some more things, though:
in GetCurrentFrame:
maybe NS_ERROR_NOT_INITIALIZED would be more appropriate than _FAILURE?

looks great otherwise!
I'm alas unable to test the patch currently... just make sure that you test
well :)

r=biesi.

Comment 33

[Stuart Parmenter]

Comment on attachment 101950 [details] [diff] [review]
1of3: imgContainer stripped

Please either remove all the NS_ENSURE crap or move the return outside a macro.
 Macros that return suck ass for debugging since you can't break on return.
I think that checking these out paramters is a waste of cycles and just
clutters up the code, but if you really really want them, then change them all
to something that doesn't have return in the macro.

aside from this, the patch looks good.	Post a new patch and i'll r= it.

Comment 34

[ArronM (:paper)]

Attachment: 1of3: imgContainer stripped

For all you anti NS_ENSURE-ers out there :)

Comment 35

[Stuart Parmenter]

Comment on attachment 102024 [details] [diff] [review]
1of3: imgContainer stripped

bleh, r=pavlov

Comment 36

[tor]

Comment on attachment 102024 [details] [diff] [review]
1of3: imgContainer stripped

sr=tor

Comment 37

[TimMaroney]

Attachment: patch to mac build file

Not fully tested, but should work, as per #mozilla w/Paper, jkeiser.

Comment 38

[ArronM (:paper)]

Attachment: 1of3: imgContainer stripped - end check-in

Same as previous, except with the following bustage fix patch
(1 line, variable name wrong)
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/modules/libpr0n/src&command=DIFF_FRAMESET&file=imgContainer.cpp&rev1=1.35&rev2=1.36&root=/cvsroot

Comment 39

[ArronM (:paper)]

Comment on attachment 102095 [details] [diff] [review]
1of3: imgContainer stripped - end check-in

transferring r= & sr=

Comment 40

[ArronM (:paper)]

Patches checked in on 10/07/2002 14:45 & 10/07/2002 14:49
Bustage fixed on 10/07/2002 15:25