847221 - Separate the notions of "image visibility" and "image is in an active tab"

Status: NEW

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Justin Lebar (not reading bugmail)]

After bug 689623, we conflate the notions of "image X is visible" and "image X is in an active tab".

I'd like to separate out these notions, for two reasons:

1) When you're browsing a page without tons of images, we should probably keep all of the page's images decoded.  AFAICT we can't do this right now because of the conflation above.

2) When you're browsing a page without a ton of images, we might want to keep in memory the visible images from a background tab.  Right now we discard them after ~20s, which we have to do because we can't tell the difference between "image X is outside tab Y's viewport" and "image X is in a background tab".

I think this just requires some changes to nsImageLoadingContent, nsDocument, and then the imagelib stuff.

Comment 1

[Justin Lebar (not reading bugmail)]

My very wip code for this (probably doesn't even compile, in the middle of some renames, etc) is on github.

The branch is https://github.com/jlebar/mozilla-central/tree/image-scroll-jank-845147
and the current head (in case I rebase the branch or something) is https://github.com/jlebar/mozilla-central/commit/5d3428a6b7b0561afdf9affd05fe6da907a66185

Comment 2

[Justin Lebar (not reading bugmail)]

Let me know if you want me to post a traditional patch here.

Comment 3

[IU]

(In reply to Justin Lebar [:jlebar] from comment #0)
> 1) When you're browsing a page without tons of images, we should probably
> keep all of the page's images decoded.  AFAICT we can't do this right now
> because of the conflation above.

What if the page you're on doesn't have tons of images but they are all very high resolution?  Does your patch account for this?  If not, could you please include the ability and a pref to disable this behavior.
 
> 2) When you're browsing a page without a ton of images, we might want to
> keep in memory the visible images from a background tab.  Right now we
> discard them after ~20s, which we have to do because we can't tell the
> difference between "image X is outside tab Y's viewport" and "image X is in
> a background tab".

What about the scenario where someone has many image pages open in multiple tabs?  Some people extremely high number of tabs open.  If you're going to keep all the "visible" images of every tab decoded, this is going to effectively negate the progress that's been made.  I believe this should be done only for those background tabs to the left and to the right of the active tab.

Comment 4

[Justin Lebar (not reading bugmail)]

My intent here is not to add any new discard heuristics.  The intent here is just to separate out the notion of "in an active tab" and "visible in an active tab" so that we can later implement these heuristics.

We can discuss the heuristics once we get there.  You'll have to forgive me for being imprecise about the intended heuristics in comment 0.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Refactor out the image tracking from current nsDocument.

Based on Justin's WIP, refactor out the image tracking from nsDocument into DocumentImageTracking; unlike the WIP, leave it a refactoring, and don't change any logic or add new functionality.  This is just the first step, but if it's going in the same direction as the original idea, it may be useful.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 753235 [details] [diff] [review]
Refactor out the image tracking from current nsDocument.

This patch looks great to me!

>diff --git a/content/base/src/DocumentImageTracker.h b/content/base/src/DocumentImageTracker.h

>+#ifndef DocumentImageTracker_h__
>+#define DocumentImageTracker_h__

Since this header's main namespace is mozilla, we should #define
mozilla_DocumentImageTracker_h__.

>+class DocumentImageTracker {
>+private:
>+  nsDataHashtable< nsPtrHashKey<imgIRequest>, int32_t> mImages;

Nit: No space after <.  It would also be nice to have a comment explaining what
this hashtable represents.

>diff --git a/content/base/src/DocumentImageTracker.cpp b/content/base/src/DocumentImageTracker.cpp
>new file mode 100644
>--- /dev/null
>+++ b/content/base/src/DocumentImageTracker.cpp

>+PLDHashOperator
>+IncrementAnimationEnumerator(imgIRequest* aKey, int32_t aData, void* userArg)

These enumerators should be static or in an anonymous namespace; otherwise
they'll collide with anyone else who names a function with the same name.

>+DocumentImageTracker::DocumentImageTracker()
>+ : mImages(),

We don't usually include default constructors in initialization lists.

>+   mLockingImages(false),
>+   mAnimatingImages(true)
>+{
>+}

>+DocumentImageTracker::~DocumentImageTracker()
>+{
>+  Clear();
>+}

Why do we need to do this?

>+void
>+DocumentImageTracker::Clear()
>+{
>+  mImages.Clear();
>+}

It's not clear why we need this function either.

>+void
>+DocumentImageTracker::Init()
>+{
>+  if (!sPrefsInitialized) {
>+    sPrefsInitialized = true;
>+    Preferences::AddUintVarCache(&sOnloadDecodeLimit, "image.onload.decode.limit", 0);
>+  }
>+  mLockingImages = false;
>+  mAnimatingImages = true;
>+  mImages.Init();
>+}

I think all of this can go in the constructor (so you don't have to call Init()
separately).  The main reason we have Init() functions is for when you want to
call an XPCOM method on |this|.  While the constructor runs, |this| has a
refcnt of 0, so it's illegal to pass |this| to any XPCOM methods.

But DocumentImageTracker isn't refcounted, so we definitely don't have this
problem.

>+void
>+DocumentImageTracker::SetImageLockingState(bool aLocked)
>+{
>+  // If there's no change, there's nothing to do.
>+  if (mLockingImages == aLocked) {
>+    return;
>+  }
>+
>+  // Otherwise, iterate over our images and perform the appropriate action.
>+  mImages.EnumerateRead(aLocked ? LockEnumerator
>+                                      : UnlockEnumerator,
>+                              nullptr);

Indentation.  (You could align ? and :, then align nullptr with aLocked.)

>+void
>+DocumentImageTracker::SetImagesNeedAnimating(bool aAnimating)
>+{
>+
>+  // If there's no change, there's nothing to do.
>+  if (mAnimatingImages == aAnimating) {
>+    return;
>+  }
>+
>+  // Otherwise, iterate over our images and perform the appropriate action.
>+  mImages.EnumerateRead(aAnimating ? IncrementAnimationEnumerator
>+                                         : DecrementAnimationEnumerator,
>+                              nullptr);

Indentation.

>+void
>+DocumentImageTracker::Discard()

Maybe this should be called RequestDiscardAll to match the function it's
calling.

>+nsresult
>+DocumentImageTracker::AddImage(imgIRequest* aImage)
>+{
>+  NS_ENSURE_ARG_POINTER(aImage);
>+
>+  // See if the image is already in the hashtable. If it is, get the old count.
>+  int32_t oldCount = 0;
>+  mImages.Get(aImage, &oldCount);
>+
>+  // Put the image in the hashtable, with the proper count.
>+  mImages.Put(aImage, oldCount + 1);
>+
>+  nsresult rv = NS_OK;
>+
>+  // If this is the first insertion and we're locking images, lock this image
>+  // too.
>+  if (oldCount == 0) {
>+    if (mLockingImages)
>+      rv = aImage->LockImage();
>+    if (NS_SUCCEEDED(rv) && (!sOnloadDecodeLimit ||
>+                             mImages.Count() < sOnloadDecodeLimit))
>+      rv = aImage->StartDecoding();
>+  }
>+
>+  // If this is the first insertion and we're animating images, request
>+  // that this image be animated too.
>+  if (oldCount == 0 && mAnimatingImages) {
>+    nsresult rv2 = aImage->IncrementAnimationConsumers();
>+    rv = NS_SUCCEEDED(rv) ? rv2 : rv;
>+  }
>+
>+  return rv;

This rv logic is probably copied from nsDocument, but it's pretty confusing,
and I'm afraid someone is going to break it accidentally if they change this
method.  Can we simplify it, perhaps by reading and writing only |rv1| and
|rv2| within the branches, and then combining them in the final return?

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp

>+  mImageTracker.RemoveImage(aImage);
> 
>   if (aFlags & REQUEST_DISCARD) {
>     // Request that the image be discarded if nobody else holds a lock on it.
>-    // Do this even if !mLockingImages, because even if we didn't just unlock
>-    // this image, it might still be a candidate for discarding.
>+    // Do this even if !mImageTracker.mLockingImages, because even if we didn't
>+    // just unlock this image, it might still be a candidate for discarding.

It would be nice if we didn't have to refer to another class's private member
variable here.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Refactor out the image tracking from current nsDocument.

Addressing the code review comments, carrying over the r=jlebar.  There was one additional change - the list of C++ files is now in moz.build, rather than Makefile.in.

Comment 8

[Justin Lebar (not reading bugmail)]

To be clear, this is just part 1, right?  Or do you want to morph this bug to cover just the changes you're making here?

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Just part 1 - or I can create a separate bug just for this change.  Do you think it's worth doing that?

Comment 10

[Justin Lebar (not reading bugmail)]

I'd lean towards keeping everything in the one bug that you would want to land together.  Landing this without some other changes doesn't necessarily make a lot of sense to me.  But it's up to you.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Sure.  I'm still getting used to the pace.  I would have in the past landed this right away, to be able to deal with any regression ranges better, but you're right in that this is just part 1, so it may not make sense on its own.  Of course, given that it's nsDocument, the bit rot is probably very fast, so lets see how quickly this can be done.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2: Unit tests for DocumentImageTracker (wip)

I don't like what I've done to Preferences in order to get this working, but thought I'd run it by others. BenWa, any other suggestions? Or just wait for the GTest/Preferences?

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 756828 [details] [diff] [review]
Part 2: Unit tests for DocumentImageTracker (wip)

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

Opps, I missed the original email, sorry for the delay.

::: content/base/src/DocumentImageTracker.cpp
@@ +59,5 @@
>     mAnimatingImages(true)
>  {
>    if (!sPrefsInitialized) {
>      sPrefsInitialized = true;
> +    if (Preferences::GetServiceIfInitialized() != nullptr) {

I think it's better to get the preference service working in GTest but if we're looking to get a test in quickly this might be fine. I don't want this to become an accepted pattern in the long run however.

I've got a patch that added a ScopedXPCOM to the main method of GTest. Perhaps that sufficient to get the preference service initialized.

::: content/base/src/Makefile.in
@@ +5,5 @@
>  
>  DEPTH		= @DEPTH@
>  topsrcdir	= @top_srcdir@
>  srcdir		= @srcdir@
> +VPATH           =  $(srcdir) $(srcdir)/unittest

extra space

@@ +14,5 @@
>  MSVC_ENABLE_PGO := 1
>  LIBXUL_LIBRARY	= 1
>  
> +GTEST_CPPSRCS = \
> +        TestDocumentImageTracker.cpp \

Its inconsistent in this file but ideally these should be two space indented.

::: content/base/src/unittest/TestDocumentImageTracker.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/

IANAL: I think we just need this license

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from comment #13)

This can wait for the GTest preference service initialization, at which point I'll ask for a review.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.