Setting all current Open/Normal to M4.

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

Reassigning qa contact to elig@netscape.com

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.

Moving to M9

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

Moving to M12

Moving to M14

Don, could you look at this?

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.

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

Reassigning to dcone.

pavlov, fyi

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

You can take this Pavlov

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

Moving bugs to new Image: GFX component

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

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

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

reassigning for real this time

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

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 :-)

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?

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?

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.

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.

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.

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

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.

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.

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?

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.

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.

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.

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

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.

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.

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

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.

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.

I'm currently working on this.

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

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

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 on attachment 384515 [details] [diff] [review] imgFrame patch v1 surely there are some people who work on platform not r? on this bug.

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.

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.

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

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.

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

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

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?

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

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.

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?

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?

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.

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.

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

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

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.

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

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.

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

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

(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

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.

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.

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

+ [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 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 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 on attachment 388538 [details] [diff] [review] Windows widget changes win widget changes look good to me.

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.

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

+/* [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

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 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 on attachment 388537 [details] [diff] [review] changes to GTK and GNOME-specific widget code r+ assuming the comments above are addressed

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

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;

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

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

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

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

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?

Yes, Thebes is exported from xul.

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

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

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

"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

Please file followup bugs for any remaining issues.