143046 - (slowGIF) Need to Keep GIFs at original 8 bit or optimized.

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P2)

Description

[dcone (gone)]

Right now all the GIF's are kept internally as 24 bit, this needs to go to 8 bit 
to save space and time for the animated GIF's.  This can be a hug waste of time 
if we need to write the 24 bit images for each animation cycle.

Comment 1

[dcone (gone)]

Taking this

Comment 2

[dcone (gone)]

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

Comment 3

[Eugene Savitsky]

Will it fix a huge slowdawn when opened a few pages with many animated pix?

For example: http://www.o45m.ru/forum/read.php?f=4&i=3234&t=3233

Comment 4

[Markus Hübner]

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

Comment 5

[Andrew Hagen]

Is this a Platform/OS All/all bug?

Comment 6

[Christian 'CeeJay' Jensen]

If GIF's are kept as 24 bit internally , are they kept as 24 bit in the cache too ?
Are 8 (or lower) bit PNG's kept as 24 bit too or is this just a problem with GIF's ?

Would optimizing every image before it's stored in memory and in the diskcache
give any benifits when you account for the time the browser takes
optimizing/checking to see if it's optimized ?

Comment 7

[Markus Hübner]

Increasing priority as this one is blocking important bugs.

Comment 8

[dcone (gone)]

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

Comment 9

[Olivier Cahagne]

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

Comment 10

[Olivier Cahagne]

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

Comment 11

[Olivier Cahagne]

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

Comment 12

[Michael Lefevre]

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

Comment 13

[dcone (gone)]

Attachment: Update some interfaces.

Comment 14

[Scott Collins]

Comment on attachment 101403 [details] [diff] [review]
Update some interfaces.

typically one edits out the  1416 useless lines of '?' files not managed by CVS
before teh 326 of actual patch :-)

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Please also remove the ldap Makefile changes (/cygdrive/... etc).

Other than that, it looks like a good start, but the actual code to use that
depth info is still needed of course.

There will probably be a small perf hit for doing draw-time translations to
screen depth or 24 bits for compositing, but we'll save a fair amount of
footprint/cache space, and might have slightly better processor cache locality.
 It shouldn't affect pageload times.

Overall I'm definitely in favor of this.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Two quick nits..

1)  whitespace in gfx/idl/gfxIImageFrame.idl ?  Are those tabs or something?
2)  ", 24", not ",24" as you have it in a few places.

Comment 17

[tor]

Comment on attachment 101403 [details] [diff] [review]
Update some interfaces.

You probably don't really want to pass a depth of 12 for the jpeg decoder.

Comment 18

[dcone (gone)]

Attachment: Newer patch

I am sorry.. this first patch I put in without looking at the patch.. so all
that crap got in there.  This patch takes out all the garbage and fixes some
spacing issues and of course the famous 12 bits/pixel setting.	(I was just
seeing if you all were awake ;)  This patch will be the first of a few to get
GIF's saved at the original 8 bit per pixel.  I always feel safer checking in
these kinds of changes in pieces.. this interface change just seems like a
logical thing to do for our images anyway.

Comment 19

[Boris Zbarsky [:bzbarsky]]

In the idl, looks like tabs:

+            in gfx_format aFormat,
+			 in gfx_depth aDepth);

In the MNG decoder, need a space:

+  if (NS_FAILED(decoder->mImageFrame->Init(0, 0, width, height, format,24)))

In the PPM decoder, need a space:

+      mFrame->Init(0, 0, mWidth, mHeight, gfxIFormats::RGB,24);

Could we add an NS_PRECONDITION to the beginning of gfxImageFrame::Init that the
depth arg is sane?  Eg that it's a multiple of 8?  Or is either 8 or 24?  Or
something like that (I'm not sure what we would allow as "valid" values)?  

Comment 20

[dcone (gone)]

Attachment: Patch with fixed tabs, spaces and check for right depth.

Comment 21

[Boris Zbarsky [:bzbarsky]]

+    NS_ASSERTION(0, "This Depth is not supported\n");

NS_ERROR("This ...").

+  gfx_depth depth = aWidth;

aDepth, you mean?

sr=me with those changes...

Comment 22

[dcone (gone)]

Attachment: Fix for aDepth...

Comment 23

[rods (gone)]

Comment on attachment 101436 [details] [diff] [review]
Fix for aDepth... 

r=rods

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yikes.  No idea how I missed this...

+  if ( (aDepth != 8) || (aDepth != 24) ){

This will always test true, no?

+    NS_ASSERTION(0, "This Depth is not supported\n");

Like I said, use NS_ERROR, please.

Comment 25

[dcone (gone)]

Attachment: Updated patch, fixes error output and the logical check.

I tested this patch.. all works as advertised.

Comment 26

[rods (gone)]

Comment on attachment 101467 [details] [diff] [review]
Updated patch, fixes error output and the logical check.

r=rods

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 101467 [details] [diff] [review]
Updated patch, fixes error output and the logical check.

sr=bzbarsky

Comment 28

[Olivier Cahagne]

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

Comment 29

[Markus Hübner]

Did it fix things so far?

Comment 30

[dcone (gone)]

I am now putting in the support to read in 8 bit images.  An nsImage is capable
of holding 8 bit images but the GIF decoder assumes we want a 24 bit nsImage.  

Comment 31

[Kevin McCluskey (gone)]

Bulk moving P1-P5 un-milestoned bugs to future. 

Comment 32

[dcone (gone)]

Attachment: GFX part of the patch for 8 bit gif. This is version 1.

This patch converts the gfx part for 8 bit gifs.  This is version one.. because
it works.. but is slow.  I am putting it in this but for archival sake.

Comment 33

[dcone (gone)]

Attachment: the libpr0n part of the patch.

This part of the patch seems to be stable for the 8 bit gif.  The slowness
comes from the GFX part.

Comment 34

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

why only use an 8 bit depth on XP_PC?

Comment 35

[dcone (gone)]

Attachment: Part 1 of 8 Bit GIF decoder. Reads in gifs and stores as 8 bit.

Comment 36

[dcone (gone)]

Attachment: Part 2 of patch. Renders and does and ordered dither on image to paletized device

This patch include an error diffusion dither to render the images.  This error
diffusion version of the 8 bit and 24 bit images on a palettized device is
handled in the optimize call.. and the original verions are deleted.. saving on
space.	I did not use the windows dither call.. because its very slow going to
the screen (like 100x slower) and very ugly if you go to an offscreen HDC. 
Rolled my own.. very fast because it is done once at optimize and looks just as
good or better than windows.

Comment 37

[ArronM (:paper)]

looking good so far.  The only thing I've spotted is in nsImageWin::DrawToImage,
you replace dst's colormap with src.  This is will make for horible looking
gifs.  For example, a 2 color gif with 2 frames, first frame's colormap is
(black, red), the smaller second frame's colormap is (red, black).. the second
frame will be inverted.

I'd leave animated gifs out of it for this go around.  They aren't being
optimized anyway.  Switching to optimized animated gifs is going to need a bit
more coding in imgContainerGIF.

Comment 38

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

why only use an 8 bit depth on XP_PC?

Comment 39

[dcone (gone)]

1.)  Size.  There is no need to use 24 bits for an 8 bit gif.
2.)  On a device which has a palette.. 8 bit is all you need, and you can optimize  
     for that depth.  Saves space and is faster.
3.)  If the device is not palette'ized.. then the 24 bit images are not converted, 
     the 8 bit still are.  Dithering will be used on a non-palette'ized  
     devices.

Comment 40

[dcone (gone)]

sorry... Dithering will NOT be used on a non-palette'ized  devices.


As far as the "replacing the dst's colormap with src".  I am assuming the src
colormap is the same as the destinations color map.. which so far seems to be a
good assumption (with it this way, images look great, with it not this way,
images look aweful).. because we copy the src pixel to the destination.. this
pixel is an index into a color table.. if the dest does not have these same
index to color mapping.. then we would have to do a color lookup for each index
to the destinations color... this would be a serious hit for time.  So far so
good on how I have fixed this.. if you have an animation that would test this
scenario.. send it to me and I will make sure I have made the right assumptions.
 So far for my tests, things are working.  The reason for the copy in the first
place.. is to  make sure the dest color map has been put into the proper color
table mode (iusage).

Comment 41

[Boris Zbarsky [:bzbarsky]]

dcone, you misread comment 38.  The "only" goes with "XP_PC" not with "8".  Or
to rephrase "Why not make this great change on all platforms, not just Windows?"
 Which is a valid question, since the bug is marked "All" and there is no
mention of platform-specific anything anywhere in this bug...  If you're only
planning to fix windows, please make that clear and file equivalent bugs for
other platforms, at the very least.

Comment 42

[dcone (gone)]

Opps.. ok on to the real question...
1.)  I think Mac could also use this change.. next platform on my list.
2.)  I heard that GTK had no support for Dithering.. but since I since my first
try rolled my own dither.. GTK could also go this route.. that would be the
third platform on my list.. so we could do all platforms.

The one question I still have to answer for this.. is what about printing. 
Since the printer is a higher resolution device..and we are optimizing for the
screen.. the printouts will suffer unless we cache, reload, something for the
images that go to the printer.  Is a high quality printout needed for an 8 bit
device?  I am sure someone would want it.. can we make this an option.. all this
needs to be discussed.

Comment 43

[dcone (gone)]

Opps.. ok on to the real question...
1.)  I think Mac could also use this change.. next platform on my list.
2.)  I heard that GTK had no support for Dithering.. but since I hit the windows
dither roadblock.. I rolled my own dither.. GTK could also go this route.. that
would be the third platform on my list.. so we could do those platforms for
sure.  Each platform could then follow.

The one question I still have to answer for this.. is what about printing. 
Since the printer is a higher resolution device..and we are optimizing for the
screen.. the printouts will suffer unless we cache, reload, something for the
images that go to the printer.  Is a high quality printout needed for an 8 bit
device?  I am sure someone would want it.. can we make this an option.. all this
needs to be discussed.

Comment 44

[Andrew Hagen]

Just an opinion on printing. It's unlikely that an image will be dynamically
generated, so reloading from the web should be no problem. 

One caveat might be images that are dynamically generated, like on Mapquest. I'm
not sure that would even be an issue, though.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> It's unlikely that an image will be dynamically generated

Wrong.  It's a trivial exercise to find the dozens of bugs we have on issues
related to precisely this assumption... (most of them thankfully fixed by now).

Not to mention the performance penalty of having the network latency in the
middle of the print job, of course.

Comment 46

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

ah yeah and one more, important, thing that I mentioned. XP_PC does _not_ only
apply to windows. It is also used for OS2. Maybe you should use XP_WIN, iirc
that's its name.

Comment 47

[Olivier Cahagne]

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

Comment 48

[ArronM (:paper)]

dcone, why do we store the color map in two places?  From what I can tell, it's
being stored in mColorMap as well as in mBHead.  Is mColorMap needed at all?

Comment 49

[dcone (gone)]

mColormap is a permenent place to store the colormap in a cross platform way. 
The color map in the DIB header is for the DIB.  This can change based on the
type of DIB we are using or optimized for.  For example this color map will be
just indexes  into a color table held by the destination bitmap.  So the
actually colors will be lost.  This is the mode parameter of the windows bliting
operations.

Comment 50

[dcone (gone)]

Attachment: new Modules Patch with prefs to turn on/off dither and 8bit support

Comment 51

[dcone (gone)]

Attachment: New GFX patch uses the prefs for dithering support.

Comment 52

[[e-mail]]

Comment on attachment 106946 [details] [diff] [review]
New GFX patch uses the prefs for dithering support.

CreateDitheredImage and ConvertTo8BitHalftone look remarkably similar :-)

Comment 53

[dcone (gone)]

they are.. but one is for dithering an 8 bit image.. the other for dithering a
24 bit image to an 8 bit image.

Comment 54

[ArronM (:paper)]

With the following in prefs.js:
user_pref("images.gifis8bit", true );
user_pref("images.xpdither", true );

The following things do not work:
- icons on Window menu, tab bar, etc have weird background
- seems to crash randomly
- resource:///res/samples/test2.html (the wheels looked dithered)
- http://animecity.nu/mozilla/IMGTest/images/redblue.gif (Doesn't load.. if it
did, image should not change color)
- http://bugzilla.mozilla.org/attachment.cgi?id=86361&action=view doesn't show
at all (almost makes mozilla.exe not unload when closed too)
- http://animecity.nu/mozilla/IMGTest/AnimatedTest.html Some gifs mess up
- http://bugzilla.mozilla.org/attachment.cgi?id=26957&action=view
- http://bugzilla.mozilla.org/attachment.cgi?id=58510&action=view
- http://bugzilla.mozilla.org/attachment.cgi?id=58511&action=view
- http://bugzilla.mozilla.org/attachment.cgi?id=58512&action=view

There's others, but that's enough to give you an idea that something needs
fixed.  removing the prefs makes everything okay again.

Comment 55

[Olivier Cahagne]

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

Comment 56

[dcone (gone)]

k.. fixed a few problems and crasher.. because some of those gifs were using a
local color table. Question , the large tiled animated GIF .. that draws real
slow.. it seems that each frame has its own color table.. to create the large
image.  Is that true?  If that is true.. should this gif only animate once.. or
does it animate every time it redraws.  In the current code we convert this to
24 bit so it was not an issue, but now we keep the color tables around.. so is
this a case we need to keep track of each local color table when we re-draw.  
Any information on how this type of GIF is drawn would be appreciated. 

Comment 57

[ArronM (:paper)]

Yes, the large tiled animated gif (of fruit) does animate once.  It was an
attempt by someone to form true color image with gif (pretty silly if you ask
me).  However, banner ads (the 468x60 kind) often have local color palettes,
result in more than 256 colors, and loop, loop, loop.

Comment 58

[dcone (gone)]

Attachment: Updated patch, fixes animations and a crasher.

Comment 59

[dcone (gone)]

Attachment: GFX part of patch, this fixed the animation module.

The last two patches fix all the test cases, and there are preferences to turn
on the dither and the 8 bit support.  So when this is checked in with those
preferences turned off.. this code can be tested by QA and anyone else before
its a permenent part of the code.  The last fix I did saves animations as 8
bit, but has a 24 bit frame for the screen.  This is because a GIF can have a
color table for each frame, that is not so bad except if one frame sits ontop
of another (transparency) and switches the color table, bad things can happen. 
Or if frames are drawn in different areas of the target..  then when the entire
target gets drawn only the last color table would be used and .. UGLY PICTURE.

Comment 60

[dcone (gone)]

Attachment: GFX patch.. replaced because of a bad initialization.

Comment 61

[rods (gone)]

Comment on attachment 107477 [details] [diff] [review]
GFX patch.. replaced because of a bad initialization. 

Minor nit:
dError*.187 should be dError * 0.187 (add spaces and put zero in front)

Comment or remove:
+  //if ( mColorTableType == DIB_RGB_COLORS )
+    //return;

Also, I assume this doesn;t have anything to do with printing? (Or at least you
tested print)

Comment 62

[rods (gone)]

Comment on attachment 107477 [details] [diff] [review]
GFX patch.. replaced because of a bad initialization. 

r=rods

Comment 63

[kinmoz]

Comment on attachment 107404 [details] [diff] [review]
Updated patch, fixes animations and a crasher.

==== |depth| was added to  this method, but it's never used?


@@ -133,6 +137,7 @@
   // for animation compositing (GIF) or for filling in with a background
color.
   // XXX IMPORTANT: this means that the frame should be initialized BEFORE
appending to container
   PRUint32 numFrames = inlinedGetNumFrames();
+  PRUint32 depth;


==== Can we initialize during  the declaration?


+    PRUint32 depth;
+    depth = 24;


==== I think further use of |nsIPref| is being dicouraged in favor of
|nsIPrefBranch|. There was a large sweep done to the editor to do the switch
over, so perhaps this should use |nsIPrefBranch| too? Why the need for
declaring the const |k8BitSupport|, instead of just passing the  constant
string directly as an arg?


+#if defined (XP_PC)
+    PRBool   b;
+    const char k8BitSupport[]	      = "images.gifis8bit";
+    nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID);


==== Put a space between the comma and the |&b| arg. Also, use |if (b)| instead
of |if (b == PR_TRUE)| since that also seems to be the prevailing style in the
file.


+    if (NS_SUCCEEDED(prefs->GetBoolPref(k8BitSupport,&b))) { 
+      if ( b == PR_TRUE) {
+	 depth = 8;
+      }
+    }
+#endif  


==== Add a space between the  comma and |tranIndex|, and fix the indentation of
the code you added. Same issues/questions I asked above apply to this block
too:


-  PRUint32 bpr, abpr;
+  PRUint32 bpr, abpr, depth,tranIndex;
+
+    depth = 24;
+
+#if defined (XP_PC)
+    PRBool   b;
+    const char k8BitSupport[]	      = "images.gifis8bit";
+    nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID);
+    if (NS_SUCCEEDED(prefs->GetBoolPref(k8BitSupport,&b))) { 
+      if ( b == PR_TRUE) {
+	 depth = 8;
+      }
+    }
+#endif  
+  
   // We have to delay allocation of the image frame until now because


==== Do we really have to fetch the service and check the pref for every frame
of an animation and for every decoded row? Shouldn't this pref be checked once,
it's value cached and then used in these methods? 


==== Lose the spaces after '(' and before')':


+	 if ( theColorTable ) {

+	   if ( decoder->mGIFStruct->is_transparent ) {


==== Can we add some whitespace here so this is a bit more readable?

+	   for (PRInt32 i=0,cindex=0;i<theColorTable->NumColors;i++,cindex+=3)
{


==== The indentation of this |if| line doesn't match the indentation for it's
|else|, and it's contents looks indented too far. In fact all of the code that
was added to this method uses several different styles of indentation. Can that
be cleaned up?


+      case gfxIFormats::RGB:
+	 {
+	   if (cmap) {// cmap could have null value if the global color table
flag is 0
+		 while (rowBufIndex != decoder->mGIFStruct->rowend) {


==== I know some compilers used to choke on '#' directives that didn't have the
'#' char at the beginning of the line, so just to be safe, can we move the
|#if| and |#endif| to the start of the lines they are on?


+		 #if defined(XP_MAC) || defined(XP_MACOSX)
+		     *rgbRowIndex++ = 0; // Mac is always 32bits per pixel,
this is pad
+		 #endif



==== |tranIndex| wasn't initialized when it was declared, so  there is the
possibility that it could get used here with a bogus value:


-	 memset(decoder->mRGBLine, 0, bpr);
+	 memset(decoder->mRGBLine, tranIndex, bpr);


==== Lose the space after the |true| values, and losethe extra blank line.


+pref("images.gifis8bit", true );
+pref("images.xpdither", true );
+
+

Comment 64

[dcone (gone)]

Attachment: New modules patch with updated prefs, some minor fixes.

Comment 65

[dcone (gone)]

Attachment: New GFX patch with update prefs.

Comment 66

[rods (gone)]

Comment on attachment 108112 [details] [diff] [review]
New modules patch with updated prefs, some minor fixes.

r=rods

Comment 67

[Olivier Cahagne]

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

Comment 68

[Bob Clary [:bc] (inactive)]

Nominating nsbeta1/topembed.

Comment 69

[Kevin McCluskey (gone)]

-> Simon

Comment 70

[Simon Montagu :smontagu]

Comment on attachment 108113 [details] [diff] [review]
New GFX patch with update prefs.

This is almost identical to attachment 107477 [details] [diff] [review], which had r=rods. I am giving
r=smontagu on the changes.

Comment 71

[ArronM (:paper)]

From a structure point of view, I personally don't like how some things in the
patch are done.  Particularly, adding yet another function to gfxImageFrame that
just returns an object from it's mImage.  Having said that, however, I'm no
expert or authority on this code.  I emplore module owners and mozilla
developers who like to keep Mozilla's structure clean to carefully look at these
patches.

At the very least could we at least get the patches to pass "JST Reviewer
Simulacrum" with less warnings? 
http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl

Comment 72

[Simon Fraser [no longer active]]

I agree that the patch could do with some cleaning up.

Comment 73

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

Not blocking 1.3beta.

Comment 74

[Olivier Cahagne]

adding one XHTML-compliant URL with animated background image (worst case) to
test with with, when patch will be in:
http://www.blogblues.com/standards/conversion/blogor/
(perf is horrid on Win2k, 1GHz PIII, 32-bit desktop)

Comment 75

[Simon Montagu :smontagu]

Attachment: Last 2 attachments updated

For reference: attachment 108112 [details] [diff] [review] and attachment 108113 [details] [diff] [review] combined and merged to
tip.

Comment 76

[ArronM (:paper)]

Comment on attachment 108112 [details] [diff] [review]
New modules patch with updated prefs, some minor fixes.

patch obsolete, cleaning up requests queue

Comment 77

[Stuart Parmenter]

Comment on attachment 114943 [details] [diff] [review]
Last 2 attachments updated

first off, the prefs dependancy has got to go.	Either always do it on Windows
if it doesn't hurt speed or don't do it.  Perhaps you could add an API to
nsIImage and gfxIImageFrame to get this information?

Don't include nsIImage in gfxIImageFrame.  Why not just use:
void getColorTable([array, size_is(length)] out PRUint8 index, out unsigned
long length) instead?  Its not like the colormap struct really adds much here.

I'm sure that we don't want to get the prefs service and look up a pref every
time a rendering context is created.. we create them all the time.

How does this effect printing?

Let me know when these issues have been addressed and you've got a final patch
and I'll review it.

Comment 78

[Simon Montagu :smontagu]

Attachment: Modules patch which works with current trunk

I have now got this working with the current trunk (at least as of this
morning, I think the target has moved again since then :-) and corrected a bug
with transparency.

It's still work in progress: I haven't yet addressed stuart's concerns in
comment 77.

Comment 79

[Simon Montagu :smontagu]

Attachment: gfx patch which works with current trunk

Comment 80

[Kevin McCluskey (gone)]

"first off, the prefs dependancy has got to go."

I think it would be good to land it with the pref initially, so we can easily
tell if there are any regressions from landing this large change.  Once we are
confident that there aren't any regressions then we should remove the pref
completely.

Comment 81

[Simon Montagu :smontagu]

Attachment: Fixed biesi's IRC comments (modules/libpr0n part)

Comment 82

[Simon Montagu :smontagu]

Attachment: GFX patch fixing biesi's IRC comments

Comment 83

[Simon Montagu :smontagu]

I am tracking this on the landings page as 8BitGifs. The latest patches work for
me in all the tests I have made. If anyone wants to apply them and do some more
testing I will be grateful. 

Comment 84

[Boris Zbarsky [:bzbarsky]]

We could also land this enabled by default with an env var to disable it for
testing..

Either way, really, as long as we don't ship anything like a release with that
prefs dependency.

Comment 85

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

Comment on attachment 117284 [details] [diff] [review]
GFX patch fixing biesi's IRC comments

+NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap)

should you really return NS_OK if *aColorMap is nsnull?

+  * Convert a 24 bit image to an 8 bit image and do an error diffusion on it,
+  * returns a pointer to the new bits for the image
+  * @update dc - 11/05/2002
+  */
+void
+nsImageWin::ColorMapToDIBMap()

comment and function declaration do not match...

For that matter, the description of this function is wrong, too. The function
just seems to copy the image map from mColorMap to mBHead.
The comment would seem appropriate for DitherImage, which already has a similar
comment. The use of @update in these comments is inconsistent. DitherImage does
not have such a line, ColorMapToDIBMap has it.
It would be nice if the comment before DitherImage would say what it returns.

+#define TOPERRORSPREAD 0
+#define LEFTERRORSPREAD 1
+#define RIGHTERROSPREAD 1
+#define BOTTOMERROSPREAD 1

is there a reason why the first two have ERROR in the name, while the latter
have ERRO?

Comment 86

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

Comment on attachment 117284 [details] [diff] [review]
GFX patch fixing biesi's IRC comments

+NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap)

should you really return NS_OK if *aColorMap is nsnull?

+  * Convert a 24 bit image to an 8 bit image and do an error diffusion on it,
+  * returns a pointer to the new bits for the image
+  * @update dc - 11/05/2002
+  */
+void
+nsImageWin::ColorMapToDIBMap()

comment and function declaration do not match...

For that matter, the description of this function is wrong, too. The function
just seems to copy the image map from mColorMap to mBHead.
The comment would seem appropriate for DitherImage, which already has a similar
comment. The use of @update in these comments is inconsistent. DitherImage does
not have such a line, ColorMapToDIBMap has it.
It would be nice if the comment before DitherImage would say what it returns.

+#define TOPERRORSPREAD 0
+#define LEFTERRORSPREAD 1
+#define RIGHTERROSPREAD 1
+#define BOTTOMERROSPREAD 1

is there a reason why the first two have ERROR in the name, while the latter
have ERRO?

Comment 87

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

Comment on attachment 117284 [details] [diff] [review]
GFX patch fixing biesi's IRC comments

+NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap)

should you really return NS_OK if *aColorMap is nsnull?

+  * Convert a 24 bit image to an 8 bit image and do an error diffusion on it,
+  * returns a pointer to the new bits for the image
+  * @update dc - 11/05/2002
+  */
+void
+nsImageWin::ColorMapToDIBMap()

comment and function declaration do not match...

For that matter, the description of this function is wrong, too. The function
just seems to copy the image map from mColorMap to mBHead.
The comment would seem appropriate for DitherImage, which already has a similar
comment. The use of @update in these comments is inconsistent. DitherImage does
not have such a line, ColorMapToDIBMap has it.
It would be nice if the comment before DitherImage would say what it returns.

+#define TOPERRORSPREAD 0
+#define LEFTERRORSPREAD 1
+#define RIGHTERROSPREAD 1
+#define BOTTOMERROSPREAD 1

is there a reason why the first two have ERROR in the name, while the latter
have ERRO?

Comment 88

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

Comment on attachment 117284 [details] [diff] [review]
GFX patch fixing biesi's IRC comments

+NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap)

should you really return NS_OK if *aColorMap is nsnull?

+  * Convert a 24 bit image to an 8 bit image and do an error diffusion on it,
+  * returns a pointer to the new bits for the image
+  * @update dc - 11/05/2002
+  */
+void
+nsImageWin::ColorMapToDIBMap()

comment and function declaration do not match...

For that matter, the description of this function is wrong, too. The function
just seems to copy the image map from mColorMap to mBHead.
The comment would seem appropriate for DitherImage, which already has a similar
comment. The use of @update in these comments is inconsistent. DitherImage does
not have such a line, ColorMapToDIBMap has it.
It would be nice if the comment before DitherImage would say what it returns.

+#define TOPERRORSPREAD 0
+#define LEFTERRORSPREAD 1
+#define RIGHTERROSPREAD 1
+#define BOTTOMERROSPREAD 1

is there a reason why the first two have ERROR in the name, while the latter
have ERRO?

Comment 89

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

Comment on attachment 117284 [details] [diff] [review]
GFX patch fixing biesi's IRC comments

+NS_IMETHODIMP gfxImageFrame::GetColorTable(nsColorMap **aColorMap)

should you really return NS_OK if *aColorMap is nsnull?

+  * Convert a 24 bit image to an 8 bit image and do an error diffusion on it,
+  * returns a pointer to the new bits for the image
+  * @update dc - 11/05/2002
+  */
+void
+nsImageWin::ColorMapToDIBMap()

comment and function declaration do not match...

For that matter, the description of this function is wrong, too. The function
just seems to copy the image map from mColorMap to mBHead.
The comment would seem appropriate for DitherImage, which already has a similar
comment. The use of @update in these comments is inconsistent. DitherImage does
not have such a line, ColorMapToDIBMap has it.
It would be nice if the comment before DitherImage would say what it returns.

+#define TOPERRORSPREAD 0
+#define LEFTERRORSPREAD 1
+#define RIGHTERROSPREAD 1
+#define BOTTOMERROSPREAD 1

is there a reason why the first two have ERROR in the name, while the latter
have ERRO?

Comment 90

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

gaaaaah. let me kick necko at this point. I clicked the button but nothing
happened. so I clicked it again. ...

sorry for the multiple comments.

Comment 91

[Simon Montagu :smontagu]

Attachment: Addressed comment 77 and comment 85 (modules/libpr0n)

I agree with bz that using an environment variable to disable this at runtime
is the way to go. If we want to assess the impact on performance, the pref is
going to obscure the results.

I think GetColorTable() makes a lot more sense as SetColorTable()...

Comment 92

[Simon Montagu :smontagu]

Attachment: Addressed comment 77 and comment 85 (gfx)

Comment 93

[Simon Montagu :smontagu]

|SetTransparentColor()| no longer exists (bug 197485), but I'm not going to
attach a new patch just to remove two lines.

Comment 94

[Simon Montagu :smontagu]

Attachment: Ready for review (modules/libpr0n)

Fixed a whole lot of stuff after testing on an 256-color display. The dither
actually works now :-)

Comment 95

[Simon Montagu :smontagu]

Attachment: Ready for review (gfx)

Comment 96

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

Comment on attachment 117684 [details] [diff] [review]
Ready for review (modules/libpr0n)

+	 PRUint8* theColorTable = new PRUint8[cmapsize * 3];

would it be possible to not allocate this from heap? Like, can you be sure that
the colormap is not larger than 256*3?
if so, a better (faster) solution might be:
PRUint8 theColorTable[256*3];

+	     transColor |= cmap[decoder->mGIFStruct->tpixel].red;
is this used anywhere?

Comment 97

[Simon Montagu :smontagu]

Yes, we know that the colormap has a maximum of 256 colors, so biesi's
suggestion makes sense. 

All references to |transColor| will be removed.

Comment 98

[Boris Zbarsky [:bzbarsky]]

Also, could we use a _static_ array for the colortable?  It looks like you
always pass the sizes around so the previous data being in it should not be an
issue, should it?

Comment 99

[Roland Mainz]

Boris Zbarsky wrote:
> Also, could we use a _static_ array for the colortable?  It looks like you
> always pass the sizes around so the previous data being in it should not be an
> issue, should it?

The colormap of GIFs and other formats can always be smaller, even monocrome GIF
pictures/animations are possible. Passing the size of the colormap around makes
sense in this case...

Comment 100

[Boris Zbarsky [:bzbarsky]]

Yes, yes. Read my comment again -- the fact that we pass the size may let us get
away with:

  static PRUint8 theColorTable[256*3];

So we only allocate it once.

Comment 101

[Simon Montagu :smontagu]

There is now an experimental build (based on 1.3) available at
ftp://ftp.mozilla.org/pub/mozilla/nightly/experimental/8bitgif/mozilla-i586-pc-msvc-8bitgif.zip

Comment 102

[Stuart Parmenter]

Comment on attachment 117684 [details] [diff] [review]
Ready for review (modules/libpr0n)

+  if (NOT_SETUP == gDisable8BitGIFs) {
Please follow the rest of the file (and all of imagelib) and put
gDisable8BitGIFs first.

I don't really like that NOT_SETUP and gDisable8BitGIFs is in 2 files with the
exact same code.

I like Boris's idea of using a static colortable for 'theColorTable', although
making it static could cause issues if we ever have multiple decoders running
at once.. Maybe put it on the stack?  It isn't there too long.

aside from that, r=pavlov.  Paper needs to r= this patch since he's going to
have to live with the code.

Comment 103

[Stuart Parmenter]

Comment on attachment 117685 [details] [diff] [review]
Ready for review (gfx)

+  [noscript] void setColorTable([array, size_is(length), const] in PRUint8
index, in unsigned long length);

there is no reason for this to be noscript.  Please move the method in the idl
file after unlockAlphaData.  Some comments explaining the API need to be added.

+NS_IMETHODIMP gfxImageFrame::SetColorTable(const PRUint8 *aIndex, PRUint32
aNumColors)
Please move this function to under UnlockAlphaData as well.  There isn't any
reason to declare |nsColorMap* colorMap| outside of the |if (mImage) {|

+    dstNumBytes = dstNumBits/8;
+    srcNumBytes = srcNumBits/8;
Change those to shifts for dumb compilers?

+static PRBool gDisableDither = NOT_SETUP;
Again, I don't really like having this in now 4 files...

+  // Environment variable to disable the dither
is this really needed twice?  Why can't it go in the constructor?

fix these problems and r=

Comment 104

[Simon Montagu :smontagu]

Attachment: Addressed stuart's comments (modules/libpr0n)

>I don't really like that NOT_SETUP and gDisable8BitGIFs is in 2 files with the

>exact same code.

The only way I can see to avoid this is to promote mDepth from imgContainerGif
up to imgIContainer and adding an aDepth argument to its Init(). Would you be
happy with that? It would have the virtue of consistency with the recent
additions to gfxIImageFrame.

Comment 105

[Simon Montagu :smontagu]

Attachment: Addressed stuart's comments (gfx)

I removed the duplicate use of the environment variable within
nsRenderingContextWin.cpp, but again I don't see a way to remove the
duplication completely.

Comment 106

[Simon Montagu :smontagu]

Comment on attachment 118374 [details] [diff] [review]
Addressed stuart's comments (gfx)

Transferring r=pavlov and requesting sr

Comment 107

[Stuart Parmenter]

Adding depth to imgIContainer makes no sense.  what would that even mean?  There
is no data directly associated with it.

Comment 108

[tor]

Comment on attachment 118373 [details] [diff] [review]
Addressed stuart's comments (modules/libpr0n)

* mDepth in nsGIFDecoder2 and imgContainerGIF should be removed - just
    use the global gDisable8BitGIFs (perhaps renamed to something that
    flows a bit nicer)

* instead of copying the colormap in HaveDecodedRow (which looks
    suspiciously like it's asking for problems if image decoding ever
    goes multithreaded), why not make the relatively minor changes
    needed in nsGIFDecoder2 so the decode stores in a form that can
    just be passed to gfx?

* Paper - for the transparent color, can we just set the decoder's
    colormap entry to black, or will that cause problems with
    animated gifs?

* 8-bit, case RGB: why copy the indices (which could be done with
    memcpy instead of by hand) instead of passing aRowBufPtr to
    SetImageData?

* 8-bit, case RGB_A1: see above the image portion of the data.
    shouldn't the transparent test in the for loop be against
    tranIndex (which should probably be made a PRInt32 and set
    to -1 initially)?

Comment 109

[tor]

Comment on attachment 118373 [details] [diff] [review]
Addressed stuart's comments (modules/libpr0n)

>Index: mozilla/modules/libpr0n/decoders/gif/imgContainerGIF.cpp
>@@ -582,9 +601,7 @@
>     //     could optimize further:
>     //     if aPrevFrameIndex == 1 && mLastCompositedFrameIndex <> -1,
>     //     only mFirstFrameRefreshArea needs to be drawn back to composite
>-    if (isFullPrevFrame) {
>-      CopyFrameImage(aPrevFrame, mCompositingFrame);
>-    } else {
>+    if (!isFullPrevFrame ||  (!(CopyFrameImage(aPrevFrame,  mCompositingFrame)))) {
>       BlackenFrame(mCompositingFrame);
>       SetMaskVisibility(mCompositingFrame, PR_FALSE);
>       aPrevFrame->DrawTo(mCompositingFrame, prevFrameRect.x, prevFrameRect.y,

Was this change intentional, or a missed merge somewhere along the line?

Comment 110

[Simon Montagu :smontagu]

It's intentional. CopyFrameImage() can fail if the frames have different color
depths, and if that happens I need to fall back to DrawTo().

Comment 111

[Simon Montagu :smontagu]

> * mDepth in nsGIFDecoder2 and imgContainerGIF should be removed - just
>     use the global gDisable8BitGIFs (perhaps renamed to something that
>     flows a bit nicer)

Fixed

> * instead of copying the colormap in HaveDecodedRow (which looks
>     suspiciously like it's asking for problems if image decoding ever
>     goes multithreaded), why not make the relatively minor changes
>     needed in nsGIFDecoder2 so the decode stores in a form that can
>     just be passed to gfx?

Fixed

> * Paper - for the transparent color, can we just set the decoder's
>     colormap entry to black, or will that cause problems with
>     animated gifs?

I have tested with a lot of animated gifs (the links in comment 54 and others)
and don't see any problems

> * 8-bit, case RGB: why copy the indices (which could be done with
>     memcpy instead of by hand) instead of passing aRowBufPtr to
>     SetImageData?

> * 8-bit, case RGB_A1: see above the image portion of the data.

You lost me here. Won't that lose the transparent color information?

>     shouldn't the transparent test in the for loop be against
>     tranIndex (which should probably be made a PRInt32 and set
>     to -1 initially)?

Fixed

Comment 112

[Simon Montagu :smontagu]

Attachment: Addressed tor's comments (modules/libpr0n)

Comment 113

[tor]

Comment on attachment 118505 [details] [diff] [review]
Addressed tor's comments (modules/libpr0n)

* GIF2.cpp - case gif_global_colormap/gif_image_colormap: memory
    allocation can be a malloc instead of calloc.  Copying of
    colormap can be done with memcpy.

* HaveDecodedRow - was thinking that if modifying the colormap
    entry for the transparent color is a valid optimization (Paper?)
    it could be done in the gif decoder (case gif_control_extension).
    Minor nit: the comment above this section of code should
    probably be "set the..." instead of "copy the..." now.

* HaveDecodedRow, 8-bit, case _A1 - the image portion can have
    the same optimization of eliminating the needless copy that
    you did for the RGB case (passing aRowBufPtr directly to
    SetImageData).

Comment 114

[Simon Montagu :smontagu]

Attachment: Addressed the latest round of comments (modules/libpr0n)

By the way, I'll make the same change in the gfx part of using the global
variables directly and not adding new members. I won't attach a new patch for
now.

Comment 115

[Simon Montagu :smontagu]

After making the change to pass aRowBufPtr to SetImageData, I'm left with
something rather kludgy in the case where there is no color map:


+          if (cmap) { // cmap could have null value if the global color table 
+                      // flag is 0
+            for (int i = 0; i < aDuplicateCount; ++i) {
+              decoder->mImageFrame->SetImageData(aRowBufPtr, bpr, 
+                                                 (aRowNumber+i)*bpr);
+            }
+          } else {
+            memset(decoder->mRGBLine, 0, bpr);
+            for (int i = 0; i < aDuplicateCount; ++i) {
+              decoder->mImageFrame->SetImageData(decoder->mRGBLine,
+                                                 bpr, (aRowNumber+i)*bpr);
+            }
+          }

What about just doing 

  decoder->mImageFrame->SetImageData(nsnull, bpr, (aRowNumber+i)*bpr);

(without the memset) and then doing
  if (aData)
    memcpy(imgData + newOffset, aData, aLength);
  else
    memset(imgData + newOffset, 0, aLength);
in gfxImageFrame::SetImageData() ?

Comment 116

[Simon Montagu :smontagu]

Attachment: Patch addressing comments on IRC today (gfx)

Comment 117

[Simon Montagu :smontagu]

Attachment: Patch addressing comments on IRC today (modules/libpr0n)

Comment 118

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

Comment on attachment 118824 [details] [diff] [review]
Patch addressing comments on IRC today (modules/libpr0n)

so I can't say I ever liked this construct:
+#define NOT_SETUP 0x33
+static PRBool gEnable8BitGIFs = NOT_SETUP;

Would it be possible to do the real intialization in imglib_Initialize (in
build/nsImageModule.cpp)?

Comment 119

[Simon Montagu :smontagu]

Attachment: A new approach (gfx part)

Performance testing by me and Barrett has shown that decoding to 8 bits is good
for performance when the display is 8-bit, and bad for performance when it is
24-bit; and enabling dithering is a huge performance win on a palettized
device.

Taking all that into account, I am removing the environment variables
altogether and just enabling the features depending on the display
capabilities.

Comment 120

[Simon Montagu :smontagu]

Attachment: A new approach (modules/libpr0n part)

Comment 121

[Simon Montagu :smontagu]

In the spirit of "divide et impera", I've moved the style, whitespace, and
infrastructure changes out into separate bugs, bug 201568 and bug 201576, which
I hope I can get quickly reviewed and checked in.

Comment 122

[Olivier Cahagne]

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

Comment 123

[Olivier Cahagne]

did patch for bug 216430 help here (I'm on Linux right now) ?

Comment 124

[Matthias Versen [:Matti]]

Comment on attachment 118373 [details] [diff] [review]
Addressed stuart's comments (modules/libpr0n)

removing obsolete review request

Comment 125

[Olivier Cahagne]

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

Comment 126

[Alfred Kayser]

Any progress on this one?

Comment 127

[Markus Hübner]

[e-mail], only the assigned person should change the target milestone!

Comment 128

[Simon Montagu :smontagu]

Mass reassign my image bugs to [e-mail]

Comment 129

[Markus Hübner]

Anyone willing to take this as this is topembed+ and P2?

Comment 130

[Alfred Kayser]

What about Cairo? It doesn't support 8bit images, which makes this one useless.
Options: 
1. extend Cairo with 8bit images.
2. in the GifContainer keep the 8bit data, and only generate a Gfx image when the actual frame needs to be displayed.
Both are difficult and have huge impact...

Comment 131

[Ryan VanderMeulen [:RyanVM]]

Stuart or vlad, is this still a reasonable bug given comment #130? If someone were to hack in 8bit support for Cairo, would this still result in a good performance win for gifs?

Comment 132

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

(In reply to comment #131)
> Stuart or vlad, is this still a reasonable bug given comment #130? If someone
> were to hack in 8bit support for Cairo, would this still result in a good
> performance win for gifs?

I doubt it; adding 8-bit support to cairo would be a pretty huge task and of very limited value.  The only savings that I can think of is the memory savings of storing the images as 8-bit + palette instead of as ARGB in memory, but I think drawing them would actually be slower.  I would suggest WONTFIX here, though imagelib and our image rendering story overall needs an overhaul.  Stuart and I have talked about some various ideas, but nothing concrete yet.

Comment 133

[Stuart Parmenter]

don't think we're going to do this.

Comment 134

[Alfred Kayser]

Attachment: V1: Use 8bit storage for anim frames

Proof of concept patch to keep the frames, except the first one, at 8bit depth.
By keeping the first frame in current 24bit format, this wont impact performance of still gif images.

This patch does:
* extends gfxIImageFrame to also store 8bit images, including 'DrawTo' to a real 24bit frame. Note, these 8bit frames are not valid elsewhere. Note2: 8 bit frames don't create a 'nsIImage' object, they just store image data.
* modified imgContainerGif to do the animation by composing every animated frame keeping the current frame in mCompositingFrame, and then clearing according the mDisposalMethod and the drawing the next frame (using the above DrawTo) on top.
This makes actually the frame composition easier then before...
* Only 2 or 3 real frames with images are created at most (the first frame, the current compositing frame and sometimes the previous comp. frame (for RESTORE_PREVIOUS).

Savings:
for a large image (like 'galaxy_anim_short.mov2.gif'), consists of 503 frames of 681x409 pixels. With 24bit storage this results in more than 420MB allocated memory. Using 8bit, this is then just 140MB. So, this saves about 300MB memory usage... Also because only 2 or 3 real images are created instead of 503 (!) this saves a lot of load on the OS image handling.
The image above loads in my version within 2 seconds, in current FF builds this takes more than 5 seconds, and a lot of swapping.

Performance impact during runtime seems also to be less, at least about the same.

Comment 135

[Alfred Kayser]

Attachment: V2: Update to current trunk with Cairo

This patch is completely working. I have tested with a large set of animated gifs, includes the ones that I could find attached to open&close bugs about gif decoding.

Testing also confirmed the reduced memory usage. Like 140MB for the galaxy animation instead of 420MB. Also the cpu load seems to be half of what it was before. 

The total result is that these large animations don't freeze the browser that easily, and even also loads the animation even faster and starts the animation almost directly (instead of first swamping memory).

Comment 136

[Stuart Parmenter]

dveditz is almost certainly the wrong person to review this.  the reason I said we weren't going to do it was because we're about to change all the image interfaces and we're about to land APNG (any day now) which redoes a bunch of the gif imgIContainer stuff (gets rid of imgContainerGIF and merges it all back in to imgContainer (yes, this undoes a previous patch)).

I appreciate the work and the memory savings here but I really don't want to complicate the image code any more until we've redone it all.  Can we hold on this patch for a while until we get some more of the image apis cleaned up?

Comment 137

[Alfred Kayser]

Sure, no problem. I will await the refactoring.
Removing the imgContainerGif is a good idea, and when we can fully switch to Cairo there is more tuning/optimisation to be had.

Comment 138

[Daniel Veditz [:dveditz]]

Comment on attachment 247469 [details] [diff] [review]
V2: Update to current trunk with Cairo

I might be OK for a sr= here if needed, but you need a gfx module owner/peer for review (stuart, vlad, roc)

It's not always entirely up-to-date, but when looking for owner/reviewers start at http://www.mozilla.org/owners.html

Comment 139

[Ryan VanderMeulen [:RyanVM]]

Alfred, any chance of revisiting this bug now that APNG has landed?

Comment 140

[Alfred Kayser]

Pavlov is still planning to revamp the image,frame,container structure, and has asked me to wait for that.

Comment 141

[Ryan VanderMeulen [:RyanVM]]

Stuart - are you planning on doing those changes still for 1.9? If not, would it be possible for Alfred to go ahead with this patch? It would be a nice win.

Comment 143

[Alfred Kayser]

I am working on a new patch (where animation frames for GIF's are kept as 8bit in gfxImageFrame), but bug 367281, bug 366465, and bug 386268 need to be completed to make a good and clean patch.

Comment 144

[Alfred Kayser]

Also bug 216682 needs to be SR'ed and committed before I can make the patch for this bug.

Comment 145

[Alfred Kayser]

Once bug 367281 is checked in, I can generate a patch to keep animation frames of GIF's stored as 8 bit.

Comment 146

[Alfred Kayser]

Attachment: V3: Updated again, smaller, and using GetPaletteData for nicer interface between gfxImageFrame and imgContainer

Note, long description of the changes will follow soon.
Meanwhile people could try this out if it works in their tree.

Comment 147

[Alfred Kayser]

Attachment: Very minor bitrot removed, so that it cleanly applies to trunk

Summary of approach:
Let gfxImageFrame store 8bit frames for animation frames, as palette (with 2<<depth entries) and image buffer with a byte for each pixel.
Let imgContainer draw 8bit frames into the composite frame during animation.
Let nsGIFDecoder2 decode to 8bit frames for the animation frames and 32bit for the first frame.

Annotation to the diff:
* gfxIFormats.idl:
    add 'PAL' and 'PAL_A1' formats

* gfxIImageFrame.idl: 
    add 'GetPaletteData' to get pointer to palette data (like GetImageData)

* gfxImageFrame.cpp: 
    Add support for PAL and PAL_A1 formats. 
    For PAL/PAL_A1 no 'nsImage' is created, only the memory buffer for the palette and the 8bit pixels.
    Make all getters/setters behave for PAL images.

* gfxImageFrame.h:
    Define helpers to calc. palette and imagedata sizes

* imgContainer.cpp:
    Make DoComposite handle 8bit frames, but sort of always draw into the composite frame. Note still some optimizations are applied to this.
    The ImageUpdated notification moved to 'Clear/DrawFrames' into DoComposite, so that is only performed once.
    Extend DrawFrameTo to also handle 8bit animation frames.
    So far, all these changes are compatible with the old way of doing animation (and thus also for APNG's).

* GIF2.h:
    Make pointers to image 8bit based for easier calculation into 8bit frames.

* nsGIFDecoder2.cpp:
    BeginImageFrame: take aDepth parameter to correctly select right type of frame to create.
    FlushImageData is only needed for first frame (other frames are not flushed, as they are only handled when completed).
    EndImageFrame: Haeberli-inspired hack only for first frame.
        For first frame, convert from 8bit to 32bit cairo here.
    DoLzw: instead direct colormapping store original 8bit value only.
    ConvertColormap: as palette is now rightly sized to the depth, no need to clear the remainder.

Comment 148

[Ryan VanderMeulen [:RyanVM]]

OK, I've been playing around with Alfred's patch. I haven't noticed any rendering issues for animated GIFs yet with it. Also, the memory savings are dramatic.

For example, the image below used to cause Firefox' memory consumption to increase by nearly 24MB upon being loaded. With the patch applied, it only goes up by a bit over 9MB.
http://thermoanalytics.com/applications/mom/aircraft_cabin_temperature/images/overview_animation_640.gif

Comment 149

[Stuart Parmenter]

This patch doesn't appear to handle the case of someone doing do_GetInterface() on a gfxIImageFrame to get the nsIImage -- various places in our code do this including the clipboard I believe.  It looks like the code will just crash with this patch.  If we care about the interface that we've been providing here, then you're going to need to return an object...

Comment 150

[Stuart Parmenter]

Alfred?  Any ideas on my comment?

Comment 151

[Alfred Kayser]

All callers/users of the imgContainer::GetFrameAt, only get the first frame like:
   aCursor->GetFrameAt(0, getter_AddRefs(frame));
Or they get the current frame:
   container->GetCurrentFrame(getter_AddRefs(gfxFrame));

With this patch, both will still result in a gfxIImageFrame, with a valid nsIImage.

Several options to prevent misuse (getting a frame without an nsIImage):
1. Change GetFrameAt into GetFirstFrame, so that imgContainer only has GetFirstFrame, GetCurrentFrame. Both guaranteed to have a valid nsIImage.
2. Make the do_GetInterface() return NS_ERROR or such for gfxImageFrame without an nsIImage.
3. Make the do_GetInterface() create an nsIImage on the fly.

Ad 1. The interface won't allow anymore to get the image of each frame. But the validity of these nsImage is questionable has it is sometimes a composed image and sometimes a original image. When an user of gfxImageFrame really wants to have the source data, getImageData can still supply this.
Note, For the layout code, we only need GetFirstFrame and GetCurrentFrame.
This means that the imgIContainer.idl needs to change.

Ad 2. Callers may still have problems on the failing do_GetInterface. And while the idl doesn't change, the semantics has changed anyway: do_GetInterface on (GetFrameAt(n!=0, ...) will return NS_ERROR...

Ad 3. Costs extra memory, can cause leaks, etc...
Constructing an nsImage on the fly, will be difficult (and invalid), as the animation composition code can only do the 'current frame'.

So, the cleanest way is option 1, and is the most easy way, and the most safe way. There are only 5 callers of GetFrameAt: s/GetFrameAt(0, /GetFirstFrame(/ would suffice.
/widget/src/windows/nsWindow.cpp, line 2662 -- aCursor->GetFrameAt(0, getter_AddRefs(frame));
/widget/src/os2/nsWindow.cpp, line 1858 -- aCursor->GetFrameAt(0, getter_AddRefs(frame));
/widget/src/gtk2/nsWindow.cpp, line 1032 -- aCursor->GetFrameAt(0, getter_AddRefs(frame));
/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp, line 456 -- mImage->GetFrameAt(0, getter_AddRefs(mFrame));
/content/base/src/nsContentUtils.cpp, line 2203 -- imgContainer->GetFrameAt(0, getter_AddRefs(imgFrame));

Note, making the imgContainer.idl only have GetFirstFrame and GetCurrentFrame would allow to also to for animations that only loop once just to keep the first frame and the current frame, and destroy the frames in between sooner.


Note that the patch is currently bitrotted anyway, and needs updating anyway, so I will first attempt to update the patch, and test this approach.

Comment 152

[Alfred Kayser]

Attachment: Unbitrotted again. No other changes.

Comment 153

[Alfred Kayser]

As for comment 150 and comment 151. The patch already protects for do_GetInterface on a gfxImageFrame without a real nsIImage, by returning a NS_ERROR value. And currently all callers for GetFrameAt only ask for the first frame (0), which has always a valid nsIImage. And GetCurrentFrame always returns a frame with a valid nsIImage. So current code won't break.
I would propose to replace GetFrameAt with GetFirstFrame in a separate bug.

As for some statistics, for the image: http://vectormagic.stanford.edu/comparisons/rotational_invariance.gif, about:cache reports:
Before:  Data size: 368050176 bytes
After:   Data size: 108527616 bytes
So, for this image we save thus 260 MB.

Comment 154

[Ryan VanderMeulen [:RyanVM]]

Alfred, this patch seems to have broken animated GIFs pretty badly. The throbber has a "flashing" going on where frames after the first one seem to be washed out. Also, the image in comment #148 seems to be completely missing the Red channel.

Comment 155

[Ryan VanderMeulen [:RyanVM]]

I'm also getting a LOT of random crashiness with v8 that I wasn't getting with v7.

Comment 156

[Stuart Parmenter]

I haven't tested this patch yet, but it looks like some parts might break apngs a bit

Comment 157

[Alfred Kayser]

Attachment: V9: Better initialisation in gfxImageFrame to prevent crashes

Note, this patch works with the image of comment #148, as well as my test of APNG's. For APNG images imgContainer follows the original logic of frame composition, as only frames of type PAL do get special treatment.
Note2, with this patch, GIF animation creates at most 3 images in the X server space.

Comment 158

[tor]

Comment on attachment 287392 [details] [diff] [review]
V9: Better initialisation in gfxImageFrame to prevent crashes

> PRUint32 nsGIFDecoder2::OutputRow()
> {
...
>+    // Row to process
>+    const PRUint32 bpr = sizeof(PRUint32) * mGIFStruct.width; 
>+	PRUint8 *rowp = mImageData + (mGIFStruct.irow * bpr);

Nit - fix spacing.

Comment 159

[Boris Zbarsky [:bzbarsky]]

Did the issue from comment 149 ever get addressed?

Comment 160

[Stuart Parmenter]

mostly -- we're doing a followup patch to fully address them

Comment 161

[Reed Loden [:reed]]

Landed by stuart at 2007-11-07 13:33.

mozilla/gfx/idl/gfxIFormats.idl 	1.5
mozilla/gfx/idl/gfxIImageFrame.idl 	1.19
mozilla/gfx/src/shared/gfxImageFrame.cpp 	1.44
mozilla/gfx/src/shared/gfxImageFrame.h 	1.14
mozilla/modules/libpr0n/src/imgContainer.cpp 	1.60
mozilla/modules/libpr0n/decoders/gif/GIF2.h 	1.27
mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp 	1.86
mozilla/modules/libpr0n/decoders/gif/nsGIFDecoder2.h 	1.31

Comment 162

[Alfred Kayser]

Verified using build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007110805 Minefield/3.0b2pre.
According to the about:cache, the rotational_invariance.gif is now using 108527616 bytes instead of 368050176 bytes, saving 260MB.

Thanks for the reviews, checkin and support!

Comment 163

[Jeff Walden [:Waldo]]

We should get some testcases in for the situations where the intermediate patches had bugs in them, as such testcases would have more easily revealed some of the ways those patches were broken.

Comment 164

[[e-mail]]

Comment on attachment 287392 [details] [diff] [review]
V9: Better initialisation in gfxImageFrame to prevent crashes

> /* void getImageData([array, size_is(length)] out PRUint8 bits, out unsigned long length); */
> NS_IMETHODIMP gfxImageFrame::GetImageData(PRUint8 **aData, PRUint32 *length)
> {
>   if (!mInitialized)
>     return NS_ERROR_NOT_INITIALIZED;
> 
>+  NS_ASSERTION(mMutable, "trying to get data on an immutable frame");
What's the point of this assertion?

Comment 165

[Worcester12345]

(In reply to comment #163)
> We should get some testcases in for the situations where the intermediate
> patches had bugs in them, as such testcases would have more easily revealed
> some of the ways those patches were broken.

(In reply to comment #164)
> Comment on attachment 287392 [details] [diff] [review]
> V9: Better initialisation in gfxImageFrame to prevent crashes
> 
> > /* void getImageData([array, size_is(length)] out PRUint8 bits, out unsigned long length); */
> > NS_IMETHODIMP gfxImageFrame::GetImageData(PRUint8 **aData, PRUint32 *length)
> > {
> >   if (!mInitialized)
> >     return NS_ERROR_NOT_INITIALIZED;
> > 
> >+  NS_ASSERTION(mMutable, "trying to get data on an immutable frame");
> What's the point of this assertion?

Ever get these answered?