1510139 - Image was not rendered or flickered on Italic home page

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Sotaro Ikeda [:sotaro]]

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.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 2

[Mayank Bansal]

see also bug 1508127

Comment 3

[Darkspirit]

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

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c097cce1bbe1e7f21aa8b00713b0fd79d165610f

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1510139 - Part 1. Ensure we honor invalidations if using vector image containers.

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.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1510139 - Part 2. Do not invalidate vector image containers synchronously.

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

[Pulsebot]

Pushed by [e-mail]:
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

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/325d06d3ba73
https://hg.mozilla.org/mozilla-central/rev/b709624ce8fa

Comment 15

[Cristian Baica [:cbaica], Release Desktop QA]

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?

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

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

[Cristian Baica [:cbaica], Release Desktop QA]

Thank you for the info. As instructed we will pass the verification on this one.