layout uses imgIContainer::IsOpaque to determine if the image will draw opaque, but it doesn't guarantee that

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

({regression})

unspecified
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 wontfix, firefox50+ fixed, firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

For instance, if the image has been decoded, but then the decoded frames have been discarded the image may not draw opaquely into all pixels (because there is no image data for some pixels because they haven't been decoded yet).

The causes garbage to be drawn to screen, or the contents of a previous tab. We should not be doing this.
Posted patch patch (obsolete) — Splinter Review
Attachment #8735667 - Flags: review?(seth)
Comment on attachment 8735667 [details] [diff] [review]
patch

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

::: image/RasterImage.cpp
@@ +451,5 @@
> +  // someone on another thread inserting surfaces into the surface cache could
> +  // cause our frame(s) to be discarded. We could also check if we are locked
> +  // which would prevent that, but the tradeoff (reporting that we aren't
> +  // opaque when unlocked) to cover that highly unlikely scenario doesn't seem
> +  // worth it.

This problem is actually so severe that I think this entire design is a bad idea.

Why don't we just guarantee that we draw *something* in Draw(), even if the surface is not available, if IsOpaque() returns true? Just drawing a black rect would be enough to prevent the display of any garbage.
Attachment #8735667 - Flags: review?(seth)
(In reply to Seth Fowler [:seth] [:s2h] from comment #2)
> > +  // someone on another thread inserting surfaces into the surface cache could
> > +  // cause our frame(s) to be discarded. We could also check if we are locked
> > +  // which would prevent that, but the tradeoff (reporting that we aren't
> > +  // opaque when unlocked) to cover that highly unlikely scenario doesn't seem
> > +  // worth it.
> 
> This problem is actually so severe that I think this entire design is a bad
> idea.

Why do you think it's so severe? Seems like a very rare occurence, and it could occur without this patch. This patch only improves the situation. Further, if we decide that we are still getting too much garbage drawing I have explained how we can make a very simple change to followup that will make the approach work 100% of the time (with a trade off of declaring more images transparent and thus painting more).

For this to happen we would need to have the following happen:
1) the surface cache be near capacity (I'm a tab hoarder, and I keep Firefox open for long periods and I have never seen images come anywhere near close to even a fraction of the surface cache capacity).
2) something off the main thread will need to do decoding to allocate a surface while the main thread is painting, in the short window between the call to WillDrawOpaqueNow and the call to Draw. And the image will have to be unlocked, and the image will have to be among the least recently used.

> Why don't we just guarantee that we draw *something* in Draw(), even if the
> surface is not available, if IsOpaque() returns true? Just drawing a black
> rect would be enough to prevent the display of any garbage.

This is almost as bad as drawing garbage. If the image is loaded or decoded we should be drawing what is underneath the image. Not black, not any one color.
Attachment #8735667 - Flags: review?(seth)
(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> Why do you think it's so severe? Seems like a very rare occurence, and it
> could occur without this patch. This patch only improves the situation.
> Further, if we decide that we are still getting too much garbage drawing I
> have explained how we can make a very simple change to followup that will
> make the approach work 100% of the time (with a trade off of declaring more
> images transparent and thus painting more).

We'd be better off just reporting we're not opaque when unlocked and skipping the rest of the changes, though I agree it's a bit unfortunate that that can have performance implications, particularly since doing this would not be 100% reliable unless we also did a SurfaceCache lookup in IsOpaque(). (Of course, that ends up being similar in cost to what you're doing in this patch, anyway.)

> For this to happen we would need to have the following happen:
> 1) the surface cache be near capacity (I'm a tab hoarder, and I keep Firefox
> open for long periods and I have never seen images come anywhere near close
> to even a fraction of the surface cache capacity).
> 2) something off the main thread will need to do decoding to allocate a
> surface while the main thread is painting, in the short window between the
> call to WillDrawOpaqueNow and the call to Draw. And the image will have to
> be unlocked, and the image will have to be among the least recently used.

On desktop this is highly unlikely but it's likely to happen more often on low-memory devices where we are struggling with memory pressure.

> > Why don't we just guarantee that we draw *something* in Draw(), even if the
> > surface is not available, if IsOpaque() returns true? Just drawing a black
> > rect would be enough to prevent the display of any garbage.
> 
> This is almost as bad as drawing garbage. If the image is loaded or decoded
> we should be drawing what is underneath the image. Not black, not any one
> color.

I don't agree that it's as bad as drawing garbage, but certainly it's not ideal. I'd say this, though: by your own argument, above, this is highly unlikely to ever be seen by the user on desktop, and it's drastically simpler and has the advantage of being 100% reliable and not having any performance cost (as compared to this patch, which definitely has a performance cost).

I'd really, really rather not take one unreliable mechanism, and fix it by adding another unreliable mechanism on top of it.
Attachment #8735667 - Flags: review?(seth)
(In reply to Seth Fowler [:seth] [:s2h] from comment #4)
> > > Why don't we just guarantee that we draw *something* in Draw(), even if the
> > > surface is not available, if IsOpaque() returns true? Just drawing a black
> > > rect would be enough to prevent the display of any garbage.
> > 
> > This is almost as bad as drawing garbage. If the image is loaded or decoded
> > we should be drawing what is underneath the image. Not black, not any one
> > color.
> 
> I don't agree that it's as bad as drawing garbage, but certainly it's not
> ideal.

It is. Period. Period.

> I'd say this, though: by your own argument, above, this is highly
> unlikely to ever be seen by the user on desktop, and it's drastically
> simpler and has the advantage of being 100% reliable and not having any
> performance cost (as compared to this patch, which definitely has a
> performance cost).

My argument only above only applies after we have landed the patch in this bug. This bug happens very very frequently.

> I'd really, really rather not take one unreliable mechanism, and fix it by
> adding another unreliable mechanism on top of it.

We don't have a 100% fix, we DO have a 99.9999% reliable patch, with the option of making it 100% reliable. Let's not be horrible in 99.9999% of cases just so
Attachment #8735667 - Flags: review?(seth)
(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> My argument only above only applies after we have landed the patch in this
> bug. This bug happens very very frequently.

Heh, yes. This is a good point.

> > I'd really, really rather not take one unreliable mechanism, and fix it by
> > adding another unreliable mechanism on top of it.
> 
> We don't have a 100% fix, we DO have a 99.9999% reliable patch, with the
> option of making it 100% reliable. Let's not be horrible in 99.9999% of
> cases just so

Alright, let's not draw black. You've sold me. But why introduce this new method rather than just fixing IsOpaque(), which is a simpler change that will handle all existing callers?
Flags: needinfo?(tnikkel)
Blocks: 1290907
Moving the WillDrawOpaqueNow() logic to inside IsOpaque() sounds like a fine plan here. Any concerns with that proposal?
Priority: -- → P3
(In reply to Seth Fowler [:seth] [:s2h] from comment #6)
> But why introduce this new
> method rather than just fixing IsOpaque(), which is a simpler change that
> will handle all existing callers?

RasterImage still uses IsOpaque in too places (that do not want WillDrawOpaqueNow). I made IsOpaque a private method that only exists on RasterImage.

I rebased the patch, will land when inbound opens.
Flags: needinfo?(tnikkel)
Posted patch rebased patchSplinter Review
Attachment #8735667 - Attachment is obsolete: true
Attachment #8735667 - Flags: review?(seth.bugzilla)
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54933b5b96f1
Don't draw garbage to the screen if an image doesn't happen to be decoded. r=seth
https://hg.mozilla.org/mozilla-central/rev/54933b5b96f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I had to back this out in https://hg.mozilla.org/mozilla-central/rev/ca24710db69a for frequent Win64 reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=34506959&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(tnikkel)
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
Hmm, max difference is 1, and the test is already fuzzed for skia content and d2d (but not enough total pixels to cover this failure). I'm guessing that correctly reporting the image as not opaque before its been fully decoded is causing the drawing difference. I'll likely land a patch increasing the fuzzing and re-land this patch.
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfbb874b4641
Fuzz two reftests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85cfd0bd6eb0
Don't draw garbage to the screen if an image doesn't happen to be decoded. r=seth
Flags: needinfo?(tnikkel)
https://hg.mozilla.org/mozilla-central/rev/bfbb874b4641
https://hg.mozilla.org/mozilla-central/rev/85cfd0bd6eb0
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I just noticed this landed. Want to request uplift for 50 ? It is too late for 49.
Posted patch aurora patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: been a problem for a while, got worse with some more recent changes, but don't remember specifically off the top of my head
[User impact if declined]: large images that don't decode instantly could cause us to paint garbage or blackness if we paint them before decode (like if you have a large bg image on gmail and switch to that tab for example)
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: this should be pretty safe
[String/UUID change made/needed]: none
Attachment #8790151 - Flags: approval-mozilla-aurora?
See Also: → 1225750
Comment on attachment 8790151 [details] [diff] [review]
aurora patch

Fixes a recent regression, Aurora50+
Attachment #8790151 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1225750
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.