689623 - layout needs to provide information on which images are visible or likely to be visible

Status: VERIFIED FIXED

(Core :: Layout, defect)

Comment 1

[Justin Lebar (not reading bugmail)]

It would be awesome if we could register an observer which gets fired when an image enters/leaves the box defined by the viewport plus X pixels of padding.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

This might be somewhat orthogonal to the suggestion in Comment 1, but how hard would it be to generate a list of coordinates for each image on a page relative to the centre of the viewport? You could then sort this list in order of distance from the centre, and decode images in that order.

If we only consider vertical scrolling, this sorting should be simple:
1) special case and prioritize images that straddle the centre of the viewport
2) find all images whose lower edge is above the viewport and take image.lower - viewport.centre as the distance from the centre
3) find all images whose upper edge is bllow the viewport and take viewport.centre - image.lower as the distance from the centre
4) sort the resulting list by distance from the centre, where images identified as straddling the viewport are defined as strictly closer than the others

If we also consider horizontal scrolling (less common) the approach would become a bit more complicated (as images straddling the centre of the viewport in the y direction might be very distant in the x direction).

Comment 3

[Timothy Nikkel (:tnikkel)]

How are images currently kept track of? Everything in the visible tab is decoded and hence tracked?

Comment 4

[Joe Drew (not getting mail)]

Everything in the visible tab is locked and never discarded, and thus not tracked.

Comment 5

[Nicholas Nethercote [inactive]]

roc, this is very important for MemShrink -- any ideas who could/should work on it?

Comment 6

[Timothy Nikkel (:tnikkel)]

I was planning on working on this.

Comment 7

[Joe Drew (not getting mail)]

Note that this doesn't need to be super-predictive. We don't (initially, at least) need to predict that things are "going to be on screen soon" due to the direction the user's scrolling; it'd be enough to simply know "These images are currently on screen" and have "on screen" defined as "screen + some slack above/below."

That is, if this was implemented with a hard-coded window of one screen height above and below (and one screen width to the right and left) of the viewport, with callbacks for "this DOM element is now/is no longer in the viewport area", I'd be *ecstatic*.

Comment 8

[Nicholas Nethercote [inactive]]

> That is, if this was implemented with a hard-coded window of one screen
> height above and below (and one screen width to the right and left) of the
> viewport, with callbacks for "this DOM element is now/is no longer in the
> viewport area", I'd be *ecstatic*.

Yes!  Something simple would get us most of the way there.  It could be tweaked later if necessary.

Comment 9

[Nicholas Nethercote [inactive]]

tnikkel told me he is working on this, and he hopes to attach a patch and/or a proposal soon.

Comment 10

[Jet Villegas (inactive)]

Per our design brainstorm, let's try this heuristic:

for each viewPort
{
	for each visible scrollFrame
	{
		list images for 1 scrollFrameHeight +/- 1 offscreenScrollFrameHeight ) 
		return imageList
	}
}

refresh the list when the scrollFrame moves 1/2 a scrollFrameHeight.

Comment 11

[Nicholas Nethercote [inactive]]

Has there been any progress here?

Comment 12

[Timothy Nikkel (:tnikkel)]

Yeah, but I don't have a patch to post yet.

Comment 13

[Justin Lebar (not reading bugmail)]

If it's any motivation, apparently on mobile we discard images on the active tab, but because we don't have information from layout about which images are visible, it's totally broken.  See bug 691169.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: not working yet

Here is something very hacky, doesn't work yet, and doesn't consider plenty of important cases.

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 16

[Timothy Nikkel (:tnikkel)]

There are some try builds here https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0637e92f1637/ if anyone wants to try.

There are prefs that control how much scrolling is allowed before we update the image visibility. They are called
layout.imagevisibility.amountscrollbeforeupdatevertival
layout.imagevisibility.amountscrollbeforeupdatehorizontal
and are integers. If the value is N, then if you scroll more than 1/N of the scrollport size in that direction we update image visibility.

There are prefs to control how far to look up/down/left/right in scroll frames:
layout.imagevisibility.numscrollportheights
layout.imagevisibility.numscrollportwidths
A value of 1 indicates include 1 scroll port size up/down or left/right.

Comment 17

[ferongr]

What happens when a script loads a bunch of images asynchronously but displays only one of them at a time (e.g. a slideshow that preloads images)? Does the current patch only work for the viewport scrolling case where images are all visible. And does this prevent those kinds of webpages that are hit by Bug 683290 from exploding their memory usage for images?

Comment 18

[Justin Lebar (not reading bugmail)]

> And does this prevent those kinds of webpages that are hit by Bug 683290 from exploding their memory 
> usage for images?

The fix for bug 683290 is bug 683290.  This is a totally separate issue, which only affects our heuristics for deciding when to discard images which are in the DOM.

Comment 19

[Justin Lebar (not reading bugmail)]

Timothy, there was some confusion in today's MemShrink meeting as to the status of this bug.

Are you waiting for any feedback from us, or are you still working through some issues here?

Comment 20

[Timothy Nikkel (:tnikkel)]

No, I'm not waiting for feedback, although if you have any I would like to hear it.

There is still more work to be done here, but I am working on mobile stuff right now.

Comment 21

[Justin Lebar (not reading bugmail)]

Supposing we wanted to try to finish this patch during the MemShrink meetup next week, do you think that would be feasible?  What remains to be done here?  I admit to knowing nothing about layout, but we'll be near roc's time zone!  Does that count for something?  :)

Comment 22

[Timothy Nikkel (:tnikkel)]

The biggest thing is that it needs to be extended to handle background images.

Comment 23

[Jet Villegas (inactive)]

...and border images too?

Comment 24

[Jet Villegas (inactive)]

Attachment: Add background-image & border-image support

tn: I added some code to handle background and border images. Please check.

Comment 25

[Jet Villegas (inactive)]

tn: all my significant changes are in between line 5379 & 5411 in nsPresShell.cpp

Comment 26

[Timothy Nikkel (:tnikkel)]

Comment on attachment 615263 [details] [diff] [review]
Add background-image & border-image support

Did you forget to qref? I don't see any changes.

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #26)
> Did you forget to qref? I don't see any changes.

(In reply to Jet Villegas (:jet) from comment #25)
> tn: all my significant changes are in between line 5379 & 5411 in
> nsPresShell.cpp

Oh. Nevermind.

Comment 28

[Timothy Nikkel (:tnikkel)]

Comment on attachment 615263 [details] [diff] [review]
Add background-image & border-image support

It would be easier to evaluate this if you did it as a patch on top of mine.

>+GatherImagesFromList(nsDisplayListBuilder* aBuilder, const nsDisplayList& aList,
>+                     nsTHashtable<nsPtrHashKey<imgIRequest> > & aImagesVisible)
>+    if (!f) 
>+      return;

You don't want to return here.

>+    //xxx more display items need to do this     
>+    imgIRequest* request = item->GetImage(aBuilder);
>+    if(request){
>+      //append image to imagelist; //xxx frame, item, imgrequest? etc
>+      aImagesVisible.PutEntry(request);
>+    }
>+    
>+    // now check for background images
>+    request = nsnull;
>+    const nsStyleBackground* styleBackground = f->GetStyleBackground();
>+  
>+    NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT(i, styleBackground) {
>+      if (styleBackground->mLayers[i].mImage.GetType() == eStyleImageType_Image) {
>+        request = styleBackground->mLayers[i].mImage.GetImageData();
>+        if(request){
>+          aImagesVisible.PutEntry(request);
>+          request = nsnull;
>+        }
>+      }
>+    }
>+    
>+    // now check for border image
>+    request = nsnull;
>+    const nsStyleBorder* styleBorder = f->GetStyleBorder();
>+    request = styleBorder->GetBorderImage();
>+    if(request){
>+      aImagesVisible.PutEntry(request);
>+    }
>+    
>+  }
>+}

If we're going to ignore the GetImage function on the display item for background and borders we may as well not add it and do image frames the same way.

I think you'll also want to get rid of the locking that the style system does for border and background images.

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 615263 [details] [diff] [review]
Add background-image & border-image support

>+    // now check for background images
>+    request = nsnull;
>+    const nsStyleBackground* styleBackground = f->GetStyleBackground();
>+  
>+    NS_FOR_VISIBLE_BACKGROUND_LAYERS_BACK_TO_FRONT(i, styleBackground) {
>+      if (styleBackground->mLayers[i].mImage.GetType() == eStyleImageType_Image) {
>+        request = styleBackground->mLayers[i].mImage.GetImageData();
>+        if(request){
>+          aImagesVisible.PutEntry(request);
>+          request = nsnull;
>+        }
>+      }
>+    }
>+    
>+    // now check for border image
>+    request = nsnull;
>+    const nsStyleBorder* styleBorder = f->GetStyleBorder();
>+    request = styleBorder->GetBorderImage();
>+    if(request){
>+      aImagesVisible.PutEntry(request);
>+    }

This will also be inefficient as frames can create multiple display items. So we will consider the same frame several times, and add its images to the list several times. I don't think we want to do that.

Comment 30

[Jet Villegas (inactive)]

Attachment: Adds background-image and border-image support.

New patch using qref for easier reviewing.

Comment 31

[Jet Villegas (inactive)]

(In reply to Timothy Nikkel (:tn) from comment #28)
> Comment on attachment 615263 [details] [diff] [review]
> Add background-image & border-image support
> 
> It would be easier to evaluate this if you did it as a patch on top of mine.
> 
> >+GatherImagesFromList(nsDisplayListBuilder* aBuilder, const nsDisplayList& aList,
> >+                     nsTHashtable<nsPtrHashKey<imgIRequest> > & aImagesVisible)
> >+    if (!f) 
> >+      return;
> 
> You don't want to return here.
> 

Right! Changed to continue;

> >+    
> >+    // now check for border image
> >+    request = nsnull;
> >+    const nsStyleBorder* styleBorder = f->GetStyleBorder();
> >+    request = styleBorder->GetBorderImage();
> >+    if(request){
> >+      aImagesVisible.PutEntry(request);
> >+    }
>
> If we're going to ignore the GetImage function on the display item for
> background and borders we may as well not add it and do image frames the
> same way.

Do you think think the method for retrieving the borders and backgrounds is OK in this patch? It's cleaner to keep GetImage() but since non-image frames can have borders and backgrounds, I didn't see a way to extract those images without the underlying nsFrame.

> I think you'll also want to get rid of the locking that the style system
> does for border and background images.

Agreed.

Comment 32

[Jet Villegas (inactive)]

> >+    request = nsnull;
> >+    const nsStyleBorder* styleBorder = f->GetStyleBorder();
> >+    request = styleBorder->GetBorderImage();
> >+    if(request){
> >+      aImagesVisible.PutEntry(request);
> >+    }
> 
> This will also be inefficient as frames can create multiple display items.
> So we will consider the same frame several times, and add its images to the
> list several times. I don't think we want to do that.

Are you referring to the use of a DisplayList for the iteration, or the additional lookup of backgrounds/borders?

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Jet Villegas (:jet) from comment #31)
> > If we're going to ignore the GetImage function on the display item for
> > background and borders we may as well not add it and do image frames the
> > same way.
> 
> Do you think think the method for retrieving the borders and backgrounds is
> OK in this patch? It's cleaner to keep GetImage() but since non-image frames
> can have borders and backgrounds, I didn't see a way to extract those images
> without the underlying nsFrame.

GetImage has nothing to do with image frames, it's just a way for display items to return an imgIRequest. This method is not ok because of the problem I mention in comment 29. I'm not saying ignore the underlying frame.

(In reply to Jet Villegas (:jet) from comment #32)
> > This will also be inefficient as frames can create multiple display items.
> > So we will consider the same frame several times, and add its images to the
> > list several times. I don't think we want to do that.
> 
> Are you referring to the use of a DisplayList for the iteration, or the
> additional lookup of backgrounds/borders?

If a frame creates two display items this code will run for both display items, and add each background and border image to the list two times. I think we only want to add each background and border image to the list once.

Comment 34

[Jet Villegas (inactive)]

Attachment: Updated support for background-images & border-images

More progress on background-images & border-images to address review comments.

Comment 35

[Timothy Nikkel (:tnikkel)]

Comment on attachment 615609 [details] [diff] [review]
Updated support for background-images & border-images

I think we should either use the GetImage method (even if we need to change how it works) or just peak at each display item's type and handle the ones we want, but not do one for some things and the other for other things.

Comment 36

[Jet Villegas (inactive)]

Actually, I'm seeing that this patch isn't capturing all the background and border images and will need more work if it's to be comprehensive. I think we're better off fixing the worst offenders first (sites like the facebook.com timeline and canv.as) that scroll to infinity. background-image and border-image aren't generally large JPEGs. I'll rework the patch to concentrate on the image scroller sites and see how memory usage is helped.

Comment 37

[Jet Villegas (inactive)]

Attachment: Added support for GetImage (now GetImageRequests) to image, border, and background displayList items.

This patch fixes all the backgrounds and borders cases now. There's still a bug where images don't load on button hover due to the fact that visibility is only updated on scroll.

Comment 38

[Timothy Nikkel (:tnikkel)]

Comment on attachment 616043 [details] [diff] [review]
Added support for GetImage (now GetImageRequests) to image, border, and background displayList items.

Can you upload this as a patch on top of mine? Much easier to evaluate that way.

Comment 39

[Timothy Nikkel (:tnikkel)]

Comment on attachment 616043 [details] [diff] [review]
Added support for GetImage (now GetImageRequests) to image, border, and background displayList items.

I think we should not go with the 'adding a virtual method on every display item' method and go with inspecting the display item types in the gather images function. We will probably have to handle every caller of nsCSSRendering::PaintBackground, and we don't want to duplicate the background code that many times (or at all). This patch has the worst of both worlds as it's doing both.

Comment 40

[Jet Villegas (inactive)]

Attachment: interdiff of last patch for backgrounds/borders support

Here you go. Just learned how to use interdiff :)

Comment 41

[Timothy Nikkel (:tnikkel)]

(In reply to Jet Villegas (:jet) from comment #40)
> Here you go. Just learned how to use interdiff :)

You don't need to do an interdiff. You probably want to keep the original patch in your patch queue and then have your work as a patch on top of it.

Comment 42

[Timothy Nikkel (:tnikkel)]

(In reply to Jet Villegas (:jet) from comment #37)
> There's still a
> bug where images don't load on button hover due to the fact that visibility
> is only updated on scroll.

This is a serious problem and we need to fix it. I will think about this.

Comment 43

[Jet Villegas (inactive)]

(In reply to Timothy Nikkel (:tn) from comment #39)
> I think we should not go with the 'adding a virtual method on every display
> item' method and go with inspecting the display item types in the gather
> images function. We will probably have to handle every caller of
> nsCSSRendering::PaintBackground, and we don't want to duplicate the
> background code that many times (or at all). This patch has the worst of
> both worlds as it's doing both.

That code was leftover cruft from earlier. The latest patch removes the multiple if statements and just adds the virtual methods.  I think all the subclasses of nsDisplayBackground are actually getting caught now so that's good. 

Let me know what you think about the hover problem. 

I think I've got a handle on hg patch queues now so the next patches should be cleaner.

Comment 44

[Timothy Nikkel (:tnikkel)]

(In reply to Jet Villegas (:jet) from comment #43)
> That code was leftover cruft from earlier. The latest patch removes the
> multiple if statements and just adds the virtual methods.

Ok, I see that, but I still think we shouldn't go the 'add virtual function to every display item' route.

> I think all the
> subclasses of nsDisplayBackground are actually getting caught now so that's
> good. 

There is only one subclass of nsDisplayBackground. There are many more callers of nsCSSRendering::PaintBackground. I think they will all need the same treatment.

Comment 45

[Timothy Nikkel (:tnikkel)]

roc accidentally posted this comment to bug 732016.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Personally I think a new virtual method on all display items is cleaner than
> checking display item types.
> 
> I wonder though, would it make more sense to just walk the frame tree to
> find images instead of building display lists? Is there much win from
> culling images that are covered by other stuff? If not, we should probably
> just walk the frame tree.

Comment 46

[Timothy Nikkel (:tnikkel)]

> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> > Personally I think a new virtual method on all display items is cleaner than
> > checking display item types.

Except that we'd have to duplicate the same code in many different display items that draw background with nsCSSRendering::PaintBackground.

> > I wonder though, would it make more sense to just walk the frame tree to
> > find images instead of building display lists? Is there much win from
> > culling images that are covered by other stuff? If not, we should probably
> > just walk the frame tree.

Images that are covered wasn't really my concern, and we can drop the compute visibility call. The main advantage of using the display list infrastructure was that it took into account transform and coordinates and clipping, and everything by writing very little new code. We could do it the frame tree walk way...

Comment 47

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

(In reply to Timothy Nikkel (:tn) from comment #46)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> > > Personally I think a new virtual method on all display items is cleaner than
> > > checking display item types.
> 
> Except that we'd have to duplicate the same code in many different display
> items that draw background with nsCSSRendering::PaintBackground.

They can all call a single helper method, can't they?

> > > I wonder though, would it make more sense to just walk the frame tree to
> > > find images instead of building display lists? Is there much win from
> > > culling images that are covered by other stuff? If not, we should probably
> > > just walk the frame tree.
> 
> Images that are covered wasn't really my concern, and we can drop the
> compute visibility call. The main advantage of using the display list
> infrastructure was that it took into account transform and coordinates and
> clipping, and everything by writing very little new code. We could do it the
> frame tree walk way...

I don't think we need to care about clipping --- just rely on overflow-area checking, and intersecting the "current dirty rect" with overflow areas to cull out images that are clipped away.

For transforms, use IsTransformed/GetTransformMatrix I guess.

Comment 48

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> > Except that we'd have to duplicate the same code in many different display
> > items that draw background with nsCSSRendering::PaintBackground.
> 
> They can all call a single helper method, can't they?

That would work.

> I don't think we need to care about clipping --- just rely on overflow-area
> checking, and intersecting the "current dirty rect" with overflow areas to
> cull out images that are clipped away.
> 
> For transforms, use IsTransformed/GetTransformMatrix I guess.

Of course it can all be handled, but it would be new code, and it would have to be kept updated if any thing relevant changes with how we handle display lists.

Comment 49

[Jet Villegas (inactive)]

I've managed to catch most of the background images now using the displayList. I hit a snag with table cell backgrounds that are not readily accessible from a displayItem. I'd prefer not to have new code to traverse the table layout. Can we traverse the frame tree for the tables instead? Do we have existing code that does that?

Comment 50

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

(In reply to Timothy Nikkel (:tn) from comment #48)
> > I don't think we need to care about clipping --- just rely on overflow-area
> > checking, and intersecting the "current dirty rect" with overflow areas to
> > cull out images that are clipped away.
> > 
> > For transforms, use IsTransformed/GetTransformMatrix I guess.
> 
> Of course it can all be handled, but it would be new code, and it would have
> to be kept updated if any thing relevant changes with how we handle display
> lists.

IsTransformed/GetTransformMatrix already works.

I guess one problem with frame-tree walking that's handled by display lists is where you have SVG filters causing content that's technically outside the viewport to affect content inside the viewport (via feOffset or feGaussianBlur).

However, there are similar problems that aren't handled by display lists: SVG <foreignobject>, whose content currently doesn't show up in the main display list, and CSS -moz-element references, where we need to declare that images in referenced elements are "visible".

This is really quite a tough problem :-(.

Comment 51

[Jet Villegas (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> However, there are similar problems that aren't handled by display lists:
> SVG <foreignobject>, whose content currently doesn't show up in the main
> display list, and CSS -moz-element references, where we need to declare that
> images in referenced elements are "visible".
> 
> This is really quite a tough problem :-(.

Roc, we'd like to solve this for the foreground images that aren't visible on the viewport. Sites that scroll to infinity, like the Facebook timeline and canv.as are the use cases we'd like to reduce memory for. Can we safely modify the image locking/unlocking logic for those image types? That may be enough for now.

Comment 52

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

What are the consequences going to be if we try to paint an image that was thought to be not visible, but actually is?

If those consequences are just that we do a sync image decode and from then on we keep it decoded indefinitely, possibly janking a little and wasting memory but no worse on memory than today, then it doesn't matter too much if we ignore the edge cases I just raised.

Given you have to walk frames to handle tables, I do think that just walking the frame tree and explicitly handling transforms is the way to go. It can later be extended to handle the issues I mentioned above, if we want to.

Comment 53

[Justin Lebar (not reading bugmail)]

This was waiting until after Fennec Beta, which has now shipped.

Can we get an update on the timeframe here?  This is particularly important for low-end devices, such as those B2G is targeting.  (Fennec already does this crazy thing where it draws unlocked images, leading to images randomly disappearing...)

Comment 54

[Lawrence Mandel [:lmandel] (use needinfo)]

While this is definitely a Snappy priority this is a - for k9o at this point. We should renom if we can come up with a specific use case for B2G, Fennec or desktop that is in scope for k9o that can justify including this performance work in this project.

Comment 55

[Justin Lebar (not reading bugmail)]

The specific use-case is "Loading pages with many images without crashing the device."

For example, [1] loads 150mb of decoded images.  The (extremely preliminary) numbers I have show that B2G uses 50mb or RAM on startup, plus the system uses another 100mb of RAM for its own purposes.  So on a hypothetical device with 256mb of RAM, loading [1] would crash the process.

What we do on Fennec is an unsafe hack where we show images but allow Firefox to discard them at will.  I'm frankly amazed that patch got r+'ed -- it's not a supported configuration as far as I'm concerned.

But if we don't have this patch, and we don't use the hack we're using on Fennec, then loading a page with a lot of large images will crash the process.

[1] http://www.theatlantic.com/infocus/2012/05/scenes-from-brazil/100300/

Comment 56

[Joe Drew (not getting mail)]

Justin calling it an "unsafe" hack is perhaps overstating it a little. It's perfectly safe to draw a discarded image; that is what we do every time we go back to a background tab on desktop, for example. It is ugly, though!

Comment 57

[Andrew McCreight [:mccr8]]

Another use case is something like Facebook or Google Images where you have a page that keeps scrolling and scrolling, loading as it goes, and there are images across the whole strip, but probably you aren't spending much time going back to the old images.

Comment 58

[Bob Clary [:bc] (inactive)]

For Firefox 12, EMPTY: no crashing thread identified; corrupt dump is the number 2 crash at 7.11%. In my crash automation testing, I regularly see crashes due to pages which contain an extremely large number of images which cause either oom aborts or more dangerous crashes when the oom is not detected or handled properly. I can't say how many of these corrupt dump crashes are due to the image-suck oom crashes, but I'll wager a nice beverage that eliminating these image-suck oom crashes will reduce them significantly.

I believe eliminating image-suck oom crashes would be of significant benefit to not only the desktop but to mobile as well.

Comment 59

[Justin Lebar (not reading bugmail)]

(In reply to Joe Drew (:JOEDREW!) from comment #56)
> Justin calling it an "unsafe" hack is perhaps overstating it a little. It's
> perfectly safe to draw a discarded image

The question is whether it's safe to draw an *unlocked* image.  But Joe is telling me on IRC that's relatively safe too.  There are problems: We might decode an image more than once, for example, if we discard it before we copy it to the layer.

In that case, it sounds like we can do the following, with no help from layout:  When we want to discard an image (because we think we're running out of memory, or whatever), simply discard LRU according to the time when the image was Draw()'n.

This is non-ideal, but at least we could make it happen...  I'd probably only want to do this on mobile, because desktop users really don't like images flashing into view.

Comment 60

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Andrew McCreight [:mccr8] from comment #57)
> Another use case is something like Facebook or Google Images where you have
> a page that keeps scrolling and scrolling, loading as it goes, and there are
> images across the whole strip, but probably you aren't spending much time
> going back to the old images.

Can we demonstrate a crash as Justin described in comment 55 on Fennec or B2G with Facebook or Google Images?

Comment 61

[timbugzilla]

Could this feature also enable drawing the images in the viewport first when switching tabs? Perhaps not discarding in-viewport images in background tabs too?(both to make tab switching faster)

Comment 62

[Justin Lebar (not reading bugmail)]

(In reply to timbugzilla from comment #61)
> Could this feature also enable drawing the images in the viewport first when
> switching tabs? Perhaps not discarding in-viewport images in background tabs
> too?(both to make tab switching faster)

That first one we basically have now, because we decode images with high priority if they've been Draw()'n (bug 715308).  Yes to the second one.

Comment 63

[Steven Young]

On behalf of Facebook Inc, let me just say that we are prepared to give over one thousand free "pokes" to anyone who fixes this bug or alternatively https://bugzilla.mozilla.org/show_bug.cgi?id=683290. I will also gladly show you Mark Zuckerberg's desk and give you free dinner and cookies as part of a complementary tour of Facebook HQ in Menlo Park.

Comment 64

[Justin Lebar (not reading bugmail)]

I was thinking about this again, and here's an idea:

As I understand the current state of the patch, it's incomplete because it doesn't handle some edge cases (e.g. background or border images).

If getting the patch into a state where it covers all images is hard, perhaps we could reduce the bug's scope to cover only <img> tags (plus whatever else is easy).  Imagelib could assume that all images are visible until it gets an "invisible" notification for a given image.

We of course wouldn't be able to use locking for these "advisory" visible/invisible notifications, but that seems fine.

Comment 65

[Steven Young]

Sounds like a great idea to me!

Comment 66

[Justin Lebar (not reading bugmail)]

This is by far the worst image-related bug that I'm aware of in Firefox.  It's keeping us from being able to do all sorts of optimizations in imagelib, and it's degrading the tab-switching experience for users (because we can't be smart and not discard images which are visible in a tab, when you switch tabs).

This is a particularly bad bug on mobile, where we need to be even smarter about which images we keep alive, so we don't run out of memory.  So k9o?.

Comment 67

[Timothy Nikkel (:tnikkel)]

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #64)
> As I understand the current state of the patch, it's incomplete because it
> doesn't handle some edge cases (e.g. background or border images).

The patch has other major problems. The edge cases are a smaller issue.

> If getting the patch into a state where it covers all images is hard,
> perhaps we could reduce the bug's scope to cover only <img> tags (plus
> whatever else is easy).

That is what I'm pursuing now.

Comment 68

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 69

[Manoj]

Tim, were you able to get a review for your patch? 

In addition to what Justin mentioned in comment #66, a recent change by Facebook is going to degrade the Facebook experience for Firefox users. Facebook has foisted Timeline on all of its users; it is no longer an optional feature. With this change, Firefox users will experience image-related jank when viewing their friends' timelines.

Comment 70

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 71

[Timothy Nikkel (:tnikkel)]

Comment on attachment 658221 [details] [diff] [review]
wip

Applies on top of bug 784591.

Comment 72

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 73

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 74

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 75

[Timothy Nikkel (:tnikkel)]

Main items known to be left to work on:
assume images are visible by default until we can mark them not visible
try server issues
-not animating gifs
-off by 1 pixel errors in a handful of pixels in an image
-plugin crash?
-others
re-evaluate storage of list of visible image for performance
patch cleanup
testing
handling non-desktop cases (mobile), may be a follow up

Comment 76

[Timothy Nikkel (:tnikkel)]

Attachment: wip

Comment 77

[Timothy Nikkel (:tnikkel)]

Some try builds if anyone wants to try them out and provide feedback

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4b1e36e58e69/

Comment 78

[Jet Villegas (inactive)]

Testing on my Mac. This seems to be working quite nicely--smooth scrolling with no noticeable jank on the Facebook timeline and other sites. Is DLBI enabled in your Try sources?

Comment 79

[Timothy Nikkel (:tnikkel)]

(In reply to Jet Villegas (:jet) from comment #78)
> Is DLBI enabled in your Try sources?

Yes.

Comment 80

[Timothy Nikkel (:tnikkel)]

Updated try build
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b2c5de69ca85/

Comment 81

[Justin Lebar (not reading bugmail)]

Note the current plan is to base B2G off the FF18 branch, which means that this patch won't be in B2G v1.  (It also means that I don't have time to test this code likely until we ship B2G v1.)

Comment 82

[Stefan Fleiter (:sfleiter)]

Tested build from comment 80 which crashed once so far bp-39863b2a-432d-49e4-a342-bab582121010
At Facebook the only page, the only site I use from time to time which has the endless page which endless growing memory for me it caps memory usage very nicely.

Anything special that should be tested?

Comment 83

[Bob Clary [:bc] (inactive)]

Stefan: you might try http://congressoamericano.blogspot.com/p/fotos-do-congresso-americano-iii.html or the other urls from bug 701625

Comment 84

[Timothy Nikkel (:tnikkel)]

(In reply to Stefan Fleiter (:sfleiter) from comment #82)
> Tested build from comment 80 which crashed once so far
> bp-39863b2a-432d-49e4-a342-bab582121010

I can't tell without symbols but that's probably due to bug 731974 which has been backed out since that try build was made.

> Anything special that should be tested?

Anything really, just want to find any potential problems.

Thanks for testing.

Comment 85

[Stefan Fleiter (:sfleiter)]

Does not help for
http://congressoamericano.blogspot.com/p/fotos-do-congresso-americano-iii.html
or
http://o001oo.ru/index.php?showtopic=14210%26st=0
which let the browser consume memory until it explodes.

Using the browser for my daily work for now.

Comment 86

[Jake]

I wonder how you are going to treat a "visibility:hidden" image, it's in the viewport, but it's not visible. For example, I made a slideshow page, it preloads next picture by loading it and hiding it, when views hit the next button,it quickly change "visibility:hidden" to "visibility:visible", if this patch landed, would it still work? what if I swith to other tabs and i don't want the visible images to be discarded ( I assume it can be done with this fix), will it discard the hidden images in the viewport? I think it should be discarded, when I switch back, there is plenty of time to decode them. IMHO, the triage should be first visible images in viewport, then hidden ones, last adjacents, when switching to other tabs, only keep the visible images in viewport alive, discard the hidden and adjacent ones.

Comment 87

[Timothy Nikkel (:tnikkel)]

(In reply to Stefan Fleiter (:sfleiter) from comment #85)
> Does not help for
> http://congressoamericano.blogspot.com/p/fotos-do-congresso-americano-iii.
> html
> or
> http://o001oo.ru/index.php?showtopic=14210%26st=0
> which let the browser consume memory until it explodes.

Ah, thanks for reporting this. The next try build will have a fix for this (can't make one right now due to try being closed).

> Using the browser for my daily work for now.

Thanks!

Comment 88

[Timothy Nikkel (:tnikkel)]

(In reply to Jake from comment #86)
> I wonder how you are going to treat a "visibility:hidden" image, it's in the
> viewport, but it's not visible. For example, I made a slideshow page, it
> preloads next picture by loading it and hiding it, when views hit the next
> button,it quickly change "visibility:hidden" to "visibility:visible", if
> this patch landed, would it still work?

visibility:hidden stuff is considered not visible by my patch. This doesn't affect network loads or anything, only whether to decode and to keep around the decoded data. You can try the try build to see how your site would work.

> what if I swith to other tabs and i
> don't want the visible images to be discarded ( I assume it can be done with
> this fix), will it discard the hidden images in the viewport? I think it
> should be discarded, when I switch back, there is plenty of time to decode
> them. IMHO, the triage should be first visible images in viewport, then
> hidden ones, last adjacents, when switching to other tabs, only keep the
> visible images in viewport alive, discard the hidden and adjacent ones.

This bug isn't intended to address any kind of tab switch behaviour, it only changes what images are considered visible inside a tab.

Comment 89

[Timothy Nikkel (:tnikkel)]

New try build
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7a1502aa55a9/

Comment 90

[Stefan Fleiter (:sfleiter)]

So both urls from comment 85 could be loaded *without* making firefox consume all memory.
With the last build from comment 80 my computer was totally occupied by swapping and did not react anymore at all.

Now things are better even if still not as expected/wished.
Loading http://o001oo.ru/index.php?showtopic=14210%26st=0 in a fresh profile with only the memchaser addon added resident memory grows to more than 1.5 GB.

I have opened about:memory in a second window:

During loading I was able to capture this:
1,541.00 MB (100.0%) -- explicit
├──1,362.40 MB (88.41%) -- images
│  ├──1,362.33 MB (88.41%) -- content
│  │  ├──1,362.31 MB (88.40%) -- used
│  │  │  ├──1,248.87 MB (81.04%) ── uncompressed-heap
│  │  │  ├────113.45 MB (07.36%) ── raw
│  │  │  └──────0.00 MB (00.00%) ── uncompressed-nonheap
│  │  └──────0.02 MB (00.00%) ++ unused
│  └──────0.07 MB (00.00%) ++ chrome

After loading with the page still open memory consumption dropped significantly:
251.92 MB (100.0%) -- explicit
├──113.89 MB (45.21%) -- images
│  ├──113.81 MB (45.18%) -- content
│  │  ├──113.48 MB (45.05%) -- used
│  │  │  ├──113.46 MB (45.04%) ── raw
│  │  │  └────0.02 MB (00.01%) ++ (2 tiny)
│  │  └────0.33 MB (00.13%) ++ unused
│  └────0.07 MB (00.03%) ++ chrome

I would expect that memory usage while loading the page should not be a lot higher than that of the page after it has finished loading.

Could it be that during loading of the page layout is not far enough to decide whether the image is visible and the uncompressed image is needed?

Comment 91

[Justin Lebar (not reading bugmail)]

> Loading http://o001oo.ru/index.php?showtopic=14210%26st=0 in a fresh profile with only the memchaser 
> addon added resident memory grows to more than 1.5 GB.

BTW, memchaser is known to cause long GC pauses and perhaps leaks (bug 751557); I don't recommend using it.  Instead, use |top| or your OS's task manager.  (This doesn't bring into question the validity of your results in comment 90, which shouldn't be affected by memchaser.)

Comment 92

[JC Yang]

I need some clarify on this bug. Is the title right now? If this bug is purely about adding an image visibility query function(or maybe API), memory usage won't be affected at all. Aren't bug 542158 and 661304 the right ones for the actual image handling? Should we update the description?

Comment 93

[Timothy Nikkel (:tnikkel)]

New try builds will show up in a bit at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8c206afeefcc/

Comment 94

[Jake]

I used this bat code building a local html file to test.(save it as bat file and put it into a picutre(jpeg) folder)
@echo ^<html^> > img.html
@echo ^<body^> >> img.html
for /f %%a in ('dir /a /b *.jpg') do (
echo ^<br^> >>img.html
echo ^<img src='%%a'/^> >>img.html
echo ^<br^> >>img.html)
echo ^<br^> >>img.html
@echo ^</body^> >> img.html
@echo ^</html^> >> img.html
results: (with 32 bit xp 3g ram available, 186mb jeg images)
Memory usage will reach over 1G then drop to 4xxM stay at 29xM in the end.
With normal nightly build memory usage will reach 1.5G then stop loading images, dragging down to the bottom of the page, you will find some images are not loaded.
Here is the odd part,with test build, when you drag down to the bottom, you will also find some images (the same as normal build) are not loaded, as if it decodes all the images it can take as usual, then discards ones that are not visible, maybe that's why it still consumes that much memory at the beginning?

Comment 95

[Timothy Nikkel (:tnikkel)]

Yeah, we assume all images are visible for a short period when loading before we paint the page, and then we mark them not visible. So this spike at the beginning and then settling down is a result of that. May need to revise this somehow...

Comment 96

[Florian Bender]

Why not reverse this procedure, i. e. mark every image as not visible (but download it) and then query the layout to see which images are visible? If this causes too much flicker, how about marking only the first 10 <img> and first 15 CSS-Imgs visible and decide for the rest afterwards (or the first 5MB of images, regardless of how many there actually are?)?

Comment 97

[Jake]

Problem now is that if you don't have enough memory left, you still can't view the page properly. But for me, the most annoying problem has been solved, which is when you switch back to a image-heavy tab, firefox will freeze for a few seconds, making you think it's about to crash.It's unbearable and has been bothering me for years, thanks for the excellent work.

Comment 98

[Timothy Nikkel (:tnikkel)]

For mobile, in order to avoid creating another display list I'm thinking we should use the work in bug 783368 that creates a (smaller) critical display port in addition to the display port. We could then reuse the painting display list to determine image visibility and avoid having to create a separate one. I think the best way to handle this is to disable this (keeping the status quo) and address it in a followup bug.

Comment 99

[Timothy Nikkel (:tnikkel)]

Attachment: Adjust some reftest assert counts.

Comment 100

[Timothy Nikkel (:tnikkel)]

Attachment: Add a visible count to nsIImadeLoadingContent, and use it for locking the image.

Comment 101

[Timothy Nikkel (:tnikkel)]

Attachment: Keep a list of visible images on the presshell and code to manage it.

Comment 102

[Timothy Nikkel (:tnikkel)]

Attachment: Clear the list of visible images on presshells that we don't descend into while building the display list of visible images.

Comment 103

[Timothy Nikkel (:tnikkel)]

Attachment: Add mechanism to update image visibility upon some amount of scrolling.

Comment 104

[Timothy Nikkel (:tnikkel)]

Attachment: Add an 'unlocked draw' notification for images that are drawn when not locked so we catch any images that become visible through a means other than scrolling.

Comment 105

[Timothy Nikkel (:tnikkel)]

Attachment: Make images default to visible when they have a frame created for them.

Comment 106

[Timothy Nikkel (:tnikkel)]

Attachment: Make registering/unregistering an image for animation consistent across FrameCreated/Destroy, BindToTree/UnbindFromTree, and Track/UntrackImage.

Comment 107

[Timothy Nikkel (:tnikkel)]

Attachment: Make all images visible in print, print preview, chrome, xul, and resource docs and don't try to keep track of which are visible or not.

Comment 108

[Timothy Nikkel (:tnikkel)]

Attachment: Mark SVG feImage elements as visible because they don't create display items.

Comment 109

[Joe Drew (not getting mail)]

Comment on attachment 685999 [details] [diff] [review]
Add a visible count to nsIImadeLoadingContent, and use it for locking the image.

Review of attachment 685999 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsImageLoadingContent.cpp
@@ +420,5 @@
>                                            mPendingRequest,
>                                            &mPendingRequestRegistered);
>    }
>  
> +  nsIDocument* doc = GetOurCurrentDoc();

I guess we can't assert anything useful about our visibility count here, since it's just a frame being destroyed, and this content might still be visible.

@@ +424,5 @@
> +  nsIDocument* doc = GetOurCurrentDoc();
> +  if (doc) {
> +    if (mCurrentRequest && (mCurrentRequestFlags & REQUEST_IS_TRACKED)) {
> +      NS_ASSERTION(mCurrentRequestFlags & REQUEST_SHOULD_BE_TRACKED,
> +        "image is tracked but shouldn't be?");

indentation

@@ +430,5 @@
>        doc->RemoveImage(mCurrentRequest);
>      }
> +    if (mPendingRequest && (mPendingRequestFlags & REQUEST_IS_TRACKED)) {
> +      NS_ASSERTION(mPendingRequestFlags & REQUEST_SHOULD_BE_TRACKED,
> +        "image is tracked but shouldn't be?");

indentation

@@ +573,5 @@
> +nsImageLoadingContent::IncrementVisibleCount()
> +{
> +  mVisibleCount++;
> +  if (mVisibleCount == 1) {
> +    nsIDocument* doc = GetOurCurrentDoc();

Can you factor out the content of this if body into a helper function, and then use it here and in FrameCreated()?

@@ +604,5 @@
> +
> +void
> +nsImageLoadingContent::DecrementVisibleCount()
> +{
> +  NS_ASSERTION(mVisibleCount > 0, "visible count going negative?");

mVisibleCount is unsigned, so this can't actually happen.

@@ +625,5 @@
> +    nsIDocument* doc = GetOurCurrentDoc();
> +    if (doc) {
> +      if (mCurrentRequest && (mCurrentRequestFlags & REQUEST_IS_TRACKED)) {
> +        NS_ASSERTION(mCurrentRequestFlags & REQUEST_SHOULD_BE_TRACKED,
> +          "image is tracked but shouldn't be?");

indentation

@@ +631,5 @@
> +        doc->RemoveImage(mCurrentRequest);
> +      }
> +      if (mPendingRequest && (mPendingRequestFlags & REQUEST_IS_TRACKED)) {
> +        NS_ASSERTION(mPendingRequestFlags & REQUEST_SHOULD_BE_TRACKED,
> +          "image is tracked but shouldn't be?");

indentation

@@ +1210,5 @@
> +  if (mVisibleCount > 0 && GetOurPrimaryFrame()) {
> +    // Push a null JSContext on the stack so that callbacks triggered by the
> +    // below code won't think they're being called from JS.
> +    nsCxPusher pusher;
> +    pusher.PushNull();

Can aDocument->BlockOnload() cause callbacks to be called? In that case, this bit needs to be above this if.

Comment 110

[Joe Drew (not getting mail)]

Comment on attachment 686007 [details] [diff] [review]
Add an 'unlocked draw' notification for images that are drawn when not locked so we catch any images that become visible through a means other than scrolling.

Review of attachment 686007 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +3031,5 @@
> +  // that case we use our animation consumers count as a proxy for lock count.
> +  if (mLockCount == 0 || (mAnim && mAnimationConsumers == 0)) {
> +    nsCOMPtr<imgIDecoderObserver> observer(do_QueryReferent(mObserver));
> +    if (observer)
> +      observer->OnUnlockedDraw();

Calling a callback synchronously from a draw function makes me really, really concerned that we're going to hose ourselves a long time from now in ways we can't predict.

However, *we already do exactly that* when we call SyncDecode(), so I won't r- based on it.

::: image/src/imgRequestProxy.h
@@ +26,5 @@
> +{ /* dd292ddb-ed20-4f07-8f1e-6898205665b2 */         \
> +     0xdd292ddb,                                     \
> +     0xed20,                                         \
> +     0x4f07,                                         \
> +    {0x8f, 0x1e, 0x68, 0x98, 0x20, 0x56, 0x65, 0xb2} \

You don't need to change the class ID here, since it's the same class, just changing to track an interface whose IID has already changed.

::: layout/base/nsPresShell.cpp
@@ +5363,5 @@
> +  if (content) {
> +    PresShell* shell = static_cast<PresShell*>(content->OwnerDoc()->GetShell());
> +    NS_ASSERTION(!shell || shell == this, "wrong shell");
> +  }
> +#endif  

remove this extra whitespace please

@@ +5369,5 @@
> +  if (mImagesVisible.Contains(aImage)) { //xxx this could be slow
> +    return;
> +  }
> +
> +  mImagesVisible.AppendElement(aImage);

You could do InsertElementSorted() if you were really concerned about slowness above!

Comment 111

[Mats Palmgren (inactive)]

Comment on attachment 686001 [details] [diff] [review]
Keep a list of visible images on the presshell and code to manage it.

>layout/base/nsIPresShell.h
>+  virtual void ScheduleImageVisibilityUpdate() = 0;
>+
>+  virtual void ClearListAndMarkImagesInListVisible(const nsDisplayList& aList) = 0;

Please write doc comments for the added methods.
ClearListAndMarkImagesInListVisible is a mouthful, perhaps
shorten it to RebuildImageVisibility or something?

>layout/base/nsPresShell.cpp
>+  mUpdateImageVisibilityEvent.Revoke();
>+  mUpdateImageVisibilityEvent.Forget();

The Forget() is redundant, please remove. (this occurs in a few places)

>PresShell::UnsuppressAndInvalidate()
>   if (!mHaveShutDown)
>     SynthesizeMouseMove(false);
>+
>+  ScheduleImageVisibilityUpdate();

Move it inside the "if (!mHaveShutDown)" ?

>+PresShell::MarkImagesInListVisible(const nsDisplayList& aList)
>+    nsIFrame* f = item->GetUnderlyingFrame();
>+    // We could check the type of the display item, only a handful can hold an
>+    // image loading content.
>+    if (f) {
>+      // dont bother nscomptr here, it is wasteful
>+      nsCOMPtr<nsIImageLoadingContent> content(do_QueryInterface(f->GetContent()));
>+      if (content) {

Hmm, the assumption here is that all nsIImageLoadingContent is the content of
the frame of the display item?  It seems very limited.
For example, how about the anon content (mPosterImage) that nsVideoFrame
creates?


>+PresShell::ClearListAndMarkImagesInListVisible(const nsDisplayList& aList)
>+  beforeimagelist.Clear();

This seems redundant since it goes out of scope.

>+PresShell::UpdateImageVisibility()
>+{
>+  NS_ASSERTION(!mPresContext || mPresContext->IsRootContentDocument(), "xxx");

Use MOZ_ASSERT (everywhere). Remove "xxx".

>+  if (mHaveShutDown || mIsDestroying) {
>+    ClearVisibleImagesList();
>+    return;

Didn't Destroy already clear the list?

>+  nsIFrame* rootFrame = GetRootFrame();
>+  if (!rootFrame) {
>+    ClearVisibleImagesList();
>+    return;

How can there be anything in the list if we have no root frame?

>+  BuildListOfImages(rootFrame);

Perhaps it would be better to just inline BuildListOfImages here,
and call ClearVisibleImagesList at the top of UpdateImageVisibility.

>layout/base/nsPresShell.h
>+  class UpdateImageVisibilityEvent : public nsRunnable {

This class looks like unnecessary boiler plate.  Please use
nsRevocableEventPtr<nsRunnableMethod<PresShell>> instead - see mResizeEvent.

>+  // A list of images that are visible or almost visible.
>+  nsTArray< nsCOMPtr<nsIImageLoadingContent > > mImagesVisible;

I think mVisibleImages is easier to read.

>layout/generic/nsSubDocumentFrame.cpp
>+  if (aBuilder->IsForImageVisibility()) {

Just noting that IsForImageVisibility is defined in a later patch.
I don't mind though, so don't bother moving it unless you want to.

Comment 112

[Mats Palmgren (inactive)]

Comment on attachment 686003 [details] [diff] [review]
Clear the list of visible images on presshells that we don't descend into while building the display list of visible images.

It's less repetition to write:

   if (NS_SUCCEEDED(rv)) {
      ClearListAndMarkImagesInListVisible(list);
   } else {
      ClearVisibleImagesList();
   }
   ClearImageVisibilityVisited(aRootFrame->GetView(), true);
   list.DeleteAll();

(you can skip the 'else'-part if you merge this into UpdateImageVisibility
 as suggested earlier)

Please assert !mImageVisibilityVisited on entry to
ClearListAndMarkImagesInListVisible(), and also in some
unrelated method like PresShell::Paint().

Comment 113

[Mats Palmgren (inactive)]

Comment on attachment 686006 [details] [diff] [review]
Add mechanism to update image visibility upon some amount of scrolling.

>layout/generic/nsGfxScrollFrame.cpp
>+      horzScrollFraction =
>+        Preferences::GetInt("layout.imagevisibility.amountscrollbeforeupdatehorizontal", 2);

This requires a restart to change the value, right?
Please use AddIntVarCache instead. (ditto for vertical)
(don't forget to add std::max(horzScrollFraction, 1) when using it though)

>+      horzScrollFraction = PR_MAX(horzScrollFraction, 1);

std::max (multiple occurrences)

>+    nsPoint dist(NS_ABS(pt.x - mLastUpdateImagesPos.x),
>+                 NS_ABS(pt.y - mLastUpdateImagesPos.y));

abs

>+    nscoord horzAllowance = PR_MAX(mScrollPort.width/horzScrollFraction,
>+                                   nsPresContext::AppUnitsPerCSSPixel());

Please add spaces around / operator.  (multiple occurrences)

>+    if (dist.x >= horzAllowance || dist.y >= vertAllowance) {

Perhaps the code would be clearer with:
  bool needImageVisibilityUpdate;
that would be set for "mLastUpdateImagesPos == nsPoint(-1,-1)" or the
expression above.  Followed by a single:
  if (needImageVisibilityUpdate) {
    presContext->PresShell()->ScheduleImageVisibilityUpdate();
  }

>nsGfxScrollFrameInner::BuildDisplayList(
>+      horzExpandScrollPort =
>+        Preferences::GetUint("layout.imagevisibility.numscrollportwidths", (uint32_t)0);

Remove the 2nd arg, zero is the default.
Use AddUintVarCache instead.

>+    dirtyRect = dirtyRect.Union(
>+      dirtyRectBefore - nsPoint(0, vertExpandScrollPort*dirtyRectBefore.height));

Spaces around * operator please.

>+    dirtyRect = dirtyRect.Union(
>+      dirtyRectBefore + nsPoint(0, vertExpandScrollPort*dirtyRectBefore.height));

Please declare a local for the nsPoint value and use it for both -/+,
it should make the code easier to read with less lines.
(ditto for horizontal)

Comment 114

[Mats Palmgren (inactive)]

Comment on attachment 686007 [details] [diff] [review]
Add an 'unlocked draw' notification for images that are drawn when not locked so we catch any images that become visible through a means other than scrolling.

Ah, so EnsureImageInVisibleList covers the anon content image loads?

Comment 115

[Mats Palmgren (inactive)]

Comment on attachment 686008 [details] [diff] [review]
Make images default to visible when they have a frame created for them.

> nsImageLoadingContent::FrameCreated(nsIFrame* aFrame)
>+  nsPresContext* presContext = aFrame->PresContext();
>+  if (presContext) {
>+    nsIPresShell* presShell = presContext->PresShell();
>+    if (presShell) {
>+      presShell->EnsureImageInVisibleList(this);
>+    }
>+  }

I don't think you need any null-checks there.

Comment 116

[Mats Palmgren (inactive)]

Comment on attachment 686010 [details] [diff] [review]
Make all images visible in print, print preview, chrome, xul, and resource docs and don't try to keep track of which are visible or not.

I think it would be good idea to consolidate these tests into one
function so they don't become out-of-sync.

Comment 117

[Mats Palmgren (inactive)]

Comment on attachment 686011 [details] [diff] [review]
Mark SVG feImage elements as visible because they don't create display items.

>     imageLoader->FrameCreated(this);
>+    imageLoader->IncrementVisibleCount();

Add a comment that this may increment the count twice and it's
intentional since we need to guarantee it's incremented at
least once.  Or, you could check the count before/after
FrameCreated and only increment it in case FrameCreated
didn't?

Comment 118

[Mats Palmgren (inactive)]

> >+    nsPoint dist(NS_ABS(pt.x - mLastUpdateImagesPos.x),
> >+                 NS_ABS(pt.y - mLastUpdateImagesPos.y));
> 
> abs

Actually, that depends on the underlying nscoord type, so disregard
my comment.

Comment 119

[Mats Palmgren (inactive)]

I've now removed NS_ABS from the tree in bug 817574 - please use std::abs instead.
For example:
https://hg.mozilla.org/comm-central/diff/eafbbc84bd3d/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp

Comment 120

[(no longer active)]

(In reply to comment #119)
> I've now removed NS_ABS from the tree in bug 817574 - please use std::abs
> instead.

Perhaps this is worth a post to dev.platform?

Comment 121

[Timothy Nikkel (:tnikkel)]

If I don't reply to something that means I made the change and there was nothing more to say.

(In reply to Joe Drew (:JOEDREW! \o/) from comment #109)
> I guess we can't assert anything useful about our visibility count here,
> since it's just a frame being destroyed, and this content might still be
> visible.

Yeah, I guess not, we could just be doing the destroy half of recreating the frame at this point.

> Can you factor out the content of this if body into a helper function, and
> then use it here and in FrameCreated()?

Done. Did the same for DecrementVisibleCount and FrameDestroyed.

> > +  NS_ASSERTION(mVisibleCount > 0, "visible count going negative?");
> 
> mVisibleCount is unsigned, so this can't actually happen.

I changed the wording of the assertion.

> > +  if (mVisibleCount > 0 && GetOurPrimaryFrame()) {
> > +    // Push a null JSContext on the stack so that callbacks triggered by the
> > +    // below code won't think they're being called from JS.
> > +    nsCxPusher pusher;
> > +    pusher.PushNull();
> 
> Can aDocument->BlockOnload() cause callbacks to be called? In that case,
> this bit needs to be above this if.

The PushNull bits were added before there was a BlockOnload call, so I'm not sure. I'll just move it out to be safe.

Comment 122

[Timothy Nikkel (:tnikkel)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #110)
> Calling a callback synchronously from a draw function makes me really,
> really concerned that we're going to hose ourselves a long time from now in
> ways we can't predict.
> 
> However, *we already do exactly that* when we call SyncDecode(), so I won't
> r- based on it.

Good point.

> @@ +5369,5 @@
> > +  if (mImagesVisible.Contains(aImage)) { //xxx this could be slow
> > +    return;
> > +  }
> > +
> > +  mImagesVisible.AppendElement(aImage);
> 
> You could do InsertElementSorted() if you were really concerned about
> slowness above!

Don't worry, I remove the mImagesVisible.Contains call in the very next patch.

Comment 123

[Timothy Nikkel (:tnikkel)]

If I didn't reply to something that means I made that change.

(In reply to Mats Palmgren [:mats] from comment #111)
> >+PresShell::MarkImagesInListVisible(const nsDisplayList& aList)
> >+    nsIFrame* f = item->GetUnderlyingFrame();
> >+    // We could check the type of the display item, only a handful can hold an
> >+    // image loading content.
> >+    if (f) {
> >+      // dont bother nscomptr here, it is wasteful
> >+      nsCOMPtr<nsIImageLoadingContent> content(do_QueryInterface(f->GetContent()));
> >+      if (content) {
> 
> Hmm, the assumption here is that all nsIImageLoadingContent is the content of
> the frame of the display item?  It seems very limited.
> For example, how about the anon content (mPosterImage) that nsVideoFrame
> creates?

For poster images specifically we create a display item for the poster image here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsVideoFrame.cpp#393

But more generally anon content is only special in the content tree. In the frame tree anon content creates regular frames and regular display items, they aren't hidden children in the frame tree of display list.

And more generally again, if the image doesn't get an item in the display list, how can it ever be drawn to the screen? (Ok, there is at least one exception to this, SVG feImage frames, because the element that uses the filter has a display item whose paint method gets the feImage.)

> >+  if (mHaveShutDown || mIsDestroying) {
> >+    ClearVisibleImagesList();
> >+    return;
> 
> Didn't Destroy already clear the list?

Extra safety I guess?

> >+  nsIFrame* rootFrame = GetRootFrame();
> >+  if (!rootFrame) {
> >+    ClearVisibleImagesList();
> >+    return;
> 
> How can there be anything in the list if we have no root frame?

If we had a root frame and destroyed it maybe? If that situation arises then clearing the list is the right thing to do.

> >+  BuildListOfImages(rootFrame);
> 
> Perhaps it would be better to just inline BuildListOfImages here,
> and call ClearVisibleImagesList at the top of UpdateImageVisibility.

Ok, I moved the body of BuildListOfImages into UpdateImageVisibility, but we can't call ClearVisibleImagesList at the start of the function: we need to keep the visible count on the images positive until after we get the new list of visible images so we don't have any time when the visible count of a visible image is 0 so it doesn't get discarded.

Comment 124

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #112)
> It's less repetition to write:
> 
>    if (NS_SUCCEEDED(rv)) {
>       ClearListAndMarkImagesInListVisible(list);
>    } else {
>       ClearVisibleImagesList();
>    }
>    ClearImageVisibilityVisited(aRootFrame->GetView(), true);
>    list.DeleteAll();
> 
> (you can skip the 'else'-part if you merge this into UpdateImageVisibility
>  as suggested earlier)

I actually think it's clearer how it is currently. We handle the unlikely error condition and return and then we can handle the usual normal case. But I think we plan to eventually remove this nsresult return values from display list things as they can only fail on malloc failures, so when that happens this code will be simpler.

Comment 125

[Timothy Nikkel (:tnikkel)]

Attachment: Make all images visible in print, print preview, chrome, xul, and resource docs and don't try to keep track of which are visible or not. v2

Factored out the common code to a helper function.

Comment 126

[Timothy Nikkel (:tnikkel)]

Attachment: Disable image visibility pass on mobile and b2g.

We would like to reuse the painting display list on mobile for image visibility. This is possible on mobile because we have a displayport defined that is larger than what is currently visible for which we paint its contents. This code is not written yet and will be left for a followup, so disable this extra display list construction for now on mobile and b2g.

Comment 127

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #117)
> Mark SVG feImage elements as visible because they don't create display items.
> 
> >     imageLoader->FrameCreated(this);
> >+    imageLoader->IncrementVisibleCount();
> 
> Add a comment that this may increment the count twice and it's
> intentional since we need to guarantee it's incremented at
> least once.  Or, you could check the count before/after
> FrameCreated and only increment it in case FrameCreated
> didn't?

FrameCreated will call EnsureImageInVisibleList, but that isn't good enough since the next time we update image visibility information is will mark it as not visible because it won't find it in the display list. So we do need the separate IncrementVisibleCount call here no matter what.

Comment 128

[Leandro]

Tested [e-mail]-9511324c014e/try-win32 version

Wow firefox memory consume reduced 70% but,
process stills slows after loaded a certain quantity of images.

Nice works :tn !!

Comment 129

[Timothy Nikkel (:tnikkel)]

https://tbpl.mozilla.org/?tree=Try&rev=9511324c014e

Comment 130

[Timothy Nikkel (:tnikkel)]

(In reply to Leandro from comment #128)
> Tested [e-mail]-9511324c014e/try-win32 version
> 
> Wow firefox memory consume reduced 70% but,
> process stills slows after loaded a certain quantity of images.

Thanks for testing. If you want to file bugs for the remaining issues you see after this lands that would be great.

Comment 131

[Leandro]

I opened a report about this: bug 820113 

Your patch is a partial solution to the problem that I reported.

It is a major breakthrough, but the problems is: 
Why firefox process "slows/lag" after rendering lot of pictures?

Comment 132

[Mats Palmgren (inactive)]

Comment on attachment 693179 [details] [diff] [review]
Disable image visibility pass on mobile and b2g.

r=mats

Comment 133

[Timothy Nikkel (:tnikkel)]

Attachment: Add mechanism to update image visibility upon some amount of scrolling.

Oops, forgot that you wanted to look at this again and just noticed now. Here is the new patch I think I addressed all your comments.

Comment 134

[Timothy Nikkel (:tnikkel)]

The changes that I made in review in bug 784591 (which these patches are based on) made me refactor "Add a visible count to nsIImadeLoadingContent, and use it for locking the image." patch into 3 patches. So I'm going to ask for review on those.

Also I decided that I think we should leave alone the current RegisterImageRequest behaviour (where we register animated images with the refresh driver). So that we register/deregister when creating/destroying the frame, or when we get notified that the image request is animated. We can make having a non-zero visible count a requirement for registering an image request with the refresh driver at a later date if we determine it useful. So that means that the "Make registering/unregistering an image for animation consistent across FrameCreated/Destroy, BindToTree/UnbindFromTree, and Track/UntrackImage." patch will not be landed.

Comment 135

[Timothy Nikkel (:tnikkel)]

Attachment: In nsImageLoadingContent make the discard request an optional part of UntrackImage so that we can use UntrackImage everywhere instead of RemoveImage.

Comment 136

[Timothy Nikkel (:tnikkel)]

Attachment: In nsImageLoadingContent add a flag to TrackImage that skips the check if we have a frame so that we can use TrackImage everywhere (including FrameCreated) instead of AddImage.

Comment 137

[Timothy Nikkel (:tnikkel)]

Attachment: Add a visible count to nsIImadeLoadingContent, and use it when locking the image.

This is a lot simpler now.

Comment 138

[Joe Drew (not getting mail)]

Comment on attachment 703792 [details] [diff] [review]
In nsImageLoadingContent make the discard request an optional part of UntrackImage so that we can use UntrackImage everywhere instead of RemoveImage.

Review of attachment 703792 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsImageLoadingContent.cpp
@@ +1232,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +nsImageLoadingContent::UntrackImage(imgIRequest* aImage, uint32_t aFlags)

Can you do "aFlags /* = 0 */" here to emphasize that it has a default value?

@@ +1248,5 @@
>    if (doc) {
>      if (aImage == mCurrentRequest && (mCurrentRequestFlags & REQUEST_IS_TRACKED)) {
>        mCurrentRequestFlags &= ~REQUEST_IS_TRACKED;
> +      doc->RemoveImage(mCurrentRequest,
> +        (aFlags & REQUEST_DISCARD) ? nsIDocument::REQUEST_DISCARD : 0);

vertical alignment

@@ +1253,5 @@
>      }
>      if (aImage == mPendingRequest && (mPendingRequestFlags & REQUEST_IS_TRACKED)) {
>        mPendingRequestFlags &= ~REQUEST_IS_TRACKED;
> +      doc->RemoveImage(mPendingRequest,
> +        (aFlags & REQUEST_DISCARD) ? nsIDocument::REQUEST_DISCARD : 0);

vertical alignment

::: content/base/src/nsImageLoadingContent.h
@@ +330,5 @@
>    /**
>     * Adds/Removes a given imgIRequest from our document's tracker.
>     *
>     * No-op if aImage is null.
> +   * 

whitespace

@@ +337,5 @@
>     */
>    nsresult TrackImage(imgIRequest* aImage);
> +  enum {
> +    REQUEST_DISCARD = 0x1
> +  };

Can you make this a typesafe enum, with NONE as the default value? (Instead of flags.)

Comment 139

[Joe Drew (not getting mail)]

Comment on attachment 703794 [details] [diff] [review]
In nsImageLoadingContent add a flag to TrackImage that skips the check if we have a frame so that we can use TrackImage everywhere (including FrameCreated) instead of AddImage.

Review of attachment 703794 [details] [diff] [review]:
-----------------------------------------------------------------

Er, I'm not clear on which of this and the previous patch comes first, because they clearly both can't apply?

Comment 140

[Timothy Nikkel (:tnikkel)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #139)
> Er, I'm not clear on which of this and the previous patch comes first,
> because they clearly both can't apply?

Sorry, they apply in the order they are posted to this bug. They don't seem to have any problem applying this way in my patch queue.

Comment 141

[Timothy Nikkel (:tnikkel)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #138)
> @@ +337,5 @@
> >     */
> >    nsresult TrackImage(imgIRequest* aImage);
> > +  enum {
> > +    REQUEST_DISCARD = 0x1
> > +  };
> 
> Can you make this a typesafe enum, with NONE as the default value? (Instead
> of flags.)

I can but I think flags is the right idiom for this. I could see adding another flag that is independent from REQUEST_DISCARD, that we might want to specify both with and without REQUEST_DISCARD.

Comment 142

[Joe Drew (not getting mail)]

Comment on attachment 703794 [details] [diff] [review]
In nsImageLoadingContent add a flag to TrackImage that skips the check if we have a frame so that we can use TrackImage everywhere (including FrameCreated) instead of AddImage.

Review of attachment 703794 [details] [diff] [review]:
-----------------------------------------------------------------

OK then.

Comment 143

[Mats Palmgren (inactive)]

Comment on attachment 703791 [details] [diff] [review]
Add mechanism to update image visibility upon some amount of scrolling.

It looks like you've lost the change from nsDisplayListBuilder::OTHER to
nsDisplayListBuilder::IMAGE_VISIBILITY in PresShell::BuildListOfImages.

In nsGfxScrollFrameInner::ScrollToImpl:
> +  if (needImageVisibilityUpdate) {
> +    //presContext->PresShell()->ScheduleImageVisibilityUpdate();
> +  }

I assume you didn't mean to comment that out.

Looks good otherwise, r=mats with the above addressed.

Comment 145

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #143)
> It looks like you've lost the change from nsDisplayListBuilder::OTHER to
> nsDisplayListBuilder::IMAGE_VISIBILITY in PresShell::BuildListOfImages.
> 
> In nsGfxScrollFrameInner::ScrollToImpl:
> > +  if (needImageVisibilityUpdate) {
> > +    //presContext->PresShell()->ScheduleImageVisibilityUpdate();
> > +  }
> 
> I assume you didn't mean to comment that out.
> 
> Looks good otherwise, r=mats with the above addressed.

Thanks for picking up on that. I must have gotten confused when trying to make each patch build individually.

Comment 146

[Nicholas Nethercote [inactive]]

Just to clarify the relationship between this bug and bug 542158:

- When you switch to an already loaded page in a tab, this bug's patches will cause only the "visible" images to be re-decoded.

- In contrast, when you load a page for the first time, all the images in the page will still be decoded, even after this bug's patches have landed.  More work needs to be done to lazify the decoding in that case, and that will happen in bug 542158.

Comment 147

[Timothy Nikkel (:tnikkel)]

Attachment: Assume all images in popups are visible.

The display list we build to find images isn't going to find images in popups.

I don't think we need to worry about adding or removing the popup state bit dynamically, any change that moves a frame from popup to non-popup or vice versa will at least recreate the frame.

Comment 148

[Mats Palmgren (inactive)]

Comment on attachment 716395 [details] [diff] [review]
Assume all images in popups are visible.

r=mats

Comment 149

[IU]

(In reply to Timothy Nikkel (:tn) from comment #147)
> Created attachment 716395 [details] [diff] [review]
> Assume all images in popups are visible.

Shouldn't this be conditioned on the page length of the popup?

What if the popup basically contains a regular web page?

Comment 150

[Timothy Nikkel (:tnikkel)]

(In reply to IU from comment #149)
> (In reply to Timothy Nikkel (:tn) from comment #147)
> > Created attachment 716395 [details] [diff] [review]
> > Assume all images in popups are visible.
> 
> Shouldn't this be conditioned on the page length of the popup?
> 
> What if the popup basically contains a regular web page?

Popups here doesn't mean popup windows in the normal web vernacular. It means XUL panels and select dropdowns. Yes, the point of this patch is to preserve the current behaviour in this situation. I don't think a large amount of images in popups is a significant problem. If we find otherwise another solution can be implemented but I don't think it's warranted at this point.

Comment 151

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37f6664e8f1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7027a245b90a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca13b79b1e51
https://hg.mozilla.org/integration/mozilla-inbound/rev/9431c7190f20
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c39b261e724
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb776fb156d
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b37e1f4a78
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d424f250fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/42c64da3cdfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee05f33e1aa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/923d807d6ae5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4150df6216e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77277825dd5

Comment 152

[Justin Lebar (not reading bugmail)]

I have neither the words nor the emoticons to express how excited I am.

Comment 153

[timbugzilla]

(In reply to Justin Lebar [:jlebar] from comment #152)
> I have neither the words nor the emoticons to express how excited I am.

Here ya go. :)

http://i.imgur.com/3wNFXr1.gif

Comment 154

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37f6664e8f1e
https://hg.mozilla.org/mozilla-central/rev/7027a245b90a
https://hg.mozilla.org/mozilla-central/rev/ca13b79b1e51
https://hg.mozilla.org/mozilla-central/rev/9431c7190f20
https://hg.mozilla.org/mozilla-central/rev/0c39b261e724
https://hg.mozilla.org/mozilla-central/rev/2fb776fb156d
https://hg.mozilla.org/mozilla-central/rev/d7b37e1f4a78
https://hg.mozilla.org/mozilla-central/rev/f5d424f250fe
https://hg.mozilla.org/mozilla-central/rev/42c64da3cdfd
https://hg.mozilla.org/mozilla-central/rev/ee05f33e1aa2
https://hg.mozilla.org/mozilla-central/rev/923d807d6ae5
https://hg.mozilla.org/mozilla-central/rev/4150df6216e1
https://hg.mozilla.org/mozilla-central/rev/b77277825dd5

\o/

Comment 155

[Nicholas Nethercote [inactive]]

I've done some testing and things are looking good.  I had a long discussion with tn to clarify exactly what behaviours have changed.  Here is a high-level description that tn has checked.  (Note that comment 146 understated the improvements here.)

Old behaviour:

- When you load a page in a tab, every image in it is decoded as soon as it 
  finishes loading.  As long as the page remains in the foreground, none of the
  decoded images are discarded, no matter how you scroll within the page.

- When you switch to another tab, all decoded images from the first tab will be
  discarded after ~10 seconds.

- When your switch back to the first tab, all the images are immediately
  decoded again.  Again, as long as the page remains in the foreground, none of
  them are discarded, no matter how you scroll within the page.

New behaviour:

- The new visibility heuristics kick in shortly after you start loading a page 
  in a tab.  Any not-yet-satisfied decode requests for images that are
  sufficiently far from the viewport are canceled;  this saves both memory and 
  CPU time.  Furthermore, any images that were decoded before the heuristic
  kicked in but are sufficiently far from the viewport will be discarded after
  ~10 seconds.
  
- Once you start to scroll around the page, decoded images will be discarded
  (again after ~10 seconds) once they have moved sufficiently far from the
  viewport.  Undecoded images that move sufficiently close to the viewport will
  of course get decoded.

- When you switch to another tab, all decoded images from the first tab will be
  discarded after ~10 seconds.  (This behaviour is unchanged.)

- When you switch back to the first tab, only the images that are sufficiently
  close to the viewport are immediately decoded;  this saves both memory and
  CPU time.

So the peak memory consumption used by decoded images in the extreme case is 
unchanged -- it's still possible for all images in a large page to be decoded
(a) when the page first loads (though it may not happen, depending on the exact
timing of things) and (b) if you scroll through the whole page very quickly.
But in most cases memory consumption is likely to be much less.

Another consequence is that rapid scrolling through large pages is slightly
less smooth than before, because images have to be re-decoded.  This is a
reasonable trade-off for the greatly reduced memory consumption.

Comment 156

[Justin Lebar (not reading bugmail)]

tn can correct me if I'm wrong, but it sounds like if we set image.mem.max_decoded_image_kb to a smaller value (it's currently set "infinity" == 256mb), then that will place a soft limit on the extreme-case memory consumption caused by scrolling.  (It's still possible for a page to force us to have arbitrarily many decoded images in memory by sticking them all on one screen.)

We've previously encountered tricky problems with reducing the value in that pref.  In particular, we've run into cases where we decode and discard images in an infinite loop.  I think a prerequisite to reducing the pref is modifying the discard tracker to back off discarding images that were decoded recently.

> Another consequence is that rapid scrolling through large pages is slightly
> less smooth than before, because images have to be re-decoded.  This is a
> reasonable trade-off for the greatly reduced memory consumption.

Do you have a testcase for this that you've been playing with?  I don't think there's any technical reason that we must have additional jank in this case.

> When you switch to another tab, all decoded images from the first tab will be
> discarded after ~10 seconds.  (This behaviour is unchanged.)

I'd like us to look into changing this, now that we can.

Comment 157

[Nicholas Nethercote [inactive]]

> Do you have a testcase for this that you've been playing with? 

http://mpdrolet.tumblr.com/archive.  Scroll down for a while to load lots of images.

> > When you switch to another tab, all decoded images from the first tab will be
> > discarded after ~10 seconds.  (This behaviour is unchanged.)

Change it how?

Comment 158

[Justin Lebar (not reading bugmail)]

Thanks for the testcase.

This page has a bunch of small images, which we sync decode.  Perhaps we can avoid sync-decoding images which have been previously discarded.

> Change it how?

I was thinking that, "if we can" (waves hands), we'd try to save decoded images that are in-viewport in some bg tabs.

Comment 159

[Nicholas Nethercote [inactive]]

> This page has a bunch of small images, which we sync decode.  Perhaps we can
> avoid sync-decoding images which have been previously discarded.

Ah, so I guess a page with large images won't have the jerkiness, though maybe some of the images will be missing as they are scrolled past.

Comment 160

[Justin Lebar (not reading bugmail)]

> Ah, so I guess a page with large images won't have the jerkiness, though maybe some of 
> the images will be missing as they are scrolled past.

That is what I would expect.  I filed bug 845147 on what you reported here.

Comment 161

[Nicholas Nethercote [inactive]]

> tn can correct me if I'm wrong, but it sounds like if we set
> image.mem.max_decoded_image_kb to a smaller value (it's currently set
> "infinity" == 256mb), then that will place a soft limit on the extreme-case
> memory consumption caused by scrolling.  (It's still possible for a page to
> force us to have arbitrarily many decoded images in memory by sticking them
> all on one screen.)

That sounds right.  I've done some testing (see https://bugzilla.mozilla.org/show_bug.cgi?id=542158 comment 14) and the fast scroll peak was ~250 MiB of decoded images on two different pages that I tested.  Likewise for the initial page load peak.

Comment 162

[Justin Lebar (not reading bugmail)]

I already clarified this to njn, but for everyone else's benefit, it's probably better to think of the 256mb not as a limit, but instead as a way to short-circuit the discard timer.  Normally we discard unlocked images after ~10s.  But when we have 256mb of decoded images, we start discarding unlocked images immediately.

But under no circumstances do we discard locked images (unless you flip a pref, which I believe Android has, but B2G and desktop have not).  This bug makes fewer images locked, but you can still have more than 256mb of images stuck in memory if they're all on-screen.

Comment 163

[Timothy Nikkel (:tnikkel)]

(In reply to Justin Lebar [:jlebar] from comment #162)
> But under no circumstances do we discard locked images (unless you flip a
> pref, which I believe Android has, but B2G and desktop have not).

Android flips the pref, but I think we recently found out that due to a bug the pref has no effect.

Comment 164

[Justin Lebar (not reading bugmail)]

(In reply to Timothy Nikkel (:tn) from comment #163)
> (In reply to Justin Lebar [:jlebar] from comment #162)
> > But under no circumstances do we discard locked images (unless you flip a
> > pref, which I believe Android has, but B2G and desktop have not).
> 
> Android flips the pref, but I think we recently found out that due to a bug
> the pref has no effect.

Ah, that's right!

Comment 165

[ferongr]

I'm not sure if these patches handle this usecase, correct me if I'm wrong:

1: Long (in height) document loads, paints etc.
2:Some time later JS inserts images into the DOM, but most of them far away from the viewport, which are then loaded and inserted in the document.
3: Memory spikes and doesn't drop until the user scrolls.

Comment 166

[Asa Dotzler [:asa]]

(In reply to Nicholas Nethercote [:njn] from comment #155)

> Another consequence is that rapid scrolling through large pages is slightly
> less smooth than before, because images have to be re-decoded.  This is a
> reasonable trade-off for the greatly reduced memory consumption.

Why do we think this is a reasonable trade-off? How much jank does this introduce into page scrolling?

Comment 167

[IU]

(In reply to Asa Dotzler [:asa] from comment #166)
> Why do we think this is a reasonable trade-off? How much jank does this
> introduce into page scrolling?

Probably best to compare against Chrome, IE, and Opera, since they too do not decode images not in/near view.  If we're no worse than them, I would agree that the tradeoff is reasonable.

Comment 168

[Justin Lebar (not reading bugmail)]

> Why do we think this is a reasonable trade-off?

We believe this is a reasonable trade-off because this is one of the oldest, highest-priority MemShrink bugs, with countless dupes.  The benefit to users who browser image-heavy websites is very high, and we know based on telemetry and informal measures that this is a common use-case.  Many of the known issues fixed or made fixable by this bug result in crashes for users.

Nonetheless, I'd love it if you could help us find resources to fix bug 845147.  The root issue there has existed for a long time, and we have not had the resources to fix it.

> How much jank does this introduce into page scrolling?

Probably the easiest way to answer this is to try it yourself with a nightly before and after this change.  STR are in bug 845147.

Comment 169

[Timothy Nikkel (:tnikkel)]

(In reply to John Volikas from comment #165)
> I'm not sure if these patches handle this usecase, correct me if I'm wrong:
> 
> 1: Long (in height) document loads, paints etc.
> 2:Some time later JS inserts images into the DOM, but most of them far away
> from the viewport, which are then loaded and inserted in the document.
> 3: Memory spikes and doesn't drop until the user scrolls.

Is this causing a problem for you on a page?

Comment 170

[ferongr]

(In reply to Timothy Nikkel (:tn) from comment #169)
> (In reply to John Volikas from comment #165)
> > I'm not sure if these patches handle this usecase, correct me if I'm wrong:
> > 
> > 1: Long (in height) document loads, paints etc.
> > 2:Some time later JS inserts images into the DOM, but most of them far away
> > from the viewport, which are then loaded and inserted in the document.
> > 3: Memory spikes and doesn't drop until the user scrolls.
> 
> Is this causing a problem for you on a page?

Well, not really (I have enough memory and address space so Fx doesn't crash) but I thought I'd throw it out there. I'll probably create a testcase and file a bug blocking this one soon.

Comment 171

[Timothy Nikkel (:tnikkel)]

(In reply to John Volikas from comment #170)
> (In reply to Timothy Nikkel (:tn) from comment #169)
> > (In reply to John Volikas from comment #165)
> > > I'm not sure if these patches handle this usecase, correct me if I'm wrong:
> > > 
> > > 1: Long (in height) document loads, paints etc.
> > > 2:Some time later JS inserts images into the DOM, but most of them far away
> > > from the viewport, which are then loaded and inserted in the document.
> > > 3: Memory spikes and doesn't drop until the user scrolls.
> > 
> > Is this causing a problem for you on a page?
> 
> Well, not really (I have enough memory and address space so Fx doesn't
> crash) but I thought I'd throw it out there. I'll probably create a testcase
> and file a bug blocking this one soon.

Ok, well that is the expected behaviour, so unless it shows up for real users on real web pages I would be inclined not to devote any time to it.

Comment 172

[ferongr]

To word myself accurately, this does happen on real pages (with userscripts that load images) but it's not a problem for me personally. But I digress.

Comment 173

[Nicholas Nethercote [inactive]]

> Why do we think this is a reasonable trade-off? How much jank does this
> introduce into page scrolling?

Bug 701625 comment 5 is illustrative:

> With the Nightly from yesterday, 
> http://congressoamericano.blogspot.com/p/fotos-do-congresso-americano-iii.html
> (which has hundreds of photos on it, displayed one at a time on a very long 
> page) took up huge amounts of memory and my whole browser dragged to a crawl.
> With today's Nightly, which I think is the first with bug 689623, the browser
> remains responsive.

Furthermore, the additional jank I see when scrolling at maximum speed through such pages is quite mild.

Comment 174

[Timothy Nikkel (:tnikkel)]

(In reply to John Volikas from comment #172)
> To word myself accurately, this does happen on real pages (with userscripts
> that load images) but it's not a problem for me personally. But I digress.

Yes it does happen, but it's a tradeoff, memory usage isn't the only thing to be concerned with. Those userscripts probably think those images are likely to be visible soon or they wouldn't be adding them to the document.

Comment 175

[Nicholas Nethercote [inactive]]

> > With the Nightly from yesterday, 
> > http://congressoamericano.blogspot.com/p/fotos-do-congresso-americano-iii.html
> > (which has hundreds of photos on it, displayed one at a time on a very long 
> > page) took up huge amounts of memory and my whole browser dragged to a crawl.
> > With today's Nightly, which I think is the first with bug 689623, the browser
> > remains responsive.
> 
> Furthermore, the additional jank I see when scrolling at maximum speed
> through such pages is quite mild.

I did some tests with this page on my Mac.  Before these patches, memory consumption would reach 3 GiB (resident) -- with 2.7 GiB of that being decoded images -- and stay there as long as the tab was in the foreground.

After these patches, memory consumption briefly peaks at 2.8 GiB while loading (bug 542158 is open to get that lower) and then settles down to around 600 MiB within a few seconds, and then around 450 MiB about 10 seconds after that.  After that, even when scrolling quickly through the page I couldn't get it above 700 MiB.  (I used 'top' to measure, rather than about:memory, because 'top' updates in real-time.)

And the scrolling was just as smooth as before, because these aren't small images -- the only difference was that some of the images flashing by were blank, but the visible images filled in instantly once I stopped scrolling.

My Mac has 8 GiB of RAM so I didn't see any noticeable change in performance in the two cases.  But for someone with 2 GiB or 4 GiB, the general performance in the new version would be vastly better because there would be much less paging, and paging kills performance because it involves touching the hard disk a lot, and disks are horrendously slow.

Furthermore, there's a good chance that this page would crash anyone on a Windows build, because Windows builds are 32-bit and so the virtual memory limit is 2 or 4 GiB (depending on the OS configuration).  This bug alone probably wouldn't change that -- we still get the memory spike on loading, albeit slightly reduced -- but this bug lays the groundwork for bug 542158 to improve things there.

Comment 176

[Mayank Bansal]

(In reply to John Volikas from comment #170)
> (In reply to Timothy Nikkel (:tn) from comment #169)
> > (In reply to John Volikas from comment #165)
> > > I'm not sure if these patches handle this usecase, correct me if I'm wrong:
> > > 
> > > 1: Long (in height) document loads, paints etc.
> > > 2:Some time later JS inserts images into the DOM, but most of them far away
> > > from the viewport, which are then loaded and inserted in the document.
> > > 3: Memory spikes and doesn't drop until the user scrolls.
> > 
> > Is this causing a problem for you on a page?
> 
> Well, not really (I have enough memory and address space so Fx doesn't
> crash) but I thought I'd throw it out there. I'll probably create a testcase
> and file a bug blocking this one soon.

A potential test case is in bug 845260 . Without this patch, memory use was 2.4GB , but fast scrolling was smooth. 
With this patch, memory use is 600MB, but fast vertical scrolling is janky. (similar to what happens in Chrome).

Comment 177

[Jim Jeffery not reading bug-mail 1/2/11]

reply to comment #175 
Link http://congressoamericano.blogspot.com/p/fotos-do-congresso-americano-iii.html

Page loaded just fine on latest m-c hourly build.  Win7 x64 32bit build
Max 2.97 gig
After scrolling down several images use dropped to 409 meg.  I'd say its all good :)

Comment 178

[Timothy Nikkel (:tnikkel)]

(In reply to mayankleoboy1 from comment #176)
> A potential test case is in bug 845260 . Without this patch, memory use was
> 2.4GB , but fast scrolling was smooth. 
> With this patch, memory use is 600MB, but fast vertical scrolling is janky.
> (similar to what happens in Chrome).

The page in that bug does not seem to be an example of the scenario that John outlined in comment 165.

Comment 179

[IU]

(In reply to Timothy Nikkel (:tn) from comment #178)
> The page in that bug does not seem to be an example of the scenario that
> John outlined in comment 165.

Wouldn't that scenario be covered by bug 542158?

Comment 180

[Florian Bender]

Do we remember the image size information? I. e. discarded images should be "shown" as an "empty" rectangle and the image (once decoded) can be painted asynchronously into the reserved space. Can we actually do the image decoding for un-discarding image data asynchronously, even if the initial display uses synchronous decoding? Does this make sense at all?

Comment 181

[Justin Lebar (not reading bugmail)]

> Can we actually do the image decoding for un-discarding image data asynchronously

That's actually what I'm trying to do in bug 845147!

Comment 182

[Timothy Nikkel (:tnikkel)]

(In reply to Florian Bender from comment #180)
> Do we remember the image size information? I. e. discarded images should be
> "shown" as an "empty" rectangle and the image (once decoded) can be painted
> asynchronously into the reserved space.

Yes, the size is not discarded.

Comment 183

[Paul Silaghi, QA [:pauly]]

Is there something QA can do here ?

Comment 184

[Paul Silaghi, QA [:pauly]]

I did some testing on this, 2 FF windows opened, 1 with about:memory and the other one with: http://o001oo.ru/index.php?showtopic=14210%26st=0 http://congressoamericano.blogspot.ro/p/fotos-do-congresso-americano-iii.html

Before the fix, images-content-used-uncompressed was ~ 1.5 - 2 GB
The second link even froze my entire computer (Win 7 x64, Intel Core 2 Duo, 4GB RAM), only after reboot the things got back to normal.
After the fix, on Aurora 22.0a2 (2013-04-03), images-content-used-uncompressed was < 200 MB, Win7 has no longer frozen.
Approximately the same values on Mac OS X 10.7.5.
On Ubuntu 12.04 LTS, images-content-used-uncompressed was small (~300MB) before the fix, remained small after the fix.
Scrolling works fine, no crashes occurred after the fix.

So I would say this bug is verified fixed, except the following observation:
I did manage 2 times to make the memory stay high (~1.7 - 2GB) after the fix.
Don't have a reliable STR for this, but I would say it's something about reloading and switching windows, refreshing about:memory while images in the other window are still loading. So the memory was stuck high, but right after a minimum scrolling in the images window, the memory decreased instantaneously.
Any thoughts on this?

Comment 185

[ferongr]

http://congressoamericano.blogspot.ro/p/fotos-do-congresso-americano-iii.html

Use the above URL for the STR

Images loaded from the network are never discarded regardless of their visibility status (clear web cache or do CTRL+F5 at the URL).

1. Follow the URL with an empty cache.
2. Wait for network and decoding to settle. No discarding happens.
3. Scroll 300+ pixels
4. Images are discarded as expected, with a small jank.

Expected: Only decode images that are needed (in viewport plus breathing space).

Images loaded from the web cache are decoded and then immediately discarded afterwards, resulting in a memory spike and jank.

1. Follow the URL with a primed cache.
2. All images start getting decoded and then once they are decoded they are immediately discarded. Some jank occurs.

Expected: Only decode images that are needed (in viewport plus breathing space).

Comment 186

[Timothy Nikkel (:tnikkel)]

(In reply to Paul Silaghi [QA] from comment #184)
> So I would say this bug is verified fixed, except the following observation:
> I did manage 2 times to make the memory stay high (~1.7 - 2GB) after the fix.
> Don't have a reliable STR for this, but I would say it's something about
> reloading and switching windows, refreshing about:memory while images in the
> other window are still loading. So the memory was stuck high, but right
> after a minimum scrolling in the images window, the memory decreased
> instantaneously.
> Any thoughts on this?

Thanks for testing.

That is expected actually. There is more work to be done here, bug 847223 tracks basically the problem you mention.

Comment 187

[Timothy Nikkel (:tnikkel)]

(In reply to John Volikas from comment #185)
> http://congressoamericano.blogspot.ro/p/fotos-do-congresso-americano-iii.html
> 
> Use the above URL for the STR
> 
> Images loaded from the network are never discarded regardless of their
> visibility status (clear web cache or do CTRL+F5 at the URL).

I think this is covered by bug 847223.

Comment 188

[Paul Silaghi, QA [:pauly]]

Verified fixed Aurora 22.0a2 (2013-04-03), Nightly 23.0a1 (2013-04-03) based on comments 186, 187.

Comment 189

[Alex Keybl [:akeybl]]

This bug has been listed as part of the Aurora 22 release notes in either [1], [2], or both. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!

[1] https://www.mozilla.org/en-US/firefox/22.0a2/auroranotes/
[2] https://www.mozilla.org/en-US/mobile/22.0a2/auroranotes/

Comment 190

[Matt Brubeck (:mbrubeck)]

Disabled for Firefox 22 in bug 872235; relnotes for 22 need adjustment.

Comment 191

[bhavana bajaj [:bajaj]]

(In reply to Matt Brubeck (:mbrubeck) [away until July 3] from comment #190)
> Disabled for Firefox 22 in bug 872235; relnotes for 22 need adjustment.