Bug 143046 (slowGIF)

Need to Keep GIFs at original 8 bit or optimized.

VERIFIED FIXED in mozilla1.9beta2

Status

Core Graveyard
GFX
P2
major
VERIFIED FIXED
15 years ago
7 years ago

People

(Reporter: dcone (gone), Assigned: Alfred Kayser)

Tracking

({footprint, perf, topembed+})

Trunk
mozilla1.9beta2
footprint, perf, topembed+
Dependency tree / graph
Bug Flags:
blocking1.3b -
blocking1.4a -
blocking1.8a1 -
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 38 obsolete attachments)

53.19 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Taking this
Assignee: kmcclusk → dcone
(Reporter)

Comment 2

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

Updated

15 years ago
Keywords: footprint, perf

Comment 3

15 years ago
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

Updated

15 years ago
Blocks: 86319

Comment 4

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

Updated

15 years ago
Keywords: mozilla1.0

Updated

15 years ago
Blocks: 46942

Comment 5

15 years ago
Is this a Platform/OS All/all bug?
Blocks: 119597
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

15 years ago
Increasing priority as this one is blocking important bugs.
Severity: normal → major
OS: Windows NT → All
Hardware: PC → All
(Reporter)

Comment 8

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

Updated

15 years ago
Blocks: 147799

Updated

15 years ago
Blocks: 71668

Updated

15 years ago
Keywords: mozilla1.1

Updated

15 years ago
Keywords: mozilla1.0 → mozilla1.0.1

Updated

15 years ago
Priority: -- → P2

Comment 9

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

Comment 10

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

Updated

15 years ago
Alias: slowGIF

Comment 11

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

Comment 12

15 years ago
*** Bug 125892 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 13

15 years ago
Created attachment 101403 [details] [diff] [review]
Update some interfaces.

Comment 14

15 years ago
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 :-)
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.
Keywords: mozilla1.0.1, mozilla1.1 → mozilla1.0.2, mozilla1.2
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

15 years ago
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.
(Reporter)

Comment 18

15 years ago
Created attachment 101420 [details] [diff] [review]
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.
Attachment #101403 - Attachment is obsolete: true
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)?  
(Reporter)

Comment 20

15 years ago
Created attachment 101427 [details] [diff] [review]
Patch with fixed tabs, spaces and check for right depth.
+    NS_ASSERTION(0, "This Depth is not supported\n");

NS_ERROR("This ...").

+  gfx_depth depth = aWidth;

aDepth, you mean?

sr=me with those changes...
(Reporter)

Comment 22

15 years ago
Created attachment 101436 [details] [diff] [review]
Fix for aDepth...
Attachment #101420 - Attachment is obsolete: true
Attachment #101427 - Attachment is obsolete: true

Comment 23

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

r=rods
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.

(Reporter)

Comment 25

15 years ago
Created attachment 101467 [details] [diff] [review]
Updated patch, fixes error output and the logical check.

I tested this patch.. all works as advertised.
Attachment #101436 - Attachment is obsolete: true

Comment 26

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

r=rods
Attachment #101467 - Flags: review+
Comment on attachment 101467 [details] [diff] [review]
Updated patch, fixes error output and the logical check.

sr=bzbarsky
Attachment #101467 - Flags: superreview+

Comment 28

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

Comment 29

15 years ago
Did it fix things so far?
(Reporter)

Comment 30

15 years ago
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.  
Bulk moving P1-P5 un-milestoned bugs to future. 
Target Milestone: --- → Future
(Reporter)

Updated

15 years ago
Target Milestone: Future → mozilla1.2beta
(Reporter)

Comment 32

15 years ago
Created attachment 104409 [details] [diff] [review]
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.
(Reporter)

Comment 33

15 years ago
Created attachment 104411 [details] [diff] [review]
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.
why only use an 8 bit depth on XP_PC?
(Reporter)

Comment 35

15 years ago
Created attachment 105890 [details] [diff] [review]
Part 1 of  8 Bit GIF decoder.  Reads in gifs and stores as 8 bit.
Attachment #104409 - Attachment is obsolete: true
Attachment #104411 - Attachment is obsolete: true
(Reporter)

Comment 36

15 years ago
Created attachment 105892 [details] [diff] [review]
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

15 years ago
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.
why only use an 8 bit depth on XP_PC?
(Reporter)

Comment 39

15 years ago
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.
(Reporter)

Comment 40

15 years ago
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).

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

Comment 42

15 years ago
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.
(Reporter)

Comment 43

15 years ago
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

15 years ago
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.
> 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.
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

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

Comment 48

15 years ago
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?

Updated

15 years ago
Keywords: mozilla1.2 → mozilla1.3
(Reporter)

Comment 49

15 years ago
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.
(Reporter)

Comment 50

15 years ago
Created attachment 106945 [details] [diff] [review]
new Modules Patch with prefs to turn on/off dither and 8bit support
(Reporter)

Updated

15 years ago
Attachment #105890 - Attachment is obsolete: true
(Reporter)

Comment 51

15 years ago
Created attachment 106946 [details] [diff] [review]
New GFX patch uses the prefs for dithering support.
Attachment #105892 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #106945 - Flags: superreview?(kin)
Attachment #106945 - Flags: review?(rods)
(Reporter)

Updated

15 years ago
Attachment #106946 - Flags: superreview?(kin)
Attachment #106946 - Flags: review?(rods)

Comment 52

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

CreateDitheredImage and ConvertTo8BitHalftone look remarkably similar :-)
(Reporter)

Comment 53

15 years ago
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

15 years ago
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

15 years ago
*** Bug 181419 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 56

15 years ago
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

15 years ago
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.
(Reporter)

Comment 58

15 years ago
Created attachment 107404 [details] [diff] [review]
Updated patch, fixes animations and a crasher.
Attachment #106945 - Attachment is obsolete: true
(Reporter)

Comment 59

15 years ago
Created attachment 107405 [details] [diff] [review]
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.
Attachment #106946 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #107404 - Flags: superreview?(kin)
Attachment #107404 - Flags: review?(rods)
(Reporter)

Updated

15 years ago
Attachment #107405 - Flags: superreview?(kin)
Attachment #107405 - Flags: review?(rods)
(Reporter)

Comment 60

15 years ago
Created attachment 107477 [details] [diff] [review]
GFX patch.. replaced because of a bad initialization.
Attachment #107405 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #107477 - Flags: superreview?(kin)
Attachment #107477 - Flags: review?(rods)

Comment 61

15 years ago
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

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

r=rods
Attachment #107477 - Flags: review?(rods) → review+

Updated

15 years ago
Attachment #106945 - Flags: superreview?(kin)
Attachment #106945 - Flags: review?(rods)

Updated

15 years ago
Attachment #106946 - Flags: superreview?(kin)
Attachment #106946 - Flags: review?(rods)

Updated

15 years ago
Attachment #107405 - Flags: superreview?(kin)
Attachment #107405 - Flags: review?(rods)

Comment 63

15 years ago
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 );
+
+
Attachment #107404 - Flags: superreview?(kin) → superreview-
(Reporter)

Comment 64

15 years ago
Created attachment 108112 [details] [diff] [review]
New modules patch with updated prefs, some minor fixes.
Attachment #107404 - Attachment is obsolete: true
(Reporter)

Comment 65

15 years ago
Created attachment 108113 [details] [diff] [review]
New GFX patch with update prefs.
Attachment #107477 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #107404 - Flags: superreview-
Attachment #107404 - Flags: review?(rods)
(Reporter)

Updated

15 years ago
Attachment #107477 - Flags: superreview?(kin)
Attachment #107477 - Flags: review+
(Reporter)

Updated

15 years ago
Attachment #108112 - Flags: superreview?(kin)
Attachment #108112 - Flags: review?(rods)
(Reporter)

Updated

15 years ago
Attachment #108113 - Flags: superreview?(kin)
Attachment #108113 - Flags: review?(rods)

Comment 66

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

r=rods
Attachment #108112 - Flags: review?(rods) → review+

Comment 67

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

Updated

15 years ago
Flags: blocking1.3b?
Whiteboard: [has review]

Comment 68

15 years ago
Nominating nsbeta1/topembed.
Keywords: nsbeta1, topembed

Updated

15 years ago
Summary: Need to Keep GIF's at original 8 bit or optimized. → Need to Keep GIFs at original 8 bit or optimized.

Updated

15 years ago
Keywords: topembed → topembed+
-> Simon
Assignee: dcone → smontagu
Keywords: nsbeta1 → nsbeta1+
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.
Attachment #108113 - Flags: review?(rods) → review+

Updated

15 years ago
Target Milestone: mozilla1.2beta → mozilla1.3beta

Comment 71

15 years ago
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

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

Updated

15 years ago
Blocks: 51028

Updated

15 years ago
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Not blocking 1.3beta.
Flags: blocking1.3b? → blocking1.3b-

Comment 74

14 years ago
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)

Updated

14 years ago
Keywords: mozilla1.0.2
Created attachment 114943 [details] [diff] [review]
Last 2 attachments updated

For reference: attachment 108112 [details] [diff] [review] and attachment 108113 [details] [diff] [review] combined and merged to
tip.
Attachment #108112 - Attachment is obsolete: true
Attachment #108113 - Attachment is obsolete: true

Comment 76

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

patch obsolete, cleaning up requests queue
Attachment #108112 - Flags: superreview?(kin)

Updated

14 years ago
Attachment #108113 - Flags: superreview?(kin)

Comment 77

14 years ago
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.

Updated

14 years ago
Flags: blocking1.4a?

Updated

14 years ago
Blocks: 105370

Updated

14 years ago
Whiteboard: [has review]
Created attachment 117269 [details] [diff] [review]
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.
Attachment #101467 - Attachment is obsolete: true
Attachment #114943 - Attachment is obsolete: true
Created attachment 117270 [details] [diff] [review]
gfx patch which works with current trunk
"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.
Created attachment 117281 [details] [diff] [review]
Fixed biesi's IRC comments (modules/libpr0n part)
Attachment #117269 - Attachment is obsolete: true
Attachment #117281 - Attachment description: Fixed biesi's IRC comments → Fixed biesi's IRC comments (modules/libpr0n part)
Created attachment 117284 [details] [diff] [review]
GFX patch fixing biesi's IRC comments
Attachment #117270 - Attachment is obsolete: true
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. 
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 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 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 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 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 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?
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.
Created attachment 117528 [details] [diff] [review]
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()...
Attachment #117281 - Attachment is obsolete: true
Created attachment 117529 [details] [diff] [review]
Addressed comment 77 and comment 85 (gfx)
Attachment #117284 - Attachment is obsolete: true
|SetTransparentColor()| no longer exists (bug 197485), but I'm not going to
attach a new patch just to remove two lines.
Created attachment 117684 [details] [diff] [review]
Ready for review (modules/libpr0n)

Fixed a whole lot of stuff after testing on an 256-color display. The dither
actually works now :-)
Attachment #117528 - Attachment is obsolete: true
Created attachment 117685 [details] [diff] [review]
Ready for review (gfx)
Attachment #117529 - Attachment is obsolete: true

Updated

14 years ago
Attachment #117685 - Flags: review?(pavlov)

Updated

14 years ago
Attachment #117684 - Flags: review?(pavlov)
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?
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.
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

14 years ago
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...
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.
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

Updated

14 years ago
Flags: blocking1.4a? → blocking1.4a-

Comment 102

14 years ago
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.
Attachment #117684 - Flags: review?(pavlov) → review?(paper)

Comment 103

14 years ago
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=
Attachment #117685 - Flags: review?(pavlov) → review+
Created attachment 118373 [details] [diff] [review]
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.
Created attachment 118374 [details] [diff] [review]
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.
Attachment #117684 - Attachment is obsolete: true
Attachment #117685 - Attachment is obsolete: true

Updated

14 years ago
Attachment #118373 - Flags: superreview?(tor)
Attachment #118373 - Flags: review?(paper)

Updated

14 years ago
Attachment #117684 - Flags: review?(paper)
Comment on attachment 118374 [details] [diff] [review]
Addressed stuart's comments (gfx)

Transferring r=pavlov and requesting sr
Attachment #118374 - Flags: superreview?(tor)
Attachment #118374 - Flags: review+

Comment 107

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

Comment 108

14 years ago
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

14 years ago
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?
It's intentional. CopyFrameImage() can fail if the frames have different color
depths, and if that happens I need to fall back to DrawTo().
> * 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
Created attachment 118505 [details] [diff] [review]
Addressed tor's comments (modules/libpr0n)
Attachment #118373 - Attachment is obsolete: true

Comment 113

14 years ago
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).
Created attachment 118716 [details] [diff] [review]
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.

Updated

14 years ago
Attachment #118505 - Attachment is obsolete: true
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() ?
Created attachment 118823 [details] [diff] [review]
Patch addressing comments on IRC today (gfx)
Attachment #118374 - Attachment is obsolete: true
Created attachment 118824 [details] [diff] [review]
Patch addressing comments on IRC today (modules/libpr0n)
Attachment #118716 - Attachment is obsolete: true
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)?
Created attachment 119504 [details] [diff] [review]
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.
Attachment #118823 - Attachment is obsolete: true
Created attachment 119506 [details] [diff] [review]
A new approach (modules/libpr0n part)
Attachment #118824 - Attachment is obsolete: true

Updated

14 years ago
Depends on: 201568

Updated

14 years ago
Depends on: 201576
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.

Updated

14 years ago
Blocks: 203448

Updated

14 years ago
Target Milestone: mozilla1.4alpha → mozilla1.5alpha

Comment 122

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

Comment 123

14 years ago
did patch for bug 216430 help here (I'm on Linux right now) ?
Comment on attachment 118373 [details] [diff] [review]
Addressed stuart's comments (modules/libpr0n)

removing obsolete review request
Attachment #118373 - Flags: superreview?(tor)
Attachment #118373 - Flags: review?(paper)
Attachment #118374 - Flags: superreview?(tor)

Comment 125

14 years ago
*** Bug 189805 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 126

14 years ago
Any progress on this one?

Updated

13 years ago
Flags: blocking1.8a?
Keywords: mozilla1.3
Target Milestone: mozilla1.5alpha → ---

Updated

13 years ago
Flags: blocking1.8a? → blocking1.8a-

Comment 127

13 years ago
xah@myrealbox.com, only the assigned person should change the target milestone!
Target Milestone: --- → mozilla1.5alpha

Updated

13 years ago
Target Milestone: mozilla1.5alpha → ---
Mass reassign my image bugs to nobody@mozilla.org
Assignee: smontagu → nobody

Updated

13 years ago
Blocks: 254792
(Assignee)

Updated

11 years ago
Blocks: 124608

Comment 129

11 years ago
Anyone willing to take this as this is topembed+ and P2?
(Assignee)

Comment 130

11 years ago
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...
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?
(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

11 years ago
don't think we're going to do this.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 134

11 years ago
Created attachment 246565 [details] [diff] [review]
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.
Assignee: nobody → alfredkayser
Attachment #119504 - Attachment is obsolete: true
Attachment #119506 - Attachment is obsolete: true
Status: RESOLVED → ASSIGNED
Resolution: WONTFIX → ---
(Assignee)

Updated

11 years ago
Blocks: 331298
(Assignee)

Comment 135

11 years ago
Created attachment 247469 [details] [diff] [review]
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).
Attachment #246565 - Attachment is obsolete: true
Attachment #247469 - Flags: review?(dveditz)

Comment 136

11 years ago
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?
(Assignee)

Comment 137

11 years ago
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 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
Attachment #247469 - Flags: review?(dveditz) → review?
Alfred, any chance of revisiting this bug now that APNG has landed?
(Assignee)

Comment 140

10 years ago
Pavlov is still planning to revamp the image,frame,container structure, and has asked me to wait for that.
(Assignee)

Updated

10 years ago
Blocks: 250290
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.
(Assignee)

Updated

10 years ago
Duplicate of this bug: 95810
(Assignee)

Comment 143

10 years ago
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.
Depends on: 366465, 367281, 386268
(Assignee)

Comment 144

10 years ago
Also bug 216682 needs to be SR'ed and committed before I can make the patch for this bug.
Depends on: 216682
(Assignee)

Comment 145

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

Comment 146

10 years ago
Created attachment 283522 [details] [diff] [review]
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.
(Assignee)

Comment 147

10 years ago
Created attachment 283596 [details] [diff] [review]
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.
Attachment #247469 - Attachment is obsolete: true
Attachment #283522 - Attachment is obsolete: true
Attachment #283596 - Flags: review?
Attachment #247469 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #283596 - Flags: review? → review?(pavlov)
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
Flags: blocking1.9+

Comment 149

10 years ago
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...

Updated

10 years ago
Blocks: 399925

Updated

10 years ago
Target Milestone: --- → mozilla1.9 M10

Comment 150

10 years ago
Alfred?  Any ideas on my comment?
(Assignee)

Comment 151

10 years ago
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.
(Assignee)

Comment 152

10 years ago
Created attachment 286948 [details] [diff] [review]
Unbitrotted again. No other changes.
Attachment #283596 - Attachment is obsolete: true
Attachment #286948 - Flags: review?(pavlov)
Attachment #283596 - Flags: review?(pavlov)
(Assignee)

Comment 153

10 years ago
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.
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.
I'm also getting a LOT of random crashiness with v8 that I wasn't getting with v7.

Comment 156

10 years ago
I haven't tested this patch yet, but it looks like some parts might break apngs a bit
(Assignee)

Comment 157

10 years ago
Created attachment 287392 [details] [diff] [review]
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.
(Assignee)

Updated

10 years ago
Attachment #286948 - Attachment is obsolete: true
Attachment #286948 - Flags: review?(pavlov)
(Assignee)

Updated

10 years ago
Attachment #287392 - Flags: review?(pavlov)

Updated

10 years ago
Attachment #287392 - Flags: review?(pavlov) → review+
(Assignee)

Updated

10 years ago
Attachment #287392 - Flags: superreview?(tor)

Comment 158

10 years ago
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.
Attachment #287392 - Flags: superreview?(tor) → superreview+
Did the issue from comment 149 ever get addressed?

Comment 160

10 years ago
mostly -- we're doing a followup patch to fully address them
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago10 years ago
QA Contact: chrispetersen → general
Resolution: --- → FIXED
(Assignee)

Comment 162

10 years ago
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!
Status: RESOLVED → VERIFIED
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.
Flags: in-testsuite?

Updated

10 years ago
Depends on: 403363

Updated

10 years ago
Depends on: 404898

Updated

10 years ago
Depends on: 403578

Updated

10 years ago
Depends on: 408073
Product: Core → Core Graveyard
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?
Depends on: 522155

Comment 165

7 years ago
(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?
You need to log in before you can comment on or make changes to this bug.