Image was not rendered or flickered on Italic home page

RESOLVED FIXED in Firefox 66

Status

()

P3
normal
RESOLVED FIXED
4 months ago
16 days ago

People

(Reporter: sotaro, Assigned: aosmond)

Tracking

(Blocks: 1 bug, {regression})

unspecified
mozilla66
Unspecified
All
regression
Points:
---

Firefox Tracking Flags

(firefox-esr60 disabled, firefox63 disabled, firefox64 disabled, firefox65 wontfix, firefox66 fixed)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

4 months ago
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

4 months ago
(Reporter)

Updated

4 months ago
See Also: → bug 1509244
(Reporter)

Comment 1

4 months 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

4 months ago
see also bug 1508127
(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.
Blocks: 1386669
status-firefox63: --- → disabled
status-firefox64: --- → disabled
status-firefox65: --- → affected
status-firefox-esr60: --- → disabled
Keywords: regression
OS: Unspecified → All
Priority: -- → P3
(Assignee)

Updated

3 months ago
Assignee: nobody → aosmond
(Assignee)

Comment 4

3 months 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

3 months 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

3 months 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)

Updated

3 months ago
Duplicate of this bug: 1510306
(Assignee)

Comment 8

3 months 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.)
status-firefox65: affected → fix-optional
status-firefox66: --- → affected
(Assignee)

Comment 9

3 months 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 11

3 months 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

3 months 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

2 months 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

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
status-firefox65: fix-optional → wontfix

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?

Flags: needinfo?(aosmond)
(Assignee)

Comment 16

17 days 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.

Flags: needinfo?(aosmond)

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

You need to log in before you can comment on or make changes to this bug.