Last Comment Bug 847223 - Don't decode images that aren't visible when we download them
: Don't decode images that aren't visible when we download them
Status: VERIFIED FIXED
[MemShrink:P1]
: feature
Product: Core
Classification: Components
Component: Layout: Images (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla26
Assigned To: Timothy Nikkel (:tnikkel)
: Mihai Morar, (:MihaiMorar)
Mentors:
: 542158 (view as bug list)
Depends on: 851785 863658 865945 900662 919129
Blocks: 542158 689623 860818 915526
  Show dependency treegraph
 
Reported: 2013-03-03 15:18 PST by Justin Lebar (not reading bugmail)
Modified: 2013-11-15 21:43 PST (History)
37 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
26+


Attachments
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell. (5.42 KB, patch)
2013-04-27 15:16 PDT, Timothy Nikkel (:tnikkel)
mats: review-
Details | Diff | Splinter Review
Part 2. Add a function to remove an image from the visible list. (3.65 KB, patch)
2013-04-27 15:18 PDT, Timothy Nikkel (:tnikkel)
mats: review-
Details | Diff | Splinter Review
Part 3. Factor out the expand the scrollport code so we can use it on individual images later. (10.58 KB, patch)
2013-04-27 15:19 PDT, Timothy Nikkel (:tnikkel)
mats: review-
Details | Diff | Splinter Review
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll. (2.51 KB, patch)
2013-04-27 15:27 PDT, Timothy Nikkel (:tnikkel)
mats: review-
Details | Diff | Splinter Review
Part 5. Make AssumeAllImagesVisible usuable outside of PresShell. (5.67 KB, patch)
2013-04-27 15:28 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 6. Use the first reflow of relevant image frames to add/remove them from the visible list. (12.88 KB, patch)
2013-04-27 15:32 PDT, Timothy Nikkel (:tnikkel)
mats: review-
Details | Diff | Splinter Review
Part 7. Stop assuming all images are visible on frame create. (1.46 KB, patch)
2013-04-27 15:37 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 8. Use a bool to track if FrameCreate has been called. (4.95 KB, patch)
2013-05-07 00:34 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 9. Try to avoid asking for a decode of an image whose network request finishes before painting is unsuppressed. (1.59 KB, patch)
2013-05-07 00:38 PDT, Timothy Nikkel (:tnikkel)
mats: review+
joe: review+
Details | Diff | Splinter Review
Part 10. Make the new code work when we have the image visibility analysis prefed off. (2.66 KB, patch)
2013-09-11 18:30 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
Part 10. Make the new code work when we have the image visibility analysis prefed off. (4.92 KB, patch)
2013-09-11 19:43 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 2. Add a function to remove an image from the visible list. (3.82 KB, patch)
2013-09-11 23:24 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 3. Factor out the expand the scrollport code so we can use it on individual images later. (10.27 KB, patch)
2013-09-11 23:34 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 6. Use the first reflow of relevant image frames to add/remove them from the visible list. (13.09 KB, patch)
2013-09-11 23:36 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll. (2.67 KB, patch)
2013-09-13 10:11 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell. (5.10 KB, patch)
2013-09-13 10:44 PDT, Timothy Nikkel (:tnikkel)
mats: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2013-03-03 15:18:26 PST
Now that we've landed bug 689623, we can discard images that aren't onscreen.

But we apparently still decode images as soon as we download them.  Then we throw away this data some 20s later if the image is not onscreen.

What would be involved in fixing this?
Comment 1 Nicholas Nethercote [:njn] 2013-03-03 17:03:50 PST

*** This bug has been marked as a duplicate of bug 542158 ***
Comment 2 Timothy Nikkel (:tnikkel) 2013-03-03 17:09:59 PST
Actually I think this bug is nicely focused on one specific problem and bug 542158 is kind of a grab bag of things. So I think we should keep this open.
Comment 3 Justin Lebar (not reading bugmail) 2013-03-04 04:57:16 PST
tn, do you have an idea offhand what would be involved in fixing this?  Is it a matter of no longer assuming that all new frames are visible?
Comment 4 Timothy Nikkel (:tnikkel) 2013-03-04 08:13:41 PST
(In reply to Justin Lebar [:jlebar] from comment #3)
> tn, do you have an idea offhand what would be involved in fixing this?  Is
> it a matter of no longer assuming that all new frames are visible?

No, that won't be enough, sometimes new frames are visible.
Comment 5 Timothy Nikkel (:tnikkel) 2013-03-04 08:20:23 PST
Right now my thinking is we walk up the frame tree and determine if the frame is visible or close to visible via scrolling after the frame has position information (ie after its first reflow).
Comment 6 Timothy Nikkel (:tnikkel) 2013-04-27 15:16:47 PDT
Created attachment 742747 [details] [diff] [review]
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell.

We'll need this so we have a fast way to remove an image from the visible list.
Comment 7 Timothy Nikkel (:tnikkel) 2013-04-27 15:18:10 PDT
Created attachment 742748 [details] [diff] [review]
Part 2. Add a function to remove an image from the visible list.
Comment 8 Timothy Nikkel (:tnikkel) 2013-04-27 15:19:53 PDT
Created attachment 742749 [details] [diff] [review]
Part 3. Factor out the expand the scrollport code so we can use it on individual images later.
Comment 9 Timothy Nikkel (:tnikkel) 2013-04-27 15:27:45 PDT
Created attachment 742750 [details] [diff] [review]
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll.

This makes sense, but I think it has the largest impact on elements that are overflow: hidden that don't have any constraints on their size, so they aren't scrollable. Especially on such elements that are larger than the viewport where they can cause much more expansion than we want, especially if they are nested. And it seems such cases are quite common.
Comment 10 Timothy Nikkel (:tnikkel) 2013-04-27 15:28:48 PDT
Created attachment 742751 [details] [diff] [review]
Part 5. Make AssumeAllImagesVisible usuable outside of PresShell.

We will want to use it in other places later on.
Comment 11 Timothy Nikkel (:tnikkel) 2013-04-27 15:32:07 PDT
Created attachment 742752 [details] [diff] [review]
Part 6. Use the first reflow of relevant image frames to add/remove them from the visible list.

We only need to change image frames and svg image frames because image control frames use image frames reflow and svg fe image frames we always assume are visible.
Comment 12 Timothy Nikkel (:tnikkel) 2013-04-27 15:37:41 PDT
Created attachment 742753 [details] [diff] [review]
Part 7. Stop assuming all images are visible on frame create.

Now that we determine if the image is visible after it's first reflow we don't need to assume all images are visible when their frame is created.

This will slightly delay marking visible images as visible, but the only way we can hit the event loop after this patch before marking the image as visible is if we are doing a flush of frame construction but not doing reflow. But I think in most cases we doing a flush that involves reflow.
Comment 13 Timothy Nikkel (:tnikkel) 2013-04-27 15:45:13 PDT
With these patches (plus a couple more small patches that aren't quite ready for landing, and I don't think they would have an affect on the numbers) I compared loading the page
http://congressoamericano.blogspot.ca/p/fotos-do-congresso-americano-iii.html
Before these patches our decoded image data would peak at between 1.6GB and 2.7GB depending on the network and if you have things cached. With the patches it maxes out at 55MB (with about 40mb being discardable, and the only reason it isn't discarded is because we just haven't been able to serve the discard event due to the large amount of events floating around on the main thread for the networking events for all the images). The 55mb would be even smaller except the page doesn't provide sizes for the images so they are small and a lot more of them fit on a page until we get enough data to do a size decode on the image.
Comment 14 Mats Palmgren (vacation) 2013-04-27 16:29:20 PDT
Comment on attachment 742747 [details] [diff] [review]
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell.

>       PresShell* presShell = static_cast<PresShell*>(f->PresContext()->PresShell());
>-      if (presShell) {
>+      if (presShell && !presShell->mVisibleImages.Contains(content)) {
>         content->IncrementVisibleCount();
>-        presShell->mVisibleImages.AppendElement(content);
>+        presShell->mVisibleImages.PutEntry(content);

No need to null check a pres shell that comes from a frame.

Instead of Contains+PutEntry, would it be faster to save Count() then
PutEntry(), and then IncrementVisibleCount() only if Count() increased?

>+static PLDHashOperator
>+RemoveAndStore(nsRefPtrHashKey<nsIImageLoadingContent>* aEntry, void* userArg)
>+{
>+  nsTArray< nsRefPtr<nsIImageLoadingContent> >* array =
>+    static_cast< nsTArray< nsRefPtr<nsIImageLoadingContent> >* >(userArg);
>+  array->AppendElement(aEntry->GetKey());
>+  return PL_DHASH_REMOVE;
>+}

This looks a bit slow.  How about implementing nsTHashtable::SwapElements?
It seems trivial and could be very useful for things like this.
Comment 15 Mats Palmgren (vacation) 2013-04-27 16:42:35 PDT
Comment on attachment 742748 [details] [diff] [review]
Part 2. Add a function to remove an image from the visible list.

I think the #ifdef DEBUG block should be at the top.
This could also avoid Contains() using Count() checks, assuming it's faster.
Comment 16 Nicholas Nethercote [:njn] 2013-04-27 20:35:17 PDT
> Before these patches our decoded image data would peak at between 1.6GB and
> 2.7GB depending on the network and if you have things cached. With the
> patches it maxes out at 55MB

/me swoons
Comment 17 Mats Palmgren (vacation) 2013-04-28 04:33:39 PDT
Comment on attachment 742749 [details] [diff] [review]
Part 3. Factor out the expand the scrollport code so we can use it on individual images later.

I think the comments for the static members should go in the header.

>+nsGfxScrollFrameInner::ExpandRect(const nsRect& aRect)
>+{
>+  nsRect rect = aRect;
>+  nsPoint vertShift = nsPoint(0, sVertExpandScrollPort * aRect.height);
>+  rect = rect.Union(aRect - vertShift);
>+  rect = rect.Union(aRect + vertShift);
>+
>+  nsPoint horzShift = nsPoint(sHorzExpandScrollPort * aRect.width, 0);
>+  rect = rect.Union(aRect - horzShift);
>+  rect = rect.Union(aRect + horzShift);
>+  return rect;
>+}

Isn't rect.Inflate(horzShift,vertShift) a simpler equivalent to the four
Union calls?
Comment 18 Mats Palmgren (vacation) 2013-04-28 05:07:04 PDT
Comment on attachment 742750 [details] [diff] [review]
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll.

(For the sake of argument, let's assume s*ExpandScrollPort=1)

I have trouble understanding how ExpandRect() now makes any sense for
aRect != mScrollPort.  For example, ExpandRect(dirtyRect).
Let's say the scroll port is 0,0,100,100 and we're scrolled to 0,0
and the range is non-zero (i.e. we have something to scroll to).
For dirtyRect=5,5,10,10 ExpandRect() will now return 5,5,20,20.
Isn't the desired result 0,0,25,25 ?
With a scroll position of 1,1 the result is -5,-5,30,30.
Isn't the desired result 0,0,25,25 ?

I think something like this makes more sense:
  rect.Inflate(horzShift, vertShift);
  rect = rect.Intersect(GetScrolledRect());
but maybe I've misunderstood what you're trying to do with ExpandRect.
(it's undocumented so I have to guess)
Comment 19 Mats Palmgren (vacation) 2013-04-28 05:53:23 PDT
Comment on attachment 742752 [details] [diff] [review]
Part 6. Use the first reflow of relevant image frames to add/remove them from the visible list.

>layout/base/nsLayoutUtils.cpp
>+  MOZ_ASSERT(type == nsGkAtoms::imageFrame ||
>+             type == nsGkAtoms::imageControlFrame ||
>+             type == nsGkAtoms::svgImageFrame, "wrong type of frame");

Not sure if we need to limit this to specific frame types, but OK.
(I think any frame with content that QI's to nsIImageLoadingContent
should work fine, yes?)

>+  nsCOMPtr<nsIImageLoadingContent> content = do_QueryInterface...
>+  if (content) {

Please move this before "bool visible = true;" and do an early return
if !content to avoid calculating 'visible' which isn't going to be used.


>layout/generic/nsImageFrame.cpp
>+  if ((GetStateBits() & NS_FRAME_FIRST_REFLOW) && !mReflowCallbackPosted) {

Why do we only need to do this on the first reflow?
What if the frame has size 0,0 on the first reflow and that results in
visibility being false in UpdateImageVisibilityForFrame, then the frame
is restyled to have a large size -- how is its visibility updated?
Comment 20 Timothy Nikkel (:tnikkel) 2013-04-28 13:56:17 PDT
(In reply to Mats Palmgren [:mats] from comment #19)
> Why do we only need to do this on the first reflow?
> What if the frame has size 0,0 on the first reflow and that results in
> visibility being false in UpdateImageVisibilityForFrame, then the frame
> is restyled to have a large size -- how is its visibility updated?

Currently we update image visibility when paint suppression ends and when we've scrolled enough in some scrollable frame, this obviously doesn't handle all cases where an image can be made visible. It would be too hard to monitor all possible ways, so we just do that and then have the backup of marking images visible if they are actually Draw()'n (RasterImage::Draw()). That should cover everything.

During the loading of a page though this solution has drawbacks, the biggest being that we consider all images visible until we've had a chance to run an update image visibility event. To fix this I hooked the first reflow of images to get a good initial estimate of if the image is visible or not. If we are wrong then it's okay because we have a backup plan that should catch all cases.

And even if we did this on every reflow of an image frame we'd still be missing cases where an ancestor frame is moved to be visible but the image frame doesn't need to be reflowed.
Comment 21 Timothy Nikkel (:tnikkel) 2013-05-07 00:34:38 PDT
Created attachment 746264 [details] [diff] [review]
Part 8. Use a bool to track if FrameCreate has been called.

This let's us get rid of the SKIP_FRAME_CHECK hack.

But the real reason for this patch is that images get generic display-type frames (ie nsInlineFrame) if there isn't a usable image because we call nsImageFrame::ShouldCreateImageFrameFor before creating an image frame. For the next patch we need to know if we have a image type frame (one that calls FrameCreate and does the visible/not-visible thing on first reflow) or not.
Comment 22 Timothy Nikkel (:tnikkel) 2013-05-07 00:38:08 PDT
Created attachment 746266 [details] [diff] [review]
Part 9. Try to avoid asking for a decode of an image whose network request finishes before painting is unsuppressed.

nsImageLoadingContent::OnStopRequest will blindly ask for decodes for every image that finishes loading before painting is unsuppressed. But we can do better: if we have a frame and it has visibility information that we can trust we don't have to ask for a decode.
Comment 23 Timothy Nikkel (:tnikkel) 2013-05-07 00:47:23 PDT
(In reply to Mats Palmgren [:mats] from comment #18)
> Comment on attachment 742750 [details] [diff] [review]
> Part 4. Tweak the ExpandRect function to not expand in a direction that we
> cannot scroll.
> 
> (For the sake of argument, let's assume s*ExpandScrollPort=1)
> 
> I have trouble understanding how ExpandRect() now makes any sense for
> aRect != mScrollPort.  For example, ExpandRect(dirtyRect).
> Let's say the scroll port is 0,0,100,100 and we're scrolled to 0,0
> and the range is non-zero (i.e. we have something to scroll to).
> For dirtyRect=5,5,10,10 ExpandRect() will now return 5,5,20,20.
> Isn't the desired result 0,0,25,25 ?
> With a scroll position of 1,1 the result is -5,-5,30,30.
> Isn't the desired result 0,0,25,25 ?
> 
> I think something like this makes more sense:
>   rect.Inflate(horzShift, vertShift);
>   rect = rect.Intersect(GetScrolledRect());
> but maybe I've misunderstood what you're trying to do with ExpandRect.
> (it's undocumented so I have to guess)

You're right, ExpandRect doesn't have much underlying logic if the rect isn't the whole scrollport, and it isn't explained at all. I've mainly tried to avoid bad stuff (expanding too much) in the non-full scrollport case. I'll have a go at making it make sense while avoiding bad stuff.
Comment 24 Mats Palmgren (vacation) 2013-05-07 06:51:04 PDT
Comment on attachment 746264 [details] [diff] [review]
Part 8. Use a bool to track if FrameCreate has been called.

Makes sense.  r=mats

Nit:
> content/base/src/nsImageLoadingContent.h
I think it would be clearer to have separate comments for [Un]TrackImage.

BTW, can we make these return void?  They always return NS_OK afaict
and all consumers ignore the return value.
Comment 25 Timothy Nikkel (:tnikkel) 2013-05-07 08:15:14 PDT
(In reply to Mats Palmgren [:mats] from comment #24)
> BTW, can we make these return void?  They always return NS_OK afaict
> and all consumers ignore the return value.

I filed bug 869476 and posted a patch there.
Comment 26 Joe Drew (not getting mail) 2013-05-07 10:20:08 PDT
Comment on attachment 746266 [details] [diff] [review]
Part 9. Try to avoid asking for a decode of an image whose network request finishes before painting is unsuppressed.

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

::: content/base/src/nsImageLoadingContent.cpp
@@ +236,5 @@
>    if (shell && shell->IsVisible() &&
>        (!shell->DidInitialize() || shell->IsPaintingSuppressed())) {
>  
> +    // If we've gotten a frame and that frame has called FrameCreate and that
> +    // frame has been reflowed then we know that it checked it's own visibility

its

@@ +241,5 @@
> +    // so we can trust our visible count and we don't start decode if we are not
> +    // visible.
> +    nsIFrame* f = GetOurPrimaryFrame();
> +    if (!mFrameCreateCalled || !f ||
> +        (f->GetStateBits() & NS_FRAME_FIRST_REFLOW) || mVisibleCount > 0) {

I find this logic a little hard to follow. Maybe it would be easier if it was
if (!(mFrameCreateCalled && f && (f->GetStateBits() & NS_FRAME_FIRST_REFLOW) || mVisibleCount > 0)
  (start decoding)

I don't know; I don't feel strongly about it.
Comment 27 Mats Palmgren (vacation) 2013-05-16 12:18:57 PDT
Comment on attachment 742747 [details] [diff] [review]
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell.

r- per above comments (to clear up my review queue).
Comment 28 Nicholas Nethercote [:njn] 2013-08-20 16:29:44 PDT
tn, what's the status here?  Bug 900662 was fixed about three weeks ago... what's the next step?
Comment 29 Timothy Nikkel (:tnikkel) 2013-08-21 00:15:30 PDT
There are some other test failures, and I need to adequately address review comments. Which I am working on now.
Comment 30 Timothy Nikkel (:tnikkel) 2013-09-11 18:30:50 PDT
Created attachment 803441 [details] [diff] [review]
Part 10. Make the new code work when we have the image visibility analysis prefed off.

This was just a mistake in the earlier patches. If the image visibility pref is off we need to skip trying to determine if the image is visible and just assume it is visible.
Comment 31 Mats Palmgren (vacation) 2013-09-11 19:29:40 PDT
Comment on attachment 803441 [details] [diff] [review]
Part 10. Make the new code work when we have the image visibility analysis prefed off.

>content/base/src/nsImageLoadingContent.cpp
>-    if (!mFrameCreateCalled || !f ||
>+    if (shell->AssumeAllImagesVisible() || !mFrameCreateCalled || !f ||
>         (f->GetStateBits() & NS_FRAME_FIRST_REFLOW) || mVisibleCount > 0) {

Isn't mVisibleCount > 0 always true when AssumeAllImagesVisible() ?
(incremented in PresShell::EnsureImageInVisibleList)

>layout/generic/nsImageFrame.cpp
>@@ -894,17 +894,17 @@ nsImageFrame::Reflow(nsPresContext*     
>     nsIPresShell* shell = PresContext()->PresShell();
>-    if (shell && !shell->AssumeAllImagesVisible()) {
>+    if (shell) {

'shell'' can never be null here

>-  nsLayoutUtils::UpdateImageVisibilityForFrame(this);
>+  nsIPresShell* shell = PresContext()->PresShell();
>+  if (shell->AssumeAllImagesVisible()) {
>+    nsCOMPtr<nsIImageLoadingContent> content = do_QueryInterface(mContent);
>+    if (content) {
>+      shell->EnsureImageInVisibleList(content);
>+    }
>+  } else {
>+    nsLayoutUtils::UpdateImageVisibilityForFrame(this);
>+  }

Alternatively, you could add that block to UpdateImageVisibilityForFrame
instead.

Either way is fine, but if you keep it here then add an assertion to
UpdateImageVisibilityForFrame that AssumeAllImagesVisible() is false.

Doesn't nsSVGImageFrame::ReflowFinished() need the same change?
Comment 32 Timothy Nikkel (:tnikkel) 2013-09-11 19:35:08 PDT
(In reply to Mats Palmgren (:mats) from comment #31)
> >content/base/src/nsImageLoadingContent.cpp
> >-    if (!mFrameCreateCalled || !f ||
> >+    if (shell->AssumeAllImagesVisible() || !mFrameCreateCalled || !f ||
> >         (f->GetStateBits() & NS_FRAME_FIRST_REFLOW) || mVisibleCount > 0) {
> 
> Isn't mVisibleCount > 0 always true when AssumeAllImagesVisible() ?
> (incremented in PresShell::EnsureImageInVisibleList)

We have to call EnsureImageInVisibleList, and it may not have been called at this point.

> Either way is fine, but if you keep it here then add an assertion to
> UpdateImageVisibilityForFrame that AssumeAllImagesVisible() is false.

It's got one already. :)

> Doesn't nsSVGImageFrame::ReflowFinished() need the same change?

Good catch.
Comment 33 Timothy Nikkel (:tnikkel) 2013-09-11 19:43:33 PDT
Created attachment 803468 [details] [diff] [review]
Part 10. Make the new code work when we have the image visibility analysis prefed off.
Comment 34 Timothy Nikkel (:tnikkel) 2013-09-11 19:56:28 PDT
(In reply to Mats Palmgren (:mats) from comment #17)
> Isn't rect.Inflate(horzShift,vertShift) a simpler equivalent to the four
> Union calls?

I made this change. Although it required me to make more changes to part 4: I had to use a margin to inflate because we might not inflate uniformly on all sides.
Comment 35 Timothy Nikkel (:tnikkel) 2013-09-11 20:21:45 PDT
(In reply to Mats Palmgren (:mats) from comment #14)
> Instead of Contains+PutEntry, would it be faster to save Count() then
> PutEntry(), and then IncrementVisibleCount() only if Count() increased?

It doesn't look like the hashtable implementation caches the last lookup or anything to optimize this situation, so I guess this would save us one hashtable lookup. I made this change, and in the next patch in the series too.
Comment 36 Mats Palmgren (vacation) 2013-09-11 20:29:54 PDT
Comment on attachment 803468 [details] [diff] [review]
Part 10. Make the new code work when we have the image visibility analysis prefed off.

>content/base/src/nsImageLoadingContent.cpp
>-    if (!mFrameCreateCalled || !f ||
>+    if (shell->AssumeAllImagesVisible() || !mFrameCreateCalled || !f ||
>         (f->GetStateBits() & NS_FRAME_FIRST_REFLOW) || mVisibleCount > 0) {

AssumeAllImagesVisible() is false when this feature is enabled (except for
chrome etc, with relatively few images) so it seems it should be tested last.
And perhaps in most cases one of the other conditions are true anyway?

r=mats
Comment 37 Timothy Nikkel (:tnikkel) 2013-09-11 20:46:10 PDT
(In reply to Mats Palmgren (:mats) from comment #36)
> AssumeAllImagesVisible() is false when this feature is enabled (except for
> chrome etc, with relatively few images) so it seems it should be tested last.
> And perhaps in most cases one of the other conditions are true anyway?

Ok, moved it to the end.
Comment 38 Timothy Nikkel (:tnikkel) 2013-09-11 23:24:20 PDT
Created attachment 803519 [details] [diff] [review]
Part 2. Add a function to remove an image from the visible list.
Comment 39 Timothy Nikkel (:tnikkel) 2013-09-11 23:34:48 PDT
Created attachment 803522 [details] [diff] [review]
Part 3. Factor out the expand the scrollport code so we can use it on individual images later.

I haven't yet addressed your comments about ExpandRect making more sense. I will do it in a followup. But this should address your other comments.
Comment 40 Timothy Nikkel (:tnikkel) 2013-09-11 23:36:47 PDT
Created attachment 803523 [details] [diff] [review]
Part 6. Use the first reflow of relevant image frames to add/remove them from the visible list.
Comment 41 Mats Palmgren (vacation) 2013-09-12 14:34:06 PDT
Comment on attachment 803519 [details] [diff] [review]
Part 2. Add a function to remove an image from the visible list.

MOZ_ASSERT(mVisibleImages.Count() == 0) in the AssumeAllImagesVisible()
early return seems prudent.  r=mats
Comment 42 Mats Palmgren (vacation) 2013-09-12 14:50:22 PDT
Comment on attachment 803522 [details] [diff] [review]
Part 3. Factor out the expand the scrollport code so we can use it on individual images later.

About the use of mScrollPort in nsGfxScrollFrameInner::IsRectNearlyVisible -
I wonder if it should take the display port into account?
Similar to the patch in bug 915526.

+  virtual bool IsRectNearlyVisible(const nsRect& aRect) {
+    return mInner.IsRectNearlyVisible(aRect);
+  }

add MOZ_OVERRIDE (two places)
Comment 43 Timothy Nikkel (:tnikkel) 2013-09-12 15:04:42 PDT
(In reply to Mats Palmgren (:mats) from comment #42)
> About the use of mScrollPort in nsGfxScrollFrameInner::IsRectNearlyVisible -
> I wonder if it should take the display port into account?
> Similar to the patch in bug 915526.

Yeah, the point of the patch in bug 915526 is to make this code work when display ports are set. So you can think of the patch in bug 915526 as fixing this review comment.
Comment 44 Timothy Nikkel (:tnikkel) 2013-09-12 16:16:11 PDT
About ExpandRect, I actually originally had the more obvious way where you just use the local scroll port as the "visible rect" to expand if any part of the incoming visible rect intersected the scroll frame. But that had some problems.

The first problem was a long page that had basically the entire page contents in an overflow: hidden element. So we'd start out with a rect the size of the viewport (times 3) but when it got to the huge overflow hidden element the rect would balloon out to the size of the element, making everything visible. The best solution I came up with at the time was to just use the intersection. This made sense, but expanding this rect up/down/left/right to account for potential future scrolling didn't make much sense when the rect was not the whole scroll port. I didn't feel like this was a big problem because the rect is already the result of expanding the parent's scroll port, so if the child scroll frame still only partially intersects the "visible rect" we would need to first scroll the parent scroll frame and then scroll the child frame before this even made a difference.

The second problem was overflow hidden or scroll elements that had no scrollable range because there was no constraint set on their size, so they just fit their entire contents (this came up on a page I was testing with, probably not that uncommon either I think). I didn't want these to cause us to expand the rect at all (since the expansion is meant to factor in potential scrolling), so I added the current scroll range checks.

I'm open to different ways to implement ExpandRect, but I don't want to regress those two situations.
Comment 45 Timothy Nikkel (:tnikkel) 2013-09-12 21:37:47 PDT
comment 44 in is reference to comment 18 / comment 23
Comment 46 Mats Palmgren (vacation) 2013-09-13 09:38:44 PDT
I think ExpandRect itself makes sense, i.e. that it only inflates
the side(s) that has something to scroll to.

It's the call in BuildDisplayList that uses 'dirtyRect' that looks
a bit weird, but I guess it's a good enough heuristic since it
should reflect the viewable part when there are nested frames
that clip.  My (minor) worry there would be that it's too small so
you would see an image placeholder when you scroll.  I don't have
a better suggestion though, we can tweak it if someone observes
a problem. (perhaps making width/height be K*page-width/height
as a minimum?)

An observation while looking at the code in BuildDisplayList -
would it be worth it to return early after BuildDisplayListForChild
in the IsForImageVisibility() case, as an optimization?

Anyway, r+ to land the code as is.
Comment 47 Timothy Nikkel (:tnikkel) 2013-09-13 10:11:21 PDT
Created attachment 804550 [details] [diff] [review]
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll.

Algorithm the same, but using margins now. Tell me if you prefer the old way.
Comment 48 Timothy Nikkel (:tnikkel) 2013-09-13 10:12:35 PDT
Comment on attachment 804550 [details] [diff] [review]
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll.

Oh, and I'll add a comment with the main points from comment 44 to explain it.
Comment 49 Timothy Nikkel (:tnikkel) 2013-09-13 10:44:44 PDT
Created attachment 804565 [details] [diff] [review]
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell.

Updated to review comments except for the one about swapping hash tables, are you ok with doing that in a followup?
Comment 50 Mats Palmgren (vacation) 2013-09-13 11:30:07 PDT
Comment on attachment 804550 [details] [diff] [review]
Part 4. Tweak the ExpandRect function to not expand in a direction that we cannot scroll.

This looks good.
Comment 51 Mats Palmgren (vacation) 2013-09-13 11:36:02 PDT
Comment on attachment 804565 [details] [diff] [review]
Part 1. Use a hashtable instead of an array to store list of visible images on the presshell.

> Updated to review comments except for the one about swapping hash tables,
> are you ok with doing that in a followup?

Yes, let's do that in a followup bug.
Comment 52 Timothy Nikkel (:tnikkel) 2013-09-13 15:45:18 PDT
(In reply to Mats Palmgren (:mats) from comment #46)
> My (minor) worry there would be that it's too small so
> you would see an image placeholder when you scroll.

Just to be clear we always draw an image if we are asked to draw it, but if it's not already decoded then there will be a lag before it is drawn. So if we make a mistake here the worst that could happen is a bit of decode lag for an image.
Comment 54 Nicholas Nethercote [:njn] 2013-09-14 18:36:02 PDT
Oh wow, this landed!  tn, do you have any measurements showing the impact?  Also, in comment 2 we elected to keep this separate from bug 542158, but is there really anything left to do for bug 542158?
Comment 55 Timothy Nikkel (:tnikkel) 2013-09-14 18:50:15 PDT
Comment 13 is probably what you are looking for on measurements.

This isn't a perfect 100% solution to bug 542158. I think it should solve most (or almost all) of the problems. We'll see what kinds of images using too much memory reports we get after this bug lands on m-c.
Comment 57 Emanuel Hoogeveen [:ehoogeveen] 2013-09-15 00:22:51 PDT
Could you give an overview of when this bug kicks in? For instance:
1) Does it only affect things on page load, or does it also change what happens when you switch to a background tab after its images have been discarded?
2) Does it include any logic to discard images that are out of view on a foreground tab, or conversely, to prevent discarding images that are in view in background tabs?
Comment 58 Timothy Nikkel (:tnikkel) 2013-09-15 00:32:17 PDT
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #57)
> 1) Does it only affect things on page load, or does it also change what
> happens when you switch to a background tab after its images have been
> discarded?

It has no effect on switching to a background tab. The main effect of these patches is to stop assuming all images are visible by default, instead marking them visible or not visible after their first reflow (which is also not perfect). Images can get their first reflow on page load, or when layout items are first created for them (when they are inserted into the dom or become something other than display:none).

> 2) Does it include any logic to discard images that are out of view on a
> foreground tab, or conversely, to prevent discarding images that are in view
> in background tabs?

We already have logic to discard images that are out of view on the foreground tab. No changes were made to that. No changes were made to the logic of discarding images that are in view in background tabs, but any memory use decrease of images could result in less discarding.
Comment 59 Emanuel Hoogeveen [:ehoogeveen] 2013-09-15 00:41:14 PDT
Thanks for the clarification!

(In reply to Timothy Nikkel (:tn) from comment #58)
> We already have logic to discard images that are out of view on the
> foreground tab.
Ah, that's good to hear! I seem to have forgotten when that changed.

> No changes were made to the
> logic of discarding images that are in view in background tabs, but any
> memory use decrease of images could result in less discarding.
Are there any bugs open for that? Switching between several pages with a single large (previously decoded) image on them, there's often a noticeable wait before the image shows up again.
Comment 60 Nicholas Nethercote [:njn] 2013-09-15 01:22:53 PDT
> > We already have logic to discard images that are out of view on the
> > foreground tab.
> Ah, that's good to hear! I seem to have forgotten when that changed.

That was bug 689623 -- see https://blog.mozilla.org/nnethercote/2013/03/07/memshrink-progress-week-89-90/ for details.
Comment 61 Nicholas Nethercote [:njn] 2013-09-15 16:54:54 PDT
*** Bug 542158 has been marked as a duplicate of this bug. ***
Comment 62 Nicholas Nethercote [:njn] 2013-09-15 23:37:25 PDT
I can corroborate tn's measurements from comment 13.

I saved http://congressoamericano.blogspot.ca/p/fotos-do-congresso-americano-iii.html locally (to avoid slow network effects) and then measured RSS every 0.1 seconds.  Prior to this being fixed, it briefly peaked at ~2,400 MB before dropping back to ~280 MB.  After being fixed, it just went directly to ~280 MB.  (When loading from the network, the peak was typically lower, e.g. 1.5 GB, though it varied quite a bit.)

Also, prior to the fix, there was about a 5 second lag after I triggered the load (i.e. hit <enter> in the address bar) before the page appeared.  After being fixed, it's about a 1 second lag.

Great stuff! :)
Comment 63 Mihai Morar, (:MihaiMorar) 2013-10-25 01:29:54 PDT
  As I had discussed with :tn yesterday on IRC, I've done a couple of tests for Pre-beta Sign-Off of this feature, and I can confirm that this fix is verified. 
  I've tested this feature on Windows 7x64, Ubuntu 13.04 x64 and Mac OSX 8.4, by measuring the memory usage in about:memory while loading the URL mentioned in Comment 13 on a fixed build (Latest Aurora 26) and on a non fixed build (FF 25RC).

Results

1. Windows 7x64:
  a)Latest Aurora 26: Memory usage increased from 150MB to 280-300MB. No hang or freeze encountered.
  b)FF 25RC: Memory usage increased from 150MB to 1500MB. I've encountered hangs and then freeze while loading the URL.

2. Ubuntu 13.04 x64:
  a)Latest Aurora 26: Memory usage increased from 150MB to 350-400MB. No hang or freeze encountered.
  b)FF 25RC: Memory usage increased from 150MB to 1000MB. I've encountered hangs and then freeze while loading the URL.

2. MAC OSX 8.4:
  a)Latest Aurora 26: Memory usage increased from 200MB to 350-400MB. No hang or freeze encountered.
  b)FF 25RC: Memory usage increased from 150MB to 1350MB. I've encountered a couple of hangs while loading the URL.

I can provide similar results for Latest Nightly 27 as those mentioned above for Latest Aurora 26.

My PC configurations:
1. For Windows & Ubuntu testing: i5 Intel 3.6Ghz, 4GB DDR3.
2. For Mac OSX 8.4 testing: i5 Intel 2.5Ghz, 4GB DDR3.

Builds used for testing:

FF 25RC: BuildID: 20131022215409
Latest Aurora 26: BuildID: 20131024004004
Latest Nightly 26: BuildID: 20131024030204

If there is anything else that is required to be tested for Pre-beta Sign-Off of this feature please let me know, otherwise I will mark this Bug as being Verified. 

Thanks tn: for information provided!
Comment 64 infoplus007 2013-11-07 20:05:36 PST
I'm not a coder but a user & with some extensions like GPUM https://addons.mozilla.org/en-US/firefox/addon/gpum/  there is some problems...

The "preview display" is broken.. here's a video/screencast ---> http://www.screenr.com/IA6H  to better understand.

Thx !
Comment 65 Timothy Nikkel (:tnikkel) 2013-11-07 21:32:20 PST
(In reply to infoplus007 from comment #64)
> I'm not a coder but a user & with some extensions like GPUM
> https://addons.mozilla.org/en-US/firefox/addon/gpum/  there is some
> problems...
> 
> The "preview display" is broken.. here's a video/screencast --->
> http://www.screenr.com/IA6H  to better understand.
> 
> Thx !

Could you please file a new bug for that issue please?
Comment 66 tomorrow 2013-11-15 21:43:58 PST
Hmm i've been using the following pref for a long time. Is it the same or is this proposed change somehow different: image.mem.decodeondraw is set to true (default false).

Note You need to log in before you can comment on or make changes to this bug.