181695 - Cleanup nsIImage

Status: RESOLVED FIXED

(Core Graveyard :: Image: Painting, defect, P1)

Description

[ArronM (:paper)]

I've CC'd all the module owners and peers of mozilla/gfx/* since this effects
them all.  Please feel free to unCC yourself if you don't care. :)
----------------------
Some nsIImage stuff is not being used at all and others are redundant.  It's not
needed and confuses newbies as well as people trying to write for new platforms.

Remove the following functions from nsIImage:

GetAlphaWidth()
GetAlphaHeight()

GetAlphaLevel()
SetAlphaLevel(..)

GetNaturalWidth()
SetNaturalWidth(..)
GetNaturalHeight()
SetNaturalHeight(..)

SetDecodedRect  There's only one outside call to SetDecodedRect, and one 
GetDecodedX1    outside call to GetDecodedY2.  Neither of these are needed
GetDecodedX2    after a simple code change.
GetDecodedY1    (Someday we'll want to get rid of the variables as well, and
GetDecodedY2     store what areas of the image are avaiable in layout, where
                 Stuart says it should be anyway)

IsOptimized()
----------------------
Remove variables from nsImage*:

mAlphaLevel     only a few platforms have this variable

mNaturalHeight  See Bug 116649
mNaturalWidth

mAlphaWidth     These two are always the same as the image width and height.
mAlphaHeight    Even if we did support different mAlphaWidth/Heights, wouldn't
                a start location of the alpha also be needed?  Nuke it :)
----------------------
One might say "Why remove all these, they might be usefull".. well, they've been 
in the code for over 3 years, and they *still* aren't being used.  Most likely
they will never be used.  And if they are needed one day, then the person
needing them can add them back in easily.  In the mean time, we'll have a 
cleaner, smaller nsIImage and nsImage*.

If anyone has a good reason any of the above can't be removed, or have spotted
an additional one that can be removed, post a comment explaining :)

Comment 1

[ArronM (:paper)]

Two others to remove:

DuplicateImage()
MoveAlphaMask()

These aren't in nsIImage, but several nsImage* have them, and they are blank..
so many platforms probably have it because they used nsImageGTK as a template.

Comment 2

[ArronM (:paper)]

going CC happy here

Comment 3

[ArronM (:paper)]

Attachment: XP patch

for gfxImageFrame.cpp:
Chunk #2: Code has been moved to nsImage*::ImageUpdated(..)

for imgContainerGIF.cpp:
Chunk #1: SetDecodedRect removed. About 10 lines later, BlackenFrame is called,
which calls ImageUpdated.  ImageUpdated() now dows what SetDecodedRect did.. so
everything should be okay.

Comment 4

[ArronM (:paper)]

Attachment: The BeOS Patch

[e-mail], [e-mail], could one of you (or both!)
see if this patch compiles and r=?  You'll also have to apply Attachment 107440 [details] [diff]

Comment 5

[ArronM (:paper)]

Attachment: The GTK Patch

Need someone to see if this compiles with Attachment 107440 [details] [diff] :)
and a r/sr :)

Comment 6

[ArronM (:paper)]

Attachment: The Mac Patch

Comment 7

[ArronM (:paper)]

Attachment: The OS2 Patch

mkaply, could you compile this patch at Attachement 17440, see if images still
display, and r=? :)

Comment 8

[ArronM (:paper)]

Attachment: The Photon Patch

[e-mail], [e-mail], could one of you try this patch and Patch
107440, see if it compiles, and r=?

Comment 9

[ArronM (:paper)]

Attachment: The Qt Patch

Comment 10

[ArronM (:paper)]

Attachment: The Windows Patch

Comment 11

[ArronM (:paper)]

Attachment: The XLib Patch!

Comment 12

[ArronM (:paper)]

Attachment: The Windows Patch

This one actually draws image correctly ;)

Comment 13

[ArronM (:paper)]

Attachment: The GTK Patch

..and this one actually compiles now :)

Comment 14

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

Comment on attachment 107452 [details] [diff] [review]
The GTK Patch

+	// Assumed: [...]
you have a tab here, please fix that. (though no need to attach a new patch)

Comment 15

[jhp (no longer active)]

Comment on attachment 107444 [details] [diff] [review]
The OS2 Patch

r=pedemont (in place of mkaply)

Comment 16

[ArronM (:paper)]

Comment on attachment 107440 [details] [diff] [review]
XP patch

Rod, could you review?
Thanks:)

Comment 17

[ArronM (:paper)]

Comment on attachment 107451 [details] [diff] [review]
The Windows Patch

..this one too, please

Comment 18

[Paul]

Comment on attachment 107441 [details] [diff] [review]
The BeOS Patch

Seems to be ok to me.
r=arougthopher

Comment 19

[dcone (gone)]

I have some questions... 
are we not using the decoded sizes anymore?   I see where
we set them in the GIF decoder... and where we use them in nsImage.  I know we
use to use them to keep from painting garbage when the entire image was not read
in yet.

Also.. just because the Init is currently only called once... it may be useful..
and thats the purpose of init.. is to reset the values of the object independent
of the constructor.. right.   I
I noticed that the init now assumes the constructor has set the values.. isnt that
wrong?

I noticed where you are using the header information.. instead of the object
variables.. also.. you are assuming that the alpha image is the same size as the
main image... I am not conviced these two points will always hold true
Also.. the mBHead->biHeight can be negative.. right.  So wouldnt the following
have a problem? 

example 
-            MONOBITMAPINFO  bmi(mAlphaWidth, mAlphaHeight);
+            MONOBITMAPINFO  bmi(mBHead->biWidth, mBHead->biHeight);

why are you assuming GetIsRowOrderTopToBottom() is always false and we don't
need that variable.  It is legal and used alot in windows to reverse that order
(hence the negative mBHead->biWidth..) 

Also.. I have a patch that got it review and is getting a Code review that
cleans up some of these issues.. its for the GIF 8 bit patch (143046).  Can this
patch wait for that to get in.. since I have changed alot of stuff in nsImage.  

Comment 20

[ArronM (:paper)]

>are we not using the decoded sizes anymore?   I see where
>we set them in the GIF decoder... and where we use them in nsImage.  

It doesn't look like any of the Get's were ever used aside from gfxImageFrame. 
Perhaps the GIF decoder needed to set the decoded rect before, but now it's
totally useless.  Anything that changes the image data should be calling
ImageUpdated(..).  It's the perfect place to set the decoded area (since it's
being passed the area that's been updated).  gfxImageFrame::SetImageData already
calls ImageUpdated for the caller, so any code using SetImageData to build an
image is already updating the decoded area.


>I know we use to use them to keep from painting garbage when the entire image 
>was not read in yet.

Yes, internally we need the decoded sizes, but we don't need to use
GetDecoded[XY][12] to get them in there, we just use the variables.


>Also.. just because the Init is currently only called once... it may be useful..
>and thats the purpose of init.. is to reset the values of the object independent
>of the constructor.. right.   I
>I noticed that the init now assumes the constructor has set the values.. isnt that
>wrong?

It's only wrong if Init is being called more than once, which it never ever is.
If Init is being called only once (which it only can be), why set variables to
something that they are already being set to in the constructor?  This is a
waste of CPU cycles and bloat of code in my mind. 


>I noticed where you are using the header information.. instead of the object
>variables.. also.. you are assuming that the alpha image is the same size as the
>main image... I am not conviced these two points will always hold true
>Also.. the mBHead->biHeight can be negative.. right.  So wouldnt the following
>have a problem? 

biHeight is assumed to be positive in all other code in nsImageWin.  If that
assumption wasn't the case, then the code would have broken so many times over
by now.  There are for loops that go from 0 to biHeight that would have broken,
and passed in heights are being compared as if biHeight was positive.  We
already assume alpha width/height is the same as the image in this line:

366  ALPHA32BITMAPINFO bmi(mBHead->biWidth, mBHead->biHeight);

And as I've said in Comment #0, if we wanted to start supporting alpha layers
that aren't the same size as the image, we'd have to start storing the location
where the alpha layer starts within the image, and a lot of code would have to
be rewritten.  I'm not saying that smaller alpha layers shouldn't be done.. I'm
just saying they aren't done, and those variables/functions aren't needed until
someone writes such a patch (if they'll need them at all).  Realistically, I
don't see anyone creating such a patch.


>why are you assuming GetIsRowOrderTopToBottom() is always false and we don't
>need that variable.  It is legal and used alot in windows to reverse that order
>(hence the negative mBHead->biWidth..) 

Again, like biHeight, the existing code already assumes biWidth is positive. 
Windows stores images bottom to top.  Microsoft isn't going to change this now.
 Window's GDI is always going to store images bottom to top.  Why would we want
to store one image top to bottom, and the next bottom to top anyway?  Isn't
consistency good?  All images on a single platform are stored in the same direction.


>Also.. I have a patch that got it review and is getting a Code review that
>cleans up some of these issues.. its for the GIF 8 bit patch (143046).  Can this
>patch wait for that to get in.. since I have changed alot of stuff in nsImage.  

This doesn't mean that both patches can't be worked on, reviewed, etc
simultaineously.

Comment 21

[dcone (gone)]

1.)  I agree with the logic you have about using the header.  Just makes things
easier to maintain.  

2.)  I agree with the logic about the negative height.. but I think we have to
comment that negative height is not supported in our code (not that it would
have worked before.. but it was trying to be supported).  I assume you did not
take out the entire method because it is used somewhere... right?

3.)  The decoded size is ok.. make sure that Pavlov or Saari know this since it
comes down to there responsibility for that.

4.)  My only uncomfortableness is the init.  I just believe an init.. should
initialize the object.. otherwise.. why have an init.  I know we only call it
once.. but someone might use it again in the future.. and then something will
break.  It bloat is what your concerned about.. then maybee the init should do
everything.  I know I would be happier if init still... initited the class.

I will r it for you.. what ever we decide on 4.. it might just take me a day to
think it thru.  I will ask for opionions, and take a look how exacty nsImage
uses this init or should use it.

Comment 22

[rods (gone)]

Please dcone review this, it is his ownership area.

Comment 23

[ArronM (:paper)]

Attachment: Windows Patch

Well, I'd rather be safe than sorry too, so I added an initialized variable,
set it to false in the constructor, abort the init if it's already true, and
set it to true at the end of init.

With regards to the variable initializing in the constructor, the reason it's
done is usually the compiler can optimize it into one memcpy, which is faster
than multiple sets.

Thanks for the review offer.  I very much appreciate it.

Comment 24

[dcone (gone)]

Comment on attachment 108076 [details] [diff] [review]
Windows Patch

r=dcone.  I was making sure how we use our init.  I think a use only once init
is fine.  I am checking in.. soon I hope a patch that touches some of what you
did, fixes some stuff.. it should not effect this patch except I doot out the
CleanUpDIBSection completely.. and the associated member.  Just a heads up.. my
sr should be any day.

Comment 25

[ArronM (:paper)]

Thanks for the heads up and review, dcone.  The conflicts shouldn't be to bad.

Could you also review the XP patch (Attachment 107440 [details] [diff])?  (if not, I can get
someone else to do it)

Comment 26

[dcone (gone)]

Comment on attachment 107440 [details] [diff] [review]
XP patch

r=dcone on XP

Comment 27

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

Comment on attachment 107449 [details] [diff] [review]
The XLib Patch!

guess gisburn would be a good person to review this

Comment 28

[ArronM (:paper)]

Updating some flags.
Reviews for other patches are in progress, I'm just too lazy to mark them r?
(plus it creates bugspam)

Comment 29

[ArronM (:paper)]

biesi, timeless, or anyone with Qt.. now that Qt actually compiles, could you
try the Qt Patch along with the XP Patch (both are needed) and see if there's
any compile errors?

If you get patching conflicts, just say so and I'll refresh the patch(es).

Oh, if it does compile, you could r+ it and save tor some time :)

Comment 30

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

Comment on attachment 107446 [details] [diff] [review]
The Qt Patch

patching file nsImageQT.cpp
Hunk #1 FAILED at 53.
Hunk #2 succeeded at 108 (offset 1 line).
Hunk #3 FAILED at 135.
Hunk #4 succeeded at 203 (offset 2 lines).
Hunk #5 succeeded at 247 (offset 2 lines).
Hunk #6 succeeded at 434 (offset 1 line).
Hunk #7 succeeded at 447 with fuzz 2 (offset 1 line).
2 out of 7 hunks FAILED -- saving rejects to file nsImageQT.cpp.rej

Comment 31

[ArronM (:paper)]

Attachment: Qt Patch

Updated Qt Diff

Comment 32

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

Comment on attachment 109553 [details] [diff] [review]
Qt Patch

+  virtual PRBool      GetIsRowOrderTopToBottom() {return PR_TRUE};

if the ; is moved before the } this even compiles :)
and images still render.

timeless being the port owner, he should probably review this; requesting
r=timeless.

Comment 33

[timeless]

Comment on attachment 109553 [details] [diff] [review]
Qt Patch

it looks fine to me with biesi's stipulated change.

Comment 34

[ArronM (:paper)]

Attachment: GTK Patch

Updated to 2002-12-21

Comment 35

[ArronM (:paper)]

Attachment: Mac Patch

Updated to 2002-12-21

Comment 36

[tor]

Comment on attachment 109910 [details] [diff] [review]
GTK Patch

The switch(aMaskRequirements) needs a default: section to avoid a compiler
warning about unhandled enums.
That and r=tor.

Comment 37

[tor]

Comment on attachment 107445 [details] [diff] [review]
The Photon Patch

Same comment regarding the switch statement.  r=tor

Comment 38

[tor]

Comment on attachment 109911 [details] [diff] [review]
Mac Patch

Same comment regarding the switch statement.  r=tor

Comment 39

[tor]

Comment on attachment 107449 [details] [diff] [review]
The XLib Patch!

Same comment regarding the switch statement.  r=tor

Comment 40

[tor]

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

Comment 41

[ArronM (:paper)]

roc, my would be SR ginuea pig, is away until the 13th.  I have no idea who else
to ask for a SR.  I'm guessing some of these patches only need a RS.  If anyone
has alternate SR suggestions, please comment, otherwise this bug will patiently
wait a couple more weeks.

Comment 42

[ArronM (:paper)]

Attachment: 9 Patches in 1 Zip

All 9 patches unbitrotted.  No other changes, unless you include those switch
defaults that tor requested.

Comment 43

[ArronM (:paper)]

Comment on attachment 111267 [details]
9 Patches in 1 Zip

transferring r+
See other patches for people who reviewed them

Comment 44

[ArronM (:paper)]

roc, do you think you'll have time to SR before the freeze?  If not, I'll mark
this as a 1.4a target milestone.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I will sr it before the freeze.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 111267 [details]
9 Patches in 1 Zip

sr=roc+moz

just one question ... did you add the mInitialized check to nsImageWin just so
that any multiple-initialization bugs will be detected there?

Comment 47

[ArronM (:paper)]

Thanks for the SR.
The mInitialized check was added to appease dcone. :)  No one on the other
platforms raised concerns about nsIImage being initialized twice (which I still
maintain is impossible since gfxIImageFrame::Init is the only one who creates a
nsIImage, and gfxIImageFrame has a mInitialized check for itself before the
nsIImage)


I'll be checking this in after OS2VACPP goes green, when there's a quiet moment
(Perhaps late Sunday night)

Comment 48

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

this was checked in yesterday, should this bug be marked fixed?

Comment 49

[ArronM (:paper)]

Yes, I fell asleep before marking the bug fixed :P