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.

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.

(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.

(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.

(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

(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?

Moving the WillDrawOpaqueNow() logic to inside IsOpaque() sounds like a fine plan here. Any concerns with that proposal?

(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.

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

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

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

https://hg.mozilla.org/mozilla-central/rev/bfbb874b4641 https://hg.mozilla.org/mozilla-central/rev/85cfd0bd6eb0

I just noticed this landed. Want to request uplift for 50 ? It is too late for 49.

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

Comment on attachment 8790151 [details] [diff] [review] aurora patch Fixes a recent regression, Aurora50+