104999 - Avoid keeping redundant image data in memory

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Cathleen]

in our current drawing code, we should not keep 2 copies of images around, both
DIB and raw RGB data.  It is costing a lot of memory.

Comment 1

[Simon Fraser [no longer active]]

Can you repeat that in a way that us non-Unix and non-Windows weenies can 
understand? DIB?

Comment 2

[Cathleen]

We are keeping both device independent bitmaps (DIB) data and Red Green Blue
(RGB) code around for drawing.  This is taking too much memory.

According to waterson's profiling:
> 
> Images appear to account for about 40% of the memory used to load a recent 
> version of www.cnn.com; about 1.2MB on that page (see 
> <news://news.mozilla.org/[e-mail]>). This could be 
> especially sucky if we're keeping two copies of the image data around (e.g., on 
> Windows: one copy as a DIB and one copy as our own RGB data; on Linux: one copy 
> in the X server and one copy in our address space). 
>

Pavlov is trying to figure out a way to make drawing code work, with just one
copy of the img data.

Comment 3

[Brendan Eich [:brendan]]

DIB may be the wrong term -- at the Intel meeting, the proposal was to put image
data in the graphics card and keep the CPU from having to load and store to push
pixels.  Images in graphics cards sound like DDB to me, but what do I know?

The savings here, just to clarify, are in better L2 cache hit rate, and in fewer
cycles spent in the CPU messing with pixel data, as well as in RAM.

/be

Comment 4

[Stuart Parmenter]

Attachment: first patch to clean up/add new method to gfxIImageFrame

Comment 5

[timeless]

r=timeless w/ @param that isn't there removed [@@ -167,6 +161,19 @@] from comment.
and assertion text [@@ -221,6 +227,10 @@] changed. I'm 99% sure that's backwards. 
I think this [@@ -209,6 +213,8 @@] is also backwards.

Comment 6

[Simon Fraser [no longer active]]

Ajust summary so I can understand the bug. Sorry pav.

Comment 7

[Stuart Parmenter]

Attachment: another small patch

Comment 8

[Stuart Parmenter]

Comment on attachment 54045 [details] [diff] [review]
first patch to clean up/add new method to gfxIImageFrame

this patch has been checked in

Comment 9

[Johnny Stenback (:jst)]

In imgContainer.cpp:

+      nsCOMPtr<nsIInterfaceRequestor> ireq(do_QueryInterface(mCompositingFrame));
+      if (ireq) {
+        nsCOMPtr<nsIImage> img(do_GetInterface(ireq));
+        img->SetDecodedRect(0, 0, mSize.width, mSize.height);
+      }

no need to QI mConpositingFrame to nsIInterfaceRequestor, do_GetInterface() does
that for you. And how about null checking |img| after calling do_GetInterface()?

sr=jst with that fixed.

Comment 10

[Johnny Stenback (:jst)]

Never mind about the nsIInterfaceRequestor and do_GetInterface(), you need the
QI for using do_GetInterface() due to a lame implementation of do_GetInterface().

Comment 11

[jag (Peter Annema)]

Comment on attachment 54178 [details] [diff] [review]
another small patch

>Index: decoders/jpeg/nsJPEGDecoder.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp,v
>retrieving revision 1.36
>diff -u -r1.36 nsJPEGDecoder.cpp
>--- nsJPEGDecoder.cpp	2001/10/12 06:43:41	1.36
>+++ nsJPEGDecoder.cpp	2001/10/19 03:11:37
>@@ -865,6 +865,7 @@
>     src->decoder->mObserver->OnStopContainer(nsnull, nsnull, src->decoder->mImage);
>     src->decoder->mObserver->OnStopDecode(nsnull, nsnull, NS_OK, nsnull);
>   }
>-
>-    /* No work necessary here */
>+  if (src->decoder->mFrame) {
>+    src->decoder->mFrame->SetMutable(PR_FALSE);
>+  }
> }

Don't need that null check.

>Index: src/imgContainer.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/modules/libpr0n/src/imgContainer.cpp,v
>retrieving revision 1.15
>diff -u -r1.15 imgContainer.cpp
>--- imgContainer.cpp	2001/09/05 21:28:00	1.15
>+++ imgContainer.cpp	2001/10/19 03:11:38
>@@ -162,10 +165,14 @@
>        (numFrames >= 1)) // Not sure if I want to create a composite frame for every anim. Could be smarter.
>     {
>       mCompositingFrame = do_CreateInstance("@mozilla.org/gfx/image/frame;2");
>-      mCompositingFrame->Init(0, 0, mSize.width, mSize.height, gfxIFormats::RGB_A1); 
>-      nsCOMPtr<nsIImage> img(do_GetInterface(mCompositingFrame));
>-      img->SetDecodedRect(0, 0, mSize.width, mSize.height);
>-      
>+      mCompositingFrame->Init(0, 0, mSize.width, mSize.height, gfxIFormats::RGB_A1);
>+
>+      nsCOMPtr<nsIInterfaceRequestor> ireq(do_QueryInterface(mCompositingFrame));

Bwegh. I still say you should just null-check |img| (and we'll have to remove that assertion in
|nsGetInterface::operator()|).

But whatever, as long as I don't see this code here a month from now, r=jag.

Comment 12

[jag (Peter Annema)]

jst: the only thing lame about the implementation of do_GetInterface is the
assertion.

Comment 13

[Stuart Parmenter]

Comment on attachment 54178 [details] [diff] [review]
another small patch

patch checked in

Comment 14

[Stuart Parmenter]

Attachment: Big Nasty Patch (currently windows only)

Comment 15

[Stuart Parmenter]

Attachment: Nasty Patch (minus useless changes) (windows only)

Comment 16

[Stuart Parmenter]

OK.  So here's what this patch does:
It makes nsRenderingContextWin communicate with gfx2's gfxIImageFrame(Win) 
directly to draw images.  I've checked in a new image impl (nsImageWin 
replacement) into gfx2/src/windows as not part of the build.  This patch has a 
makefile change to get this stuff to be built.

"What problems will this patch introduce" you might ask.
1) Tiling on Windows95 (only) will be broken since it only supports 8x8 
patterns.  I'll be working on this in the next day or so.
2) Printing...  I hate printing... Not all printing is broken, but enough is.  
The basic problem with printing at this point is that since gfxImageFrameWin 
stores images (that it can) as DDBs and throws away the DIB data, there is no 
hope of restoring the original data.  What this means is that printing needs to 
redecode the image.  What this is going to require is some changes to the way 
images are cached, and could potentially result in somewhat of a slowdown, 
however I am unsure of how big of one at this point.  The changes this contains 
are:
- Have a 2 step cache.  Step 1 would look for images on the screen.  It would 
probably be a simple hashtable lookup.  Step 2 would be to look in the actual 
memory cache for decoded image data, not as a stream, but as a chunk of data, 
to decode all at once, when needed.

What this allows us to do is to get the original image, out of the memory 
cache, redecode and convert it to a DDB (or leave it as a DIB) depending on if 
the printer device supports this.

---

The less correct (but much easier) way to solve this would be to just use 
GetDIBits on the DDB and convert it back to a DIB.  This isn't as correct as 
there may be data loss between the DDB to DIB conversion, however, it may 
be "good enough" for now.  I will look in to doing it this way first since it 
shouldn't take long.

Comment 17

[[not reading bugmail]]

Suggestion: 

can the images be dumped to the disk cache then loaded to memory on print? 
Which is where most of the images are stored anyway, right or no?  maybe instead
of creating a conversion routine?

-dennis

Comment 18

[Michael Hore]

I remember one of the concerns with this scheme and printing, is that DDBs
contain just enough data to display the image on the screen as well as the
screen is able. Since printers can have a higher resolution/colour depth, we
need the DIB data to print the picture most accurately. How are you dealing with
this?

Comment 19

[Andrew Hagen]

Replying to the above concern, here is a possibility. When it first goes to a
page and downloads the image data, Mozilla could store only one copy of each
image, the smaller copy, in memory. Whenever a user prints a page, the images
could be downloaded again to get the richer image data. It is reasonable to
assume that images will continue to reside on servers, even though time passes
between when the page is downloaded and when it is printed. I'm not sure, but I
think Netscape 4.x did it this way. Apologies in advance if this is spam.

Comment 20

[Stuart Parmenter]

We can't get the data back from the server, but we can "reload" it from the
cache (think dynamically generated images).  I'm considering changing the way
the image cache works to store the original image around in cases where there
are no refs to the image anymore.. not entirely sure yet.

Comment 21

[Stuart Parmenter]

Attachment: New rendering context code

New rendering context code to take advantage of the gfx2 gfxImageFrameWin code.

Comment 22

[Stuart Parmenter]

Attachment: patch to gfx2 to build the new windows code

Patch to gfx2 to make the new windows code be built.  You will want to remove
*gfx2* from components.

Comment 23

[Stuart Parmenter]

Attachment: Patch to call Optimize on the image

This patch makes the gfxImageFrame code call Optimize on the nsIImage

Comment 24

[dcone (gone)]

That patch did not post.

Comment 25

[Stuart Parmenter]

Attachment: Patch to call Optimize()

Comment 26

[dcone (gone)]

Comment on attachment 66329 [details] [diff] [review]
Patch to call Optimize()

r=dcone

Comment 27

[Chris Waterson]

Comment on attachment 66329 [details] [diff] [review]
Patch to call Optimize()

sr=waterson

Comment 28

[Daniel Switkin]

Could we please get an update to the Optimize() comment in nsIImage.h? Something
which explains the purpose of this method and what guarantees Optimize() makes,
for example:

1. Should you delete the Mozilla-native image and alpha data? (newbie question)
2. Are subsequent calls to GetBits() and GetAlphaBits() permitted (return nsnull
or assert)?
3. Should subsequent calls to ImageUpdated() assert?

Comment 29

[Cathleen]

some test data on pav's patch...
using in98, 64MB 200mHz

page-load:
trunk (1/29) 2646 2645 2647 --> avg 2646 ms
w/ patch     2617 2633 2645 --> avg 2631 ms
                             -----------------
                                diff  15 ms   ( -0.5%)

memory:
trunk (1/29) 
   Data KB  20,000 20,124 20,424 
   Code KB   9,596  9,596  9,596
w/patch 
   Data KB  14,472 15,500 15,504
   Code KB   9,596  9,596  9,596

 (showing reduction in Data KB)

Comment 30

[Nisheeth Ranjan]

Sweet! Lets get this puppy checked in for 0.9.9!  Great work, Pav!

Comment 31

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

Since this looks like a massive footpring win for Windows, any chance for this
for Unix/X11 builds?  It might even improve remote-X11 performance (less data
transfer to redraw the screen).

Comment 32

[Stuart Parmenter]

I'm changing the platform of this bug to windows.  marking it fixed.

tor is currently working on a patch that would basically do the same thing on
linux.  I don't recall the bug number off the top of my head, but I will find it.

Comment 33

[Stuart Parmenter]

fixed

Comment 34

[tor]

Bug 121015 is the bug for the nsImageGTK work starting along this path.

Comment 35

[Carlo]

Is this an issue on the Macintosh (Fizzilla for example) ?

Comment 36

[Simon Fraser [no longer active]]

No so much. We don't have much control over where the image bits go on Mac. There 
are some flags for NewGWorld, but we don't use GWorlds for images (because of 
their overhead).

Comment 37

[Ken]

Did the patch get pushed to the trunk? I can't find the bug number or the file 
to be modified, gfxImageFrame.cpp, in the recent checkins (2/6/02-2/20/02)

Comment 38

[Stuart Parmenter]

i checked it in earlier than 2/6/02 i believe.

Comment 39

[Sören 'Chucker' Kuklau (gone)]

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=gfxImageFrame.cpp&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=month&mindate=&maxdate=&cvsroot=%2Fcvsroot

01/30/2002 14:17

Comment 40

[Jerry]

is it fixed?
I saw no patch for other platform.

IMO, some gfx code are platform depend, but some are platform independ.
Now I only see patch for independ code and win specific code,
no patch for other platform.