753 - Combined nsImage* & gfxImageFrame

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[leger]

Setting all current Open/Normal to M4.

Comment 1

[Paul MacQuiddy]

per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

Comment 2

[Christine Hoffman]

Reassigning qa contact to [e-mail]

Comment 3

[vidur (gone)]

I presume Kipp meant that the methods should be NS_IMETHODs. I'm not sure why it
was assigned to me. Michael, I presume this will go to one of the new gfx
owners.

Comment 4

[Kevin McCluskey (gone)]

Moving to M9

Comment 5

[Kevin McCluskey (gone)]

This requires changes on all three platforms WIN32, Linux, Mac for the platform
dependant implementations + all of the places that call these methods need to be
modified.

Also need to modify the postscript directory.
Need to coordinate with platform owners, probably will need a carpool to
checkin.

CC'ing platform owners, ramiro, beard, and cone.

XPCODE makes calls to methods on the nsIImage interface in:
S:\mozilla\gfx\src\nsImageRenderer.cpp
S:\mozilla\gfx\tests\btest\BitTest.cpp

Comment 6

[Kevin McCluskey (gone)]

Moving to M12

Comment 7

[Kevin McCluskey (gone)]

Moving to M14

Comment 8

[Kevin McCluskey (gone)]

Don, could you look at this? 

Comment 9

[dcone (gone)]

This bug has been marked future because we have determined that it is not 
critical for netscape 6.0.  If you feel this is an error, or if it blocks your 
work in some way -- please attach your concern to the bug for reconsideration.

Comment 10

[dcone (gone)]

Kevin, is this still an issue.. or does this need to be done.  
If you still want this done, assign it back to me.

Comment 11

[Kevin McCluskey (gone)]

Reassigning to dcone.

Comment 12

[Hixie (not reading bugmail)]

pavlov, fyi

Comment 13

[Stuart Parmenter]

i should probably take this bug, since i've already done this as part of the new
imagelib work.

Comment 14

[dcone (gone)]

You can take this Pavlov

Comment 15

[Stuart Parmenter]

This is partially fixed.. moving to future to get off my immediate radar.

Comment 16

[Stuart Parmenter]

Moving bugs to new Image: GFX component

Comment 17

[Stefan Moebius]

Pretty ol' bug eh? Still valid? ;-)

Comment 18

[ArronM (:paper)]

It looks still valid.

I might take this.  I'm currently looking into combining gfxIImageFrame and
nsIImage.  gfxIImageFrame already has an IDL.

Comment 19

[ArronM (:paper)]

I don't think stuart will mind if I reassign this bug to me. ;)

Comment 20

[ArronM (:paper)]

reassigning for real this time

Comment 21

[ArronM (:paper)]

(This comment is really for my own reference, however, it might prove usefull to
reviewers/people interested in this bug)

Here's a list of classes that inherit from nsIImage
http://lxr.mozilla.org/seamonkey/search?string=public+nsIImage%5B%5Ea-z%5D&regexp=on

All of them except the mozilla/gfx2/ one need/will be updated.  Unlucky for me,
most of nsIImage functions did not return nsresults.

Here's the list of files using nsIImage:
http://lxr.mozilla.org/seamonkey/ident?i=nsIImage
69 referenced files, 11 of which are gfx2 and can be ignored.

Comment 22

[Stuart Parmenter]

this bug should be marked WONTFIX.  nsIImage isn't public.  gfxIImageFrame is
XPCOMized and should be used far before nsIImage.  The only reason I left it
open is cause I liked  having a 3 digit bug number :-)

Comment 23

[ArronM (:paper)]

but I want to combine nsIImage and gfxIImageFrame.  There's no need for both of
them.. a lot of the gfxIImageframe functions just call nsIImage.

Here's my proposed plan:
1) XPCOMize nsIImage, create nsImage, and have nsImage* inherit from nsImage.
2) Move exact-same-code functions from nsImage* to nsImage.
3) Combine nsIImage and gfxIImageFrame.  Move gfxImageFrame into nsImage.

thoughts?

Comment 24

[dcone (gone)]

I agree with Pavlov.. or at least we need to have a reason .. a good reason to
do this.  What would doing this buy us?  What are the risks?  

Comment 25

[ArronM (:paper)]

What's gained:

- code consolidation.  How often have we added something to nsImageWin and then
have to add almost the exact same code to nsImageGTK?  How often have we made a
function in gfxIImageFrame which calls nsImage* to do the task?  Quite often
judging by the code.

- Easier to understand.  Having nsIImageWin and gfxIImageFrame just adds
confusion to a new mozilla hacker.

I can't think of any risks at the moment, but I'd love to hear some.

Comment 26

[Stuart Parmenter]

it would be far easier to take the code from nsImage* and make a gfxImageFrame
impl with it.. then it could be done incrementally.

still, there have got to be more important things to do than this.

Comment 27

[ArronM (:paper)]

aah, I'm beginning to see the light now.

nsIImage is still not needed, so this bug is now the "eliminate nsIImage" bug.

Similar nsIImage* code can go into gfxImageFrame.  nsImage* can inherit from
gfxImageFrame.  We could even rename nsImage* to gfxImageFrame* (ie nsImageWin
to gfxImageFrameWin), but as stuart mentioned on IRC, renaming stuff sucks.

Comment 28

[dcone (gone)]

I totally don't agree with eliminating nsIImage..  
nsIImage encapulates an image/bitmap.. I am sure that the gfxImageFrame
encapulates something else. 

Comment 29

[ArronM (:paper)]

gfxImageFrame encapsulates nsIImage and adds a (x,y) offset.  gfxImageFrame
contains a nsCOMPtr to one nsIImage, and almost every function inside
gfxImageFrame calls that nsIImage to get it's work done.  Perhaps a better
summary is in order:
Combined nsImage* & gfxImageFrame
                                       +----------+
                                       |nsImageGTK|
                                     _ +----------+
                                     /|
+--------------+    +-------------+ /  +----------+
|gfxIImageFrame| -> |gfxImageFrame| -> |nsImageWin|
+--------------+    +-------------+ \  +----------+
                                    _\|
                                       +---------------+
                                       |nsImageWhatever|
                                       +---------------+

Platform independent code in nsImage* would be moved to gfxImageFrame.  Things
like GetWidth, GetHeight, DecodedRect, GetBytesPix, etc.  All the platform
dependent code would stay in nsImage*.  Any calls/references to nsIImage would
be changed to gfxImageFrame.

Comment 30

[dcone (gone)]

I don't like combining those two.. at all.  gfxImageFrame does frame things and
nsImage handles bitmaps.  Its not a good place to combine classes.  I really
believe that nsImage.. of all classes is named right and does what it supposed
to do.  Its intuitive, encapulates what its supposed to..etc.  

Comment 31

[ArronM (:paper)]

dcone, have you looked at gfxImageFrame.cpp?  The only functions that have
considerable amount of code are SetImageData and SetAlphaData.. the rest of the
functions call nsImage* functions.

Even your patch for Bug 143046 adds a function to gfxIImageFrame, only to have
the code return something from nsImageWin.  It's a waste, and makes trying to
follow the code harder. gfxIImageFrame was intended to replace nsIImage.

Although I do agree the name gfxIImageFrame is less intuitive than nsIImage, I
don't feel I have enough authority to just rename gfxIImageFrame to nsIImage
(after the combine).  Some people might get gfxIImageFrame confused with
nsIImageFrame.

CCing tor.. I remember him having an interest this bug.  tor, any thought?

Comment 32

[dcone (gone)]

There are just a few things I don't like.. but there big things.

1.)  nsImage is for bitmaps/images.  I don't like the idea of having frame in
that class at all.  Its just a naming issue.. but I think its a big deal.  

2.)  Any frame methods at all in nsImage is wrong.  Its not the number of
lines.. its an encapsulation issue.. what are the boundaries of an object.

For me the bottom line is that the current implementation was true to OOD, which
makes things easier to follow.  If you have an image problem.. or want to extend
and image you know right were to go.  If we have things that need to work cross
platform then nsImage needs to have an implementation, but I really dont agree
at all with combining them.. I am on the other side.. I think it will make it
harder to maintain just because the name of the object that combines both will
not describe acurately what each does.  I think I understand what your trying to
say.. but its very important to me to have objects names and encapsulation
boundaries accurate.

Comment 33

[ArronM (:paper)]

Well, at least we do not have totally different views here. :)  Would this be to
more of your liking?

                           +----------+
                           |nsImageGTK|
                         _ +----------+
                         /|
+--------+    +-------+ /  +----------+
|nsIImage| -> |nsImage| -> |nsImageWin|
+--------+    +-------+ \  +----------+
                        _\|
                           +---------------+
                           |nsImageWhatever|
                           +---------------+

This is very similar looking to what we have now, with the exception of nsIImage
having 2 additional variables.. x and y.  Does it make sense that an image can
be set to a position?  
We do not need a whole class just to store x and y.  It's over abstraction. It's
like making a class for image without alpha layers and a seperate class class
for images with alpha layers.  Why do alpha layers belong in an image?  Because
opacity is part of an image.. just like where the image is located (x, y) is
also a part of the image.

Comment 34

[ArronM (:paper)]

I used "platform code" and "nsImage*" interchangably in this comment.  nsImage*
refers to all the classes that inherited from nsIImage (nsImageWin, nsImageGTK,
nsImageMac, to name a few).  Patches will be attached once I get them tested on
GTK and OS2.  :)
Here is the list of changes:

1) removed nsIImage.h, all references to the file have been removed, and all
references to nsIImage class have been changed to gfxIImageFrame
----------
2) removed "@mozilla.org/gfx/image/frame;2" and "@mozilla.org/gfx/image;1". 
Added "@mozilla.org/gfx/imageframe;1" and gave it the CID of GFX_IMAGEFRAME_CID.
 Is this okay, or should I assign a totally new CID?  My thinking is that
technically it's still the same class (gfxIImageFrame), with some functions from
nsIImage moved into it.  The only thing that changed is the @mozilla.org ID-name
thing.
----------
3) Made nsImage* inherit from nsImageFrame (which inherits from nsIImageFrame
which is an IDL).  See Comment #29 for nice piccy.
----------
4) replaced nsImage*::width, nsImage*::height, gfxImageFrame::mOffset, and
gfxImageFrame::mSize with gfxImageFrame::mBounds.
----------
5) All platforms had some variables in common.  These variable were moved to
gfxImageFrame as protected and are as follows:
     mRowBytes         Still set by platform code

     mAlphaRowBytes    Still set by platform code
     (mARowBytes)

     mBytesPerPixel    Set in gfxImageFrame::Init(). Removed setting variable
     (mNumBytesPixel)  from GTK, Mac, Win, BeOS, Photon, and XLib. OS2 never
                       had one (and set GetBytesPix incorrectly ;).  QT forces
                       mBytesPerPixel to 4.

     mAlphaWidth       Set to image width & height if mFormat said there was
     mAlphaHeight      an Alpha.  Otherwise 0.

     mAlphaDepth       Set in gfxImageFrame::Init() based on mFormat

     mIsOptimized      Defaults to false.  A couple of platforms are always
     (mOptimized)      optimized, so they override in their init to PR_TRUE.
                       The rest set it when the image actually gets optimized
                       (if at all)

     mIsTopToBottom    I only saw one function using this.. the rest used
                       "#if defined(XP_WIN) || defined(XP_OS2)" to determine
                       how the image was stored.  Defaults to PR_TRUE in
                       gfxImageFrame constructor.. Windows and OS2 change it
                       to PR_FALSE in Init()
----------
6) The following were moved from nsIImage to gfxIImageFrame. Names below are
after xpidl makes the header file:
     GetImageBytesPix was nsIImage::GetBytesPix

     GetBitInfo       now returns length of BitInfo as well as the pointer

     GetHasAlphaMask  gfxImageFrame returns true if mAlphaDepth > 0.
                      Platforms overide with a null check to their private
                      AlphaBits.

     GetAlphaDepth

     drawToImage      nsIImage had two draw functions. IDL doesn't support
     draw             overloading, so one was renamed to DrawScaled.  In alot
     drawScaled       of platforms, draw() just called drawScaled, so that's
     drawTile         what gfxImageFrame::draw does.  GTK, BeOS, Photon, QT,
                      and Xlib all override draw().  Everyone (must) overide
                      drawToImage, drawScaled, and drawTile.  Although in the
                      future, we should implement a generic
                      gfxImageFrame::DrawToImage.  GTK, Windows, and other
                      platforms use almost the same DrawToImage code.
                      gfxIImageFrame had a function called drawTo.  This was
                      renamed to the more descriptive drawToImage.

     ImageUpdated     Code calling ImageUpdated was changed so it didn't
                      need to use nsIInterfaceRequestor to get nsIImage
                      from gfxImageFrame.
                      Moved the SetDecodedRect call from SetImageData to here
                      and made ever nsImage* call gfxImageFrame::ImageUpdate

     SetDecodedRect   This really should be removed (in another bug).. I
                      think ImageUpdated should be doing the decoded
                      variable(s) setting
----------
7) The following functions (below) were moved to gfxImageFrame as protected. 
This means no external classes/code called them.  They can be IDL'd later (when
something actually uses them..) or deleted.
     IsOptimized
     Optimize           SetMutable calls optimize

     GetBits            mainly used by GetImageData

     GetAlphaBits       mainly used by GetAlphaData

     GetAlphaWidth      Neither are currently used
     GetAlphaHeight

     Set/GetAlphaLevel  Not used or implemented at all.  A few platforms store
                        this to mAlphaLevel, but never use it.  I was very
                        tempted to remove the functions all together.

     GetDecoded[XY][12] Currently not used at all.  We should move to a nsRect
                        structure like nsImageOS2 does anyway.

     GetIsRowOrderTopToBottom

     GetColorMap        Was only used by Windows' nsImageClipboard.  I see no
                        evidence that the nsImageClipboard is used (or works),
                        so I replaced it with something I think would work
                        equally as well (and doesn't use GetColorMap)
----------
8) The following functions from nsIImage were removed:
     GetNaturalWidth    \
     SetNaturalWidth     |__ Removed variables too.  See Bug 116649
     GetNaturalHeight    |
     SetNaturalHeight   /

     GetLineStride       gfxIImageFrame already had GetImageBytesPerRow
     GetAlphaLineStride      "             "     "  GetAlphaBytesPerRow

     UnlockImagePixels   These two functions locked either Alpha or Data
     LockImagePixels     depending on the passed in flag.  gfxIImageFrame
                         already had LockImageData, LockAlphaData,
                         UnlockImageData, UnlockAlphaData, so both nsImage*
                         functions were split into two and renamed.
                         Several platforms don't lock alpha/image pixels so
                         gfxImageFrame::(Un)LockImagePixels just returns NS_OK
                         (the other platforms override with (un)locking)
----------
9) nsIInterfaceRequestor removed from gfxIImageFrame.  From what I can tell, it
was only used to request the nsIImage stored inside gfxImageFrame.
----------
10) nsRenderingContextImpl::DrawImage and
nsRenderingContextImpl::DrawScaledImage would convert the src and dst coords so
that they are relative to the image data.  For example, if an imageframe started
at (10,10) and we needed to draw starting at (15,15), 10 was stripped from sr.x
and sr.y, leaving (5,5) into the image data.   This functionality has been moved
to gfxImageFrame as SetRelativeToImageData(), and is called upon
nsImage*::Draw() and nsImage*::DrawScaled.

Comment 35

[ArronM (:paper)]

erk. Regarding Comment 3
That's gfxImageFrame, not nsImageFrame 

Comment 36

[ArronM (:paper)]

Stuart has some major gripes.. changing target milestone (again :P).  The gripes
I remember are:

1) move draw* stuff to nsRenderingContext*

2) Get rid of decoded rect stuff.  Layout should be doing that.  Write code in
layout to remember decoded sections.

Comment 37

[dcone (gone)]

I totally disagree with that plan.. your plan in 33 was acceptable.. I dont like
at all moving anything from nsIImage into gfxIImageFrame or anything that has
the word frame on it.  The image stuff should stay image.. it has nothing to do
with frames.
I will fight tooth and nail not to let happen what your proposing.. its just
plain and simply wrong.  I CANT STRESS THAT ENOUGH!!!!!

Also.. I am trying to get in some GIF changes. so messing around in this area
will be a major nightmare.  If you feel the need to do something.. and you have
some reasons for doing it .. then do what you propose in comment 33.  But Do not
re-distribute or change nsIImage.. that is a bitmap class.... PLEASE.

Comment 38

[ArronM (:paper)]

don't worry, this bug is far from being resolved.  I want to clean up nsIImage's
stucture first.  That's structure, not really code, nor combining classes.  I'll
be filing other bugs for that, and this one will probably have no action for a
while (re: long time). I'll be sure to cc you, dcone.

One thing that hasn't changed though, is that I still believe it's silly to have
all those calls in gfxIImageFrame just calling nsImage*.

Comment 39

[Alec Flett]

As far as "not having a reason": These are some perfectly legitimate reasons to
schedule the landing of such a fix among other fixes that touch this same code,
in loose order of importance:
- code size reduction (includes reducing code duplication)
- performance improvement
- reduced dependencies
- refactoring that makes the code easier to understand and maintain

This bug seems to be driven by at least a few of these reasons, and I think we
should do what we can to fix this the right way (And that means dcone and
stuart/paper coming to an agreement) - there's no need for drama but if we reach
an impasse over specific parts of the proposed changes, then we need to bring
the plan before the layout module owners and the super-reviewers, to decide what
the best course of action is.

Comment 40

[ArronM (:paper)]

Fortunately, a lot of the work can be done without (hopefully) touching the
differences of opinion over this bug.

I've created Bug 181695 and Bug 182658, and I think there will be less issues
with them. :)  There's also Bug 181917, which isn't directly decending from this
bug, but still may be relative to people CC'd here.

Comment 41

[Joe Drew (not getting mail)]

I'm currently working on this.

Comment 42

[Joe Drew (not getting mail)]

Attachment: create imgFrame, a combination of nsThebesImage and gfxImageFrame (v0)

This is not complete yet (most of Gecko is not converted), but libpr0n compiles and links.

Basically, we want to disallow access to individual frames as the frames themselves, and instead add accessors to imgContainer for the relevant bits. This ensures that nobody has to worry about frames being a different size from the image, and also disallows access to the bits of an image from outside imagelib (modulo making a separate copy in a gfxImageSurface, which will be offered as the solution). This lets us keep images optimized, and stops the silly LockImagePixels usage that nobody should need.

My #1 concern, and the reason for my sr request right now, is the imgContainer API: how it looks, how it feels, and whether it's okay to make a bunch of stuff C++-only. I suspect it is, but I want to defer to people smarter & more experienced than myself.

Incidentally:
 40 files changed, 1589 insertions(+), 2503 deletions(-)

Comment 43

[Joe Drew (not getting mail)]

Attachment: imgFrame patch v0.1

This patch removes all remaining references to nsIImage and gfxIImageFrame. libpr0n still compiles, but I haven't tried compiling anything else.

 90 files changed, 1866 insertions(+), 3032 deletions(-)

Comment 45

[Joe Drew (not getting mail)]

Attachment: imgFrame patch v1

This patch compiles on OS X. (It crashes on startup.) I've sent it to the try server for compiles on other operating systems.

At this point, I'd like super-review of the API-level changes from Vlad, and, if possible, the changes in each individual's area (svg, layout, content, etc). Most of the changes in the users of imagelib are somewhat mechanical, but it's possible I've messed things up right good.

My main concern with regard to API change is the fact that I've removed Javascript users' access to raw image data - nsIImage and gfxIImageFrame are gone, and imgIContainer exposes gfxASurface and gfxImageSurface to noscript callers only. (I don't think we actually had users of that raw image data, but it's something to keep in mind.)

Comment 46

[Joe Drew (not getting mail)]

Comment on attachment 384515 [details] [diff] [review]
imgFrame patch v1

surely there are some people who work on platform not r? on this bug.

Comment 47

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 384515 [details] [diff] [review]
imgFrame patch v1

on file: modules/libpr0n/public/imgIContainer.idl line 72
> interface imgIContainer : nsISupports

is imgIContainer still the right name for this?


on file: modules/libpr0n/public/imgIContainer.idl line 117
>   [noscript] imgIContainer extractCurrentFrame([const] in nsIntRect aRegion);

extractCurrentFrame is only used by border-image, apparently because it wants
to use the layout helpers which need an imgIContainer.  Would be nice to be
able to remove this function -or- to at least consolidate it with the other
getters (see below).

Also, the argument is an nsIntRect; change the param to aRect, not aRegion.


on file: modules/libpr0n/public/imgIContainer.idl line 139
>   [noscript] void getCurrentFrameRect(in nsIntRect rect);

It seems a lot of the APIs operate either on an arbitrary frame N or the
current frame.  If we want to have both, I'd suggest that they mirror
eachother exactly; that is, have both a getCurrentFrameRect and a
getFrameRect(n).

Another (potentially cleaner) option is to have a getFrameContainer(n) that
would return a new imgIContainer for frame N, but would share data with the
main/parent container. 


on file: modules/libpr0n/public/imgIContainer.idl line 172
>   [noscript] readonly attribute gfxImageSurface currentFrame;

This is a readonly attribute, whereas the very similar
getCurrentFrameSurface() below is a function.  Should probably pick one or the
other.


on file: modules/libpr0n/public/imgIContainer.idl line 178
>   [noscript] gfxASurface getCurrentFrameSurface();

There are 4 different getters here:

imgIContainer extractCurrentFrame(nsIntRect r);
gfxImageSurface getFrame(uint num);
gfxImageSurface getCurrentFrame();
gfxASurface getCurrentFrameSurface();

It seems like these could be consolidated.  I mentioned extractCurrentFrame
above; might be able to get rid of it entirely.

For the others, I would suggest having everything return gfxASurface, and have
callers pass a flag saying whether they want an image surface or would be ok
with a platform-native surface.  Also, a flag could be passed that indicates
whether the caller wants a surface it can modify, or whether a read-only
(shared) surface is fine.  That would get us down to 2 API calls, a
getCurrentFrame and getFrame(int n).

At that point, getCurrentFrame becomes a convenience function, since we can
already grab the right index from currentFrameIndex.  Is it worth having that
convenience function around?  I'd say yes, since 90% of the time it's what you
want, but it would be a smaller API surface to not have it.

Comment 48

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

Some comments on imgIContainer.idl. Generally I like it.

I think '* @see "gfx2"' can go.

imgIContainer needs a big fat comment explaining exactly what an imgIContainer represents. People need to understand what the difference is between an "image" and an "image container". Or maybe it should just be called nsIImage and lose the "Container"... or heck, even mozilla::IImage?

Do the frame-combining constants at the start of imgIContainer really need to be exposed?

+   * Whether the current frame needs the background painted behind it.
+   */
+  readonly attribute boolean currentFrameNeedsBackground;

Wouldn't isOpaque be more obvious?

Can we make imgIContainer non-scriptable? That would make it easier for C++ users to use.

+  [noscript] void getCurrentFrameRect(in nsIntRect rect);
+  unsigned long getFrameImageDataLength(in unsigned long framenumber);
+  void getFrameColormap(in unsigned long framenumber, 
+                        [array, size_is(paletteLength)] out PRUint8 paletteData,
+                        out unsigned long paletteLength);
+  void setFrameDisposalMethod(in unsigned long framenumber, in PRInt32 aDisposalMethod);
+  void setFrameBlendMethod(in unsigned long framenumber, in PRInt32 aBlendMethod);
+  void setFrameTimeout(in unsigned long framenumber, in PRInt32 aTimeout);
+  void setFrameHasNoAlpha(in unsigned long framenumber);
+  [noscript] void ensureCleanFrame(in unsigned long aFramenum, in PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in gfxImageFormat aFormat,
+                                   [array, size_is(imageLength)] out PRUint8 imageData, out unsigned long imageLength);
+  [noscript] void appendFrame(in PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in gfxImageFormat aFormat,
+                              [array, size_is(imageLength)] out PRUint8 imageData, out unsigned long imageLength);
+  [noscript] void appendPalettedFrame(in PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in gfxImageFormat aFormat, in PRUint8 aPaletteDepth, 
+                                      [array, size_is(imageLength)] out PRUint8 imageData, out unsigned long imageLength,
+                                      [array, size_is(paletteLength)] out PRUint8 paletteData, out unsigned long paletteLength);
+  [noscript] void frameUpdated(in unsigned long framenum, in nsIntRect aNewRect);
   void endFrameDecode(in unsigned long framenumber);
   void decodingComplete();
  [noscript] void setDiscardable(in string aMimeType);
  [noscript] void addRestoreData([array, size_is(aCount), const] in char data,
                                 in unsigned long aCount);
  [noscript] void restoreDataDone();

Can we avoid exposing these? Looks like only imglib uses them, and outsiders shouldn't be using them. I think it's probably worth having an abstract interface that clients use and a concrete subclass that imglib uses. init() should move to the concrete class too. It's actually quite important we ensure that outsiders don't try to get the frame count, for example, since we'll be able to have animated SVG images that don't actually have a frame count.

At the very least, the methods that outsiders use should be clearly separated in the IDL and documentation.

  attribute unsigned short animationMode;
  void startAnimation();
  void stopAnimation();
  void resetAnimation();
  attribute long loopCount;

There must be some redundancy here. Seems like at least we could get rid of animationMode and rename loopCount to animationCount; then animationCount==0 would mean no animation, animationCount==1 would mean just 1 cycle and then stop, animationCount==INT32_MAX would mean unlimited cycles. Fodder for another bug perhaps.

+  [noscript] readonly attribute gfxImageSurface currentFrame;
+  [noscript] gfxASurface getCurrentFrameSurface();

Echoing Vlad, I don't know why we need to have both currentFrame and getCurrentFrameSurface. Shouldn't getCurrentFrameSurface be enough? If someone needs a gfxImageSurface they can check the type of the surface returned by getCurrentFrameSurface; if it's not a gfxImageSurface already, they can copy the gfxASurface to a gfxImageSurface manually. That's hardly any code, and if they don't need to optimize for the already-gfxImageSurface case, it's even less code. I don't think Vlad's flag idea is necessary, but I wouldn't object.

+  [noscript] gfxImageSurface getFrame(in unsigned long framenumber);

I'd quite like to change this since it's going to be a problem for animated SVG images, which don't have numbered frames. It looks like all current users are either getting frame 0 or the last decoded frame. So how about
  gfxASurface getFirstFrameSurface();
  gfxASurface getLastFrameSurface();
?

I would like to keep extractCurrentFrame, since it lets you get an object representing a subregion without necessarily going through a gfxASurface. For SVG images that could be very useful.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> I don't think we actually had users of that raw image data

We sure do: extensions.  At least last I checked.  They might be able to move to <canvas> imagedata, especially if we expose a way for them to draw the objects involved into a canvas...  But worth checking AMO for such extensions and looking into exactly what they're doing.

Comment 50

[Joe Drew (not getting mail)]

Mark Finkle used the AMO source code search and found that there are no users of gfxIImageFrame::getImageData, which is good news - everyone already uses canvas, it seems.

Comment 51

[Mook (songbird dead account)]

(In reply to comment #50)
> Mark Finkle used the AMO source code search and found that there are no users
> of gfxIImageFrame::getImageData, which is good news - everyone already uses
> canvas, it seems.

Note that the AMO source code search would only hit JS source, not C++ in extensions.  joe suggested that C++-based extensions move to use gfxImageSurface instead, if I remember correctly - and I probably don't.  <canvas> can't be used from any language other than JS.  I have no idea how frozen gfxImageSurface is, given that it's not IDL.  If this is as "frozen" as nsIFrame (i.e. not at all), this may mean there will be future crashes.

Comment 52

[Justin Dolske [:Dolske]]

Given bug 315357, I would hope there are no JS callers.

Comment 53

[Bobby Holley (:bholley)]

this patch is pretty bitrotted in the places where it touches other code (I get lots of rejections based on june commits by roc and dbaron). Any chance of merging the conflicts and posting one that applies cleanly?

Comment 54

[Joe Drew (not getting mail)]

Attachment: updated to tip

This is an updated-to-tip, slightly-fixed version of this patch. (PNGs don't display, but GIFs do!)

Comment 55

[Joe Drew (not getting mail)]

Attachment: updated to tip redux

Whoops, hg qclone didn't do what I wanted it to. This is a more up-to-date patch including fixes to gifs not decoding, etc.

Comment 56

[Bobby Holley (:bholley)]

I'm building my decode-on-draw patch on top of this one, and I've run into issues with ExtractCurrentFrame. It seems wrong to me, because this is the only case where we create an imgContainer for something that didn't come from a source image (jpg, png, gif, etc). I'm trying to associate a meaningful mimetype and decoder with each imgContainer, so this case sticks out.

 What's more, this function really _wants_ to return an imgFrame, not an imgContainer. Currently, the only use case is border-image, where essentially we extract this imgContainer, hold onto it for a while, and then call Draw() on it, which in turn just pulls the imgFrame back out and calls Draw() on the frame. So more or less it's just boxing/unboxing.

I'm not positive (I'm still relatively new to XPCOM), but my impression is that the reasoning here is that we have no imgIFrame interface, and we only want to return interfaces in idl specifications. If this is true, and if we agree with roc that we want a way to get image frames without using gfxASurface, maybe it's worth considering making an imgIFrame (certainly with less stuff than the gfxImageFrame that we're eliminating with this patch).

thoughts?

Comment 57

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

I wouldn't want to expose imgIFrame just for this one case. That adds complexity for image users. Having an Extract operation that returns the same type of object is nice in layout because it means all our image-drawing helpers only need to have one kind of signature.

To implement this, you could have a single completely different imgIContainer implementation that points to the original imgIContainer and contains the slice coordinates. Its draw() method would delegate to the original draw() method appropriately. That would work, and could be better optimized if we feel the need. How does that sound?

Comment 58

[Joe Drew (not getting mail)]

Attachment: updated to tip, PNGs work

This patch is updated to tip. PNGs and JPEGs both work, and GIF non-animations work. Unfortunately there's still a bug in paletted GIF animations - it looks like we're either sampling the wrong part of the palette, or we've got the wrong palette on each frame, I'm not sure which.

Comment 59

[Boris Zbarsky [:bzbarsky]]

So... unless you want everyone on the reviewer list to review the whole patch, it might be nice to say which one is supposed to review what.

Comment 60

[Joe Drew (not getting mail)]

I just want an initial check-for-sanity from the reviewers, not a full review yet.

bzbarsky: content
jwatt: svg
roc: layout
jmathies: win32 bits
karlt: gtk bits

And, if it interests any of you, a check-for-sanity on the imgIContainer interface changes, as roc and vlad did above.

Comment 61

[Joe Drew (not getting mail)]

Comment on attachment 385492 [details] [diff] [review]
updated to tip, PNGs work

Suspect karl meant to put this here:

 win32 and mac clipboards seem to use the current frame, but gtk seems to use the initial frame. Can gtk be made consistent with the others? (If nsImageToPixbuf::PatternToPixbuf() is still no longer needed then that can be removed.)

Comment 62

[Rob Arnold [:robarnold]]

Attachment: Compiles on windows

Haven't run tests but this will compile and the browser runs and looks ok.

Comment 63

[Joe Drew (not getting mail)]

Attachment: updated to tip, integrate robarnold's changes, GIFs work

The previous patches reflected a fundamental misunderstanding on my part of what datatype a palette/colormap should be stored as, and it turns out that fixing that fixes my GIF animation problems too.

This patch also integrates Rob Arnold's changes (I believe all of them - had some interdiff problems, so please check) to make it compile on Windows and be updated to tip.

Comment 64

[Joe Drew (not getting mail)]

Attachment: Fix imgITools xpcshell test

Comment 65

[Joe Drew (not getting mail)]

Attachment: fixed drawing images to canvas and border-image; merged to m-c

This patch is currently making its way through the try server.

Comment 66

[Joe Drew (not getting mail)]

Attachment: fix icon files not having an alpha channel; fix some compilation errors

This is very close. Still remaining are any further build failures (currently running through the try server) and updating for reviewers' comments earlier.

I will be splitting this into quite a number of patches for the purposes of review.

Comment 67

[Joe Drew (not getting mail)]

Oh, also, all our tests pass now, at least on OS X.

Comment 68

[Joe Drew (not getting mail)]

Attachment: fix SVG image reftest; fix compilation on Linux

This patch is updated to tip again. It also fixes an SVG image scaling reftest and compilation on Linux. Currently pushed to try.

Comment 69

[Joe Drew (not getting mail)]

(In reply to comment #47)
> (From update of attachment 384515 [details] [diff] [review])
> on file: modules/libpr0n/public/imgIContainer.idl line 72
> > interface imgIContainer : nsISupports
> 
> is imgIContainer still the right name for this?

No; bug 503973 will track the changes required to split imgIContainer into two separate interfaces, the non-libpr0n version should probably be called mozilla::Image. (Does xpidl support namespaces?)

(In reply to comment #48)
> Can we make imgIContainer non-scriptable? That would make it easier for C++
> users to use.

No, because imgITools uses it.
 
> At the very least, the methods that outsiders use should be clearly separated
> in the IDL and documentation.

Agreed; I'll do this in this bug, and the rest of the changes in bug 503973.
 
>   attribute unsigned short animationMode;
>   void startAnimation();
>   void stopAnimation();
>   void resetAnimation();
>   attribute long loopCount;
> 
> There must be some redundancy here. Seems like at least we could get rid of
> animationMode and rename loopCount to animationCount; then animationCount==0
> would mean no animation, animationCount==1 would mean just 1 cycle and then
> stop, animationCount==INT32_MAX would mean unlimited cycles. Fodder for another
> bug perhaps.

Yeah, please file another bug.
 
> +  [noscript] readonly attribute gfxImageSurface currentFrame;
> +  [noscript] gfxASurface getCurrentFrameSurface();
> 
> Echoing Vlad, I don't know why we need to have both currentFrame and
> getCurrentFrameSurface. Shouldn't getCurrentFrameSurface be enough? If someone
> needs a gfxImageSurface they can check the type of the surface returned by
> getCurrentFrameSurface; if it's not a gfxImageSurface already, they can copy
> the gfxASurface to a gfxImageSurface manually. That's hardly any code, and if
> they don't need to optimize for the already-gfxImageSurface case, it's even
> less code. I don't think Vlad's flag idea is necessary, but I wouldn't object.

I'd prefer Vlad's idea; no need to have boilerplate code converting to gfxImageSurfaces all over the place.

> +  [noscript] gfxImageSurface getFrame(in unsigned long framenumber);
> 
> I'd quite like to change this since it's going to be a problem for animated SVG
> images, which don't have numbered frames. It looks like all current users are
> either getting frame 0 or the last decoded frame. So how about
>   gfxASurface getFirstFrameSurface();
>   gfxASurface getLastFrameSurface();

Is it going to be easy to get an SVG file's first frame surface? If not, I see no reason why we can't just give consumers current frame only. Most places that want the 0th frame just want "a" frame, and IMO current is as good as first.

> I would like to keep extractCurrentFrame, since it lets you get an object
> representing a subregion without necessarily going through a gfxASurface. For
> SVG images that could be very useful.

extractCurrentFrame is sort of ugly, but necessary for border-image without making a lot of other changes. If we want to get rid of it, I'd prefer

Comment 70

[Joe Drew (not getting mail)]

Don't know how that got cut off. If we want to get rid of it, I'd prefer someone to file a followup bug on layout, and I'll do the fix in libpr0n.

Comment 71

[Joe Drew (not getting mail)]

Attachment: code complete

This patch compiles on Windows, Linux, and OS X, and I believe it also passes all our tests (though I'm running through the try server to be sure). It fixes crashes caused by the gfx module not being initialized by tests that don't explicitly use anything inside the gfx module, and it also implements the API changes asked for by roc and vlad in initial API review.

Please look this over; I will be splitting this patch up into multiple parts for formal review tomorrow.

Comment 72

[Joe Drew (not getting mail)]

Attachment: The new API this patch introduces, as well as changes in libpr0n required for it

This is the actual API changes, and the part that needs super-review. I'd like both roc and vlad to SR this, but everyone should take a look to make sure that imgIContainer makes sense for their uses.

Comment 73

[Joe Drew (not getting mail)]

Attachment: cocoa widget changes

Comment 74

[Joe Drew (not getting mail)]

Attachment: content changes (no svg)

Comment 75

[Joe Drew (not getting mail)]

Attachment: layout changes (no SVG)

Comment 76

[Joe Drew (not getting mail)]

Attachment: SVG changes (content and layout)

Comment 77

[Joe Drew (not getting mail)]

Attachment: changes to image decoders required by changes to imgIContainer API

Comment 78

[Joe Drew (not getting mail)]

Attachment: changes to GTK and GNOME-specific widget code

Comment 79

[Joe Drew (not getting mail)]

Attachment: Windows widget changes

Comment 80

[Joe Drew (not getting mail)]

Attachment: removal of nsIImage and gfxIImage in gfx/

Comment 81

[Joe Drew (not getting mail)]

Attachment: rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

Comment 82

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

Qt part builds and runs fine

Comment 83

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

Comment on attachment 388529 [details] [diff] [review]
The new API this patch introduces, as well as changes in libpr0n required for it

+  [noscript] void getCurrentFrameRect(in nsIntRect aFrameRect);
+  readonly attribute unsigned long currentFrameIndex;
+  readonly attribute unsigned long numFrames;

Shouldn't these be internal-only?

+  /**
+   * Get a copy of the current frame that you can write to and otherwise
+   * inspect the pixels of.
+   */
+  [noscript] gfxImageSurface copyCurrentFrame();

The comment should be clearer that the caller gets exclusive ownership of and access to the surface.

Comment 84

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

+  [noscript] void onDataAvailable(in imgIRequest aRequest, in unsigned long aFrame, [const] in nsIntRect aRect);

Instead of perpetuating public frame indices --- which need to go away --- how about just a boolean aIsCurrentFrame here?

Comment 85

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

Comment on attachment 388532 [details] [diff] [review]
layout changes (no SVG)

Looks good, except for the part where we still have to deal with frame indices.

Comment 86

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

Comment on attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

xpwidgets changes are good

Comment 87

[Jim Mathies [:jimm]]

Comment on attachment 388538 [details] [diff] [review]
Windows widget changes

win widget changes look good to me.

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 388531 [details] [diff] [review]
content changes (no svg)

> +  return imgContainer.forget().get();

That should just be .forget(), without the .get().

With that change, r=bzbarsky.

Comment 89

[Joe Drew (not getting mail)]

(In reply to comment #83)
> (From update of attachment 388529 [details] [diff] [review])
> +  [noscript] void getCurrentFrameRect(in nsIntRect aFrameRect);
> +  readonly attribute unsigned long currentFrameIndex;
> +  readonly attribute unsigned long numFrames;
> 
> Shouldn't these be internal-only?

Yes - I will add an external |readonly attribute boolean animated| to handle the current users, which are really just checking for that.

(In reply to comment #84)
> +  [noscript] void onDataAvailable(in imgIRequest aRequest, in unsigned long
> aFrame, [const] in nsIntRect aRect);
> 
> Instead of perpetuating public frame indices --- which need to go away --- how
> about just a boolean aIsCurrentFrame here?

OK.

Comment 90

[Peter Weilbacher]

Comment on attachment 388540 [details] [diff] [review]
rest of changes required (os2, qt widget changes; libeditor changes; xpwidget changes; build changes)

Joe, thanks for thinking of the OS/2 code and for the chance to review this.

>-  nsCOMPtr<gfxIImageFrame> frame;
>-  aCursor->GetFrameAt(0, getter_AddRefs(frame));
>+  nsRefPtr<imgIContainer> frame;
>+  aCursor->CopyCurrentFrame(getter_AddRefs(frame));

This gives me

M:/central/mozilla/widget/src/os2/nsWindow.cpp:1684: error: no matching function for call to 'imgIContainer::CopyCurrentFrame(nsRefPtrGetterAddRefs<imgIContainer>)'
../../../dist/include/imgIContainer.h:99: note: candidates are: virtual nsresult imgIContainer::CopyCurrentFrame(gfxImageSurface**)

Using nsRefPtr<gfxImageSurface> as suggested by the compiler works much better.

>   // if the image is ridiculously large, exit because
>   // it will be unrecognizable when shrunk to 32x32
>-  PRInt32 width, height;
>-  frame->GetWidth(&width);
>-  frame->GetHeight(&height);
>+  PRInt32 width = frame->Width();
>+  PRInt32 height = frame->Height();;

Nit: please remove the second semicolon here.

>-HBITMAP nsWindow::CreateTransparencyMask(gfx_format format,
>+HBITMAP nsWindow::CreateTransparencyMask(gfxASurface::gfxImageFormat format,

Please adapt the prototype in nsWindow.h accordingly.

Otherwise this seems to work. :-) So r+ me with these changes.

Comment 91

[Joe Drew (not getting mail)]

Attachment: changes as requested by roc in review (interdiff)

Comment 92

[Joe Drew (not getting mail)]

Attachment: Full patch including requested changes

Comment 93

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

+/* [noscript] void onDataAvailable (in imgIRequest request, in unsigned long frame, [const] in nsIntRect rect); */
 NS_IMETHODIMP imgRequest::OnDataAvailable(imgIRequest *request,

You need to fix the type of the second parameter in the comment

Comment 94

[Jim Mathies [:jimm]]

A note for drivers, a number of win7 specific features rely on this, so if possible it would be great if we could get this into 3.6.

Comment 95

[Karl Tomlinson (:karlt)]

Comment on attachment 388537 [details] [diff] [review]
changes to GTK and GNOME-specific widget code

>- * An interface that allows converting an nsIImage to a GdkPixbuf*.
>+ * An interface that allows converting the first frame of an imgIContainer to a GdkPixbuf*.

"the _current_ frame" would describe what is currently happening.

>-    return PatternToPixbuf(pattern, width, height);

Please remove the unused PatternToPixbuf method.

>     // Get first image frame
>-    nsCOMPtr<gfxIImageFrame> frame;
>-    aCursor->GetFrameAt(0, getter_AddRefs(frame));
>-    if (!frame)
>-        return NS_ERROR_NOT_AVAILABLE;
>-
>-    nsCOMPtr<nsIImage> img(do_GetInterface(frame));
>-    if (!img)
>-        return NS_ERROR_NOT_AVAILABLE;
>-
>-    GdkPixbuf* pixbuf = nsImageToPixbuf::ImageToPixbuf(img);
>+    GdkPixbuf* pixbuf = nsImageToPixbuf::ImageToPixbuf(aCursor);

Is the current frame likely to be the first frame?
If so, please add a comment to explain.

Or, are you changing the behavior because you think the current frame is as
good as the first?  If so, the comment needs updating.

Comment 96

[Karl Tomlinson (:karlt)]

Comment on attachment 388537 [details] [diff] [review]
changes to GTK and GNOME-specific widget code

r+ assuming the comments above are addressed

Comment 97

[Joe Drew (not getting mail)]

Attachment: Final patch, including all review changes

This patch includes all changes requested by review, and is ready to go.

Comment 98

[Ryo Kawaguchi]

Hi Joe, I was working on your patch and found a small mistake.

@@ -1682,37 +1680,19 @@ nsCSSRendering::PaintBackgroundWithSC(ns
+    PRBool isOpaque;
+    if (NS_SUCCEEDED(image->GetCurrentFrameIsOpaque(&isOpaque)) && !isOpaque)
+      drawBackgroundColor = PR_TRUE;

This should be:

+    if (NS_SUCCEEDED(image->GetCurrentFrameIsOpaque(&isOpaque)) && isOpaque)
+      drawBackgroundColor = PR_FALSE;

Comment 99

[Daniel Holbert [:dholbert]]

Yeah -- to provide a little more code-context, note that the chunk Ryo quotes in comment 98 is inside an "if( ... && drawBackgroundColor) {" clause.  So, drawBackgroundColor is already flipped *on* when we hit that code (and we want to flip it *off* as an optimization, if the frame is opaque).

Comment 100

[Joe Drew (not getting mail)]

Attachment: small changes

Wow, thanks so much! If you find anything else, please let me know.

This patch is some small changes noticed by others - the mistake Ryo noticed, as well as some code that re-used surfaces if it could, left over from before copyCurrentFrame was explicitly called "copy."

Comment 101

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/b94bc4be53ca

Comment 102

[Takanori MATSUURA]

We also take care of
content/canvas/src/nsCanvasRenderingContextGL.cpp
content/canvas/src/nsCanvasRenderingContextGL.h
content/canvas/src/nsCanvasRenderingContextGLWeb20.cpp

Comment 103

[Takanori MATSUURA]

(In reply to comment #102)
Filed to bug 505663.

Comment 104

[[e-mail]]

Comment on attachment 389536 [details] [diff] [review]
Final patch, including all review changes

>+EXTRA_DSO_LDOPTS += $(LIBXUL_DIST)/lib/$(LIB_PREFIX)thebes.$(LIB_SUFFIX)
Are these symbols exported from xul.dll or otherwise accessible from xulrunner?

Comment 105

[Joe Drew (not getting mail)]

Yes, Thebes is exported from xul.

Comment 106

[Walter Meinl]

(In reply to comment #100)

> Wow, thanks so much! If you find anything else, please let me know.
Joe,
the OS/2 build breaks, cause somehow in your final version of the patch you changed in widget/src/os2/nsWindow.cpp 2nd hunk
  nsRefPtr<imgIContainer> frame;
(in the first version, https://bugzilla.mozilla.org/attachment.cgi?id=388540, that got r+ from peterw) that works and is in all other widget files
to
  nsRefPtr<gfxImageFrame> frame;
which doesn't work as you removed it.
Is it possible to fix this typo here or do you want me to file a follow-up bug?

Comment 107

[Walter Meinl]

(In reply to comment #106)
> (In reply to comment #100)
Ah, from comment #90 it should be nsRefPtr<gfxImageSurface> and not nsRefPtr<gfxImageFrame> and that indeed works. In comment #106 I was a bit to early for pressing commit nsRefPtr<imgIContainer> doesn't compile.

Comment 108

[Joe Drew (not getting mail)]

If there remains a problem, please file a followup bug, and I'll fix it. :)

Comment 109

[Serge Gautherie (:sgautherie)]

"can't read .../gfx/src/shared/Makefile.in: No such file or directory"

Please, remove the following line too:

/toolkit/toolkit-makefiles.sh
    * line 122 -- gfx/src/shared/Makefile

Comment 110

[Joe Drew (not getting mail)]

Please file followup bugs for any remaining issues.