Image was not rendered or flickered on Italic home page
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
People
(Reporter: sotaro, Assigned: aosmond)
References
()
Details
(Keywords: regression)
Attachments
(3 files)
Created by Bug 1509244 Comment 3. When I scrolled down https://italic.com/, product image was not rendered. When I contined scrolling up and down, product image was rendered as flickering. I saw the problem on Win10, mac and linux.
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
Image was not rendered when I scrolled the page to the bottom. I saw the image during disabling WebRender and chrome browser.
Comment 2•6 years ago
|
||
see also bug 1508127
Comment 3•6 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #1) > Created attachment 9027818 [details] > image during causing the probolem > > Image was not rendered when I scrolled the page to the bottom. I saw the > image during disabling WebRender and chrome browser. mozregression --good 2017-08-10 --bad 2017-10-10 --pref gfx.webrender.enabled:true gfx.webrendest.enabled:true gfx.webrender.layers-free:true gfx.webrender.blob-images:true image.mem.shared:true layout.display-list.retain:false -a https://italic.com/ > 5:04.37 INFO: Last good revision: ca7d18dbacbf103d74a3213d8d08a7c3e4def9a2 (2017-09-21) > 5:04.37 INFO: First bad revision: 3d72fdb0e561ea59d9e5850c3e71367dbb8a7148 (2017-09-22) > 5:04.37 INFO: Pushlog: > https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca7d18dbacbf103d74a3213d8d08a7c3e4def9a2&tochange=3d72fdb0e561ea59d9e5850c3e71367dbb8a7148 > [...] > 7:21.00 WARNING: Skipping build 97282b0c985a: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.revision.97282b0c985aa1778ced171514d1ae61945c634c.firefox.win64-opt' > 7:21.02 INFO: There are no build artifacts on inbound for these changesets (they are probably too old). The flickering handbag has a different range.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
So bug 1510306 and this one are likely the same issue. The author used: https://codepen.io/shshaw/full/LVKEdv to generate an SVG image. When the internal raster image finishes decoding, it invalidates the image and regenerates the surface. We seem to be oscillating between the original surface (which was just transparent) and the new surface (with the actual JPEG decoded/included).
Assignee | ||
Comment 5•5 years ago
|
||
When the internal raster image finishes decoding, we are supposed to invalidate the owning vector image. This was not happening consistently with WebRender, because the logic assumed a Draw call would come in if we needed another invalidation. I fixed that issue. However there seems to be another case where we don't receive the invalidation at all when the raster image finishes decoding. There does not seem to be an observer anymore for the SVG element. This is strange. Still investigating.
Assignee | ||
Comment 6•5 years ago
|
||
It seems that NS_FRAME_DESCENDANT_NEEDS_PAINT is still set on the root frame of the SVG document from the first inner image's frame update, so the subsequent notifications from the second inner image's frame update are not passed along to the VectorImage listener to cause us to recreate the surface.
Assignee | ||
Comment 8•5 years ago
|
||
We actually end up redrawing these large SVGs way more than we need to. I added a counter to track how many times we redraw the surfaces between calls to GetImageContainerAtSize (which resets the counter): [AO] VectorImage::GetImageContainerAtSize -- 0x7fa238a28bb0 size 1036x691 invalidations 8 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa238a28bb0 size 1036x691 invalidations 4 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa232ec3530 size 1036x1254 invalidations 12 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa232ec3600 size 1036x860 invalidations 4 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa232ec3600 size 1036x860 invalidations 8 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa231afd8b0 size 1036x1037 invalidations 12 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa231afdb20 size 1036x630 invalidations 4 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa231afdb20 size 1036x630 invalidations 8 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa231afdcc0 size 1036x1036 invalidations 4 [AO] VectorImage::GetImageContainerAtSize -- 0x7fa231afdcc0 size 1036x1037 invalidations 8 In an ideal world, each of these accesses would have only caused us to draw once, or 10 redraws. Instead we redraw 8+4+12+4+8+12+4+8+4+8 = 72 times. That is a lot of extra wasted work, especially when it is done on the main thread. (Ignore the off by 1 pixel accesses. Bug 1453747 should address that.)
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
Got to the bottom of the remaining not rendered bug. We cannot update the image container directly from VectorImage::InvalidateObserversOnNextRefreshDriverTick if it is non-animated, because the layout tree state may be inconsistent. If I defer it the problem goes away.
Assignee | ||
Comment 10•5 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c097cce1bbe1e7f21aa8b00713b0fd79d165610f
Assignee | ||
Comment 11•5 years ago
|
||
If a vector image has an image container, it is unlikely the caller will call VectorImage::Draw (and thus Show indirectly) to display the image. As such, WebRender was missing subsequent invalidations and not regenerating the rasterized surface as expected. Thus we now resume honoring the invalidations if we updated the image container.
Assignee | ||
Comment 12•5 years ago
|
||
Originally we would invalidate image containers synchronously when SVGRootRenderingObserver::OnRenderingChange was called. However at this point in time, the layout tree is in the middle of updating its own state, and triggering a paint will be problematic. Animated vector images did not suffer from this problem because they would defer to the next refresh tick, but non-animated do not receive refresh tick events. As such, we should just defer the invalidation to immediately after the layout tree has finished updating itself. Depends on D15667
Comment 13•5 years ago
|
||
Pushed by aosmond@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/325d06d3ba73 Part 1. Ensure we honor invalidations if using vector image containers. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/b709624ce8fa Part 2. Do not invalidate vector image containers synchronously. r=tnikkel
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/325d06d3ba73
https://hg.mozilla.org/mozilla-central/rev/b709624ce8fa
Updated•5 years ago
|
Comment 15•5 years ago
|
||
Please note that the website layout for which this issue was submitted has changed in the meantime. Loading an affected build and setting all the config prefs to the ones mentioned in c3 and the revisiting the website, does not result in any glitchy or faulty image renders.
Taking this into account, how do you advise proceeding further on this issue?
Assignee | ||
Comment 16•5 years ago
|
||
I tried archive.org but it doesn't seem like it shows up correctly. Nor did I save any local copies of the page, since reproducing was very timing sensitive. Given that the bug is very obvious in retrospect (now that the patches have been written), I'm comfortable on passing the verification.
Comment 17•5 years ago
|
||
Thank you for the info. As instructed we will pass the verification on this one.
Description
•