1060200 - Store HQ scaled frames in the SurfaceCache

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

We want to support multiple scaled frames at the same time in RasterImage, because not doing so needlessly limits us and is the store of bugs that can peg Firefox at 100% CPU. We also want to move all the image data that we can in imagelib into SurfaceCache, so we can easily have global memory management policies that apply to everything in imagelib.

In this bug, we'll achieve the first goal and move closer to the second by moving HQ scaled frames into the SurfaceCache.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add support for RasterImage surfaces to SurfaceCache

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8481051 [details] [diff] [review]
(Part 1) - Add support for RasterImage surfaces to SurfaceCache

>diff --git a/image/src/VectorImage.cpp b/image/src/VectorImage.cpp
>--- a/image/src/VectorImage.cpp
>+++ b/image/src/VectorImage.cpp
>   DrawableFrameRef frameRef =
>     SurfaceCache::Lookup(ImageKey(this),
>-                         SurfaceKey(params.size, aSVGContext,
>-                                    animTime, aFlags));
>+                         VectorSurfaceKey(params.size,
>+                                          params.svgContext,
>+                                          params.animationTime));

Hmm... So we're dropping "flags" from the hash key here, for VectorImage. Is that safe? I think it is...?

It looks like the only flags we care about in VectorImage are FLAG_BYPASS_SURFACE_CACHE and FLAG_SYNC_DECODE, neither of which influence the rendering. So it seems like it's fine that we're ignoring the flags in making the hash key for the surface cache.

Maybe add a comment above the VectorSurfaceKey() implementation, though, as an explicit heads-up about this, saying something like:
  // NOTE: If we ever add a new FLAG_* to imgIContainer::Draw() that
  // influences VectorImage rendering, then we'll need to make this
  // function require aFlags to use as part of the hash key.

r=me with something like that

Comment 3

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Hmm... So we're dropping "flags" from the hash key here, for VectorImage. Is
> that safe? I think it is...?
> 
> It looks like the only flags we care about in VectorImage are
> FLAG_BYPASS_SURFACE_CACHE and FLAG_SYNC_DECODE, neither of which influence
> the rendering. So it seems like it's fine that we're ignoring the flags in
> making the hash key for the surface cache.

Yeah, I realized when reasoning about which flags we cared about for RasterImage that we cared about *no* flags for VectorImage. =) I agree that we could use a comment there, though!

(We could probably also use a comment about the fact that we don't care about animation for RasterImage because it's currently not possible to store animated frames in the SurfaceCache.)

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add support for RasterImage surfaces to SurfaceCache

This version of the patch adds the comments discussed in comment 2 and comment 3.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a way to release all the surfaces an imgFrame holds

This patch adds a way to release all the surfaces an imgFrame holds without destroying it. We'll use that in part 3 to allow an imgFrame that is the result of a failed scale operation to remain in the SurfaceCache, serving as a sentinel that keeps us from trying to reattempt the scale over and over, while still releasing the memory that imgFrame was using.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Ack, sorry. I changed my mind about part 2. I'm going to solve that problem another way.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add SurfaceCache::RemoveIfPresent so invalid entries can be freed eagerly

Alright, here's the new part 2. The approach in the previous part 2 was flawed, since the first time Lookup() got called the empty imgFrame would have been removed from the cache anyway, since you couldn't create a DrawableFrameRef to it. Given that, and the fact that on reflection the current code works in exactly the same way (if scaling fails we will just retry), I've decided to just do the same thing more directly rather than increasing imgFrame's internal complexity.

This patch adds a function called SurfaceCache::RemoveIfPresent(), and I think its behavior is probably pretty obvious. =)

So it's worth asking the question: even if the current code doesn't do anything to rate limit scaling requests that keep failing, wouldn't it be better to solve that problem? I think that we'd be better off deferring that problem for a while, because once downscale-during-decode arrives (and it's coming very soon), scaling failure and decode failure will be essentially the same thing, and we already have a way of handling decode failure. (Right now, if a decode *ever* fails, we'll never try again. I think that's probably the right thing, because there doesn't seem to be any reason to think that future attempts will succeed if one attempt fails.)

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest and ScaleResult

Here's the patch with the 'meat' of the change. We store HQ scaled frames in the SurfaceCache. ScaleRequest and ScaleResult are totally removed, which triggers some refactoring in various places that I think is universally an improvement.

RasterImage used to pay attention to three 'ScaleStatus' states. They all have equivalents in the new code.

SCALE_PENDING meant that a scale job had started but hadn't yet finished. Now, that's represented by the situation where SurfaceCache::Lookup successfully returns an imgFrame, but calling imgFrame::ImageCompleted() on that frame returns false.

SCALE_DONE meant that a scale job was done and we could draw the resulting imgFrame. Now we know that's the case if SurfaceCache::Lookup returns an imgFrame and calling imgFrame::ImageCompleted() returns true.

SCALE_INVALID meant that scaling failed and the resulting imgFrame was invalid, so we should scale again. Now we handle that case by removing the imgFrame from the SurfaceCache, so Lookup() just fails and we request a new scale if needed. (As discussed in comment 7, that's the existing behavior, and it's not worth diverging from that right now since downscale-during-decode will render any HQ-scaling-specific policy we make obsolete very soon.)

Comment 9

[Seth Fowler [:seth] [:s2h]]

I've tested, and this patch series does indeed resolve bug 1050815 - we get all of the HQ scaled versions we request, and no 100% CPU problem. I haven't checked the other dependent bugs yet, but I'm confident that it'll resolve them too.

Here's a try job:

https://tbpl.mozilla.org/?tree=Try&rev=4875f9f2ad2a

Comment 10

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #9)
> Here's a try job:

So green, so green...

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8481140 [details] [diff] [review]
(Part 3) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest and ScaleResult

A few nits on ScaleRunner in Part 3:

>+class ScaleRunner : public nsRunnable
> {
[...]
>+  ~ScaleRunner()
>   {

nit: The destuctor should be private or protected (since this is a refcounted object, which means we don't want just anybody to be able to call 'delete' on it).

It should also probably be marked 'virtual' (for clarity -- the parent class, nsRunnable, has a virtual destructor, so we will too).

[...]
>   NS_IMETHOD Run()
>   {
[...]
>+    if (mState == eReady) {
>+      // Collect information from the frames that we need to scale.

Nit: Run() should be marked as MOZ_OVERRIDE.

Comment 12

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> A few nits on ScaleRunner in Part 3:

Agreed; I'll make those changes. Thanks for the reviews!

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest and ScaleResult

This version of part 3 addresses Daniel's comments.

Comment 15

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8488499 [details] [diff] [review]
(Part 3) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest and ScaleResult

Just so that it's clear: this patch makes us no longer call LockImage on the RasterImage while we are scaling, this is okay because we now hold a nsRefPtr to the imgFrame that we are scaling, whereas we didn't before ("before" meaning before this patch and one of your other recent patches).

Shouldn't we be purging entries in the surface cache in RasterImage::Discard for this image?

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #15)
> Comment on attachment 8488499 [details] [diff] [review]
> (Part 3) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest
> and ScaleResult
> 
> Just so that it's clear: this patch makes us no longer call LockImage on the
> RasterImage while we are scaling, this is okay because we now hold a
> nsRefPtr to the imgFrame that we are scaling, whereas we didn't before
> ("before" meaning before this patch and one of your other recent patches).

We did that to avoid getting discarded while scaling. As you say, since we hold a strong reference to the imgFrame in ScaleRunner now, that's no longer a problem.

> Shouldn't we be purging entries in the surface cache in RasterImage::Discard
> for this image?

Yeah, we probably should. Though the surfaces get freed on a timer anyway; we don't need the DiscardTracker timer for things in the SurfaceCache, and indeed the SurfaceCache is about to replace the DiscardTracker totally. But as long as we support explicit calls to RequestDiscard() we'll want to remove SurfaceCache entries in RasterImage::Discard.

The very next bug in the series (bug 1060869) does add this feature. (And adds a significant amount of code to SurfaceCache to support image locking.)

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #16)
> The very next bug in the series (bug 1060869) does add this feature. (And
> adds a significant amount of code to SurfaceCache to support image locking.)

Yeah, I saw that after.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! I went ahead and pushed it in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6edaf07a73
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d5540e82f90
https://hg.mozilla.org/integration/mozilla-inbound/rev/d295c8ec4b5b

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this changes out since i guess this caused https://tbpl.mozilla.org/php/getParsedLog.php?id=48072061&tree=Mozilla-Inbound

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Handle SurfaceCache function calls after shutdown

So I investigated the failures that led to this getting backed out, and it looks like at the time we're destroying the RasterImage object that leads to the failure, the SurfaceCache instance has already been torn down. (In other words, ShutdownModule has already called SurfaceCache::Shutdown().)

Given the much different timing of the xpcshell test environment, it's not surprising that this could happen. Given that RasterImage objects are refcounted and kept alive indirectly by all sorts of other objects, I'm actually surprised this hasn't been an issue so far!

To make this safe, we need to make SurfaceCache functions fail gracefully when the instance has been torn down. This patch does this by changing the MOZ_ASSERT(sInstance) calls in the various SurfaceCache functions into if() checks.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest and ScaleResult

(Rebased and renumbered to part 4.)

Comment 22

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #20)
> Given that RasterImage objects are
> refcounted and kept alive indirectly by all sorts of other objects,

(This should have said "Image objects" and not "RasterImage objects". This is actually the first patch that puts surfaces from RasterImage objects in the SurfaceCache.)

Also, to state it more clearly: I've renumbered things so that the old part 3 is now part 4.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Thanks for the quick review! Just in case there are any lingering issues, I've pushed another try job:

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

Comment 24

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Store HQ scaled frames in SurfaceCache and remove ScaleRequest and ScaleResult

This version of the patch is rebased against bug 1067207 and makes a minor change (refusing to scale frames with padding) that I promised to make in that bug.

The failures in the try job above are actually coming from bug 1067207. I believe I've fixed that patch, so here's a fresh try job:

https://tbpl.mozilla.org/?tree=Try&rev=8e75f43e78e8

Comment 25

[Seth Fowler [:seth] [:s2h]]

Try looks pretty good, so I went ahead and pushed this in again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/af081b216b86
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6985b70d53e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf7aaed0b8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e337e2f16a2f

Comment 26

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/797276ccbf22 for a bunch of b2g R1 unexpected != passes, https://tbpl.mozilla.org/php/getParsedLog.php?id=48256415&tree=Mozilla-Inbound

Comment 27

[Seth Fowler [:seth] [:s2h]]

So I'd expect that to be happening if we were suddenly using HQ scaled frames on B2G. I've pushed a modified version of the patch that will crash if we do that:

https://tbpl.mozilla.org/?tree=Try&rev=0ac86eb833ef

Comment 28

[Seth Fowler [:seth] [:s2h]]

OK, after seeing those results I have come to the conclusion that, although I'm not really sure why, this patch seems to have fixed those tests on B2G. Nothing wrong seems to be happening here; we're not using scaled frames inappropriately, or anything like that.

I'd feel more comfortable if I understood exactly how this patch caused those tests to start passing, but it's important to get this bug in ASAP, as it fixes some important user-visible problems. I don't think it's worth taking the time to debug any further at this point. So I went ahead and removed the 'fails-if(B2G)' annotations from those tests.

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4717057623
https://hg.mozilla.org/integration/mozilla-inbound/rev/af506098d6b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e14ef96366
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa25a75b4ad9

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a4717057623
https://hg.mozilla.org/mozilla-central/rev/af506098d6b6
https://hg.mozilla.org/mozilla-central/rev/17e14ef96366
https://hg.mozilla.org/mozilla-central/rev/fa25a75b4ad9

Comment 30

[Elbart]

https://bugzilla.mozilla.org/show_bug.cgi?id=795072#c6 (rather nsfw) ain't fixed yet.
Opening the mentioned links in that order, when opening the last link the tiny image is hq-scaled but the bigger version isn't.

When loading the last link first or reloading it with Shift-F5, both versions of the image are hq-scaled.

Should I open a new bug?

Comment 31

[Seth Fowler [:seth] [:s2h]]

(In reply to Elbart from comment #30)
> Should I open a new bug?

I couldn't reproduce this on my local build that has the patches in this bug. I'm not sure whether you're testing on Nightly or your own build, but if you're using Nightly, I suggest to wait for tomorrow's build and see if you can still reproduce the problem. If so, please file a new imagelib bug and CC me on it.

BTW, I'd *really* appreciate it if you could create a new testcase that isn't NSFW. Just change the image to a cat picture or something. It will get the bug fixed faster.

Comment 32

[Elbart]

(In reply to Seth Fowler [:seth] from comment #31)
> (In reply to Elbart from comment #30)
> > Should I open a new bug?
> 
> I couldn't reproduce this on my local build that has the patches in this
> bug. I'm not sure whether you're testing on Nightly or your own build, but
> if you're using Nightly,

I used the latest inbound I could find just minutes before I sent the posting.
I will wait for the next Nightly, to be sure.

Comment 33

[Seth Fowler [:seth] [:s2h]]

(In reply to Elbart from comment #32)
> I used the latest inbound I could find just minutes before I sent the
> posting.
> I will wait for the next Nightly, to be sure.

If you're using inbound, that should be good enough. Maybe there's something different in how I'm testing it? I'm opening each link in a new tab, looking at that tab, then opening the next one. (The new bug would be a good place to put more precise steps to reproduce; we don't need to keep talking about it here.)

Comment 34

[:Gijs (he/him)]

Seth, is there any chance of uplifting this to 34 or is that too risky / not an option / dependent on too many other bugs that got fixed on 35?

Comment 35

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

While I have no doubt Seth's code is of awesome quality and stability, uplifting something of this magnitude to what is going to be Beta in less than two weeks really scares me.

Comment 36

[Seth Fowler [:seth] [:s2h]]

(In reply to Milan Sreckovic [:milan] from comment #35)
> While I have no doubt Seth's code is of awesome quality and stability,
> uplifting something of this magnitude to what is going to be Beta in less
> than two weeks really scares me.

Thanks for the diplomatic answer. =)

I agree with Milan; this patch it built on a whole stack of complex changes that would need to be uplifted. It was only last week I was racing against the clock to fix hard-to-reproduce crash bugs that were threatening to force a backout. Though I wish it were otherwise, I think we need to let this ride the trains normally to make absolutely sure all the bugs are squashed.

Comment 37

[Florian Bender]

Release Note Request (optional, but appreciated)
[Why is this notable]: Less ressource usage for scaled images (especially very big images scaled to very small ones), see e.g. Bug 846315
[Suggested wording]: Reduced ressource usage for scaled images
[Links (documentation, blog post, etc)]:

Comment 38

[Emanuel Hoogeveen [:ehoogeveen]]

ressource -> resource