Closed
Bug 1446309
Opened 7 years ago
Closed 6 years ago
Some images are periodically flickering white in WebRender
Categories
(Core :: Graphics: WebRender, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | --- | unaffected |
firefox62 | --- | disabled |
firefox64 | --- | fixed |
People
(Reporter: alice0775, Assigned: aosmond)
References
(Blocks 1 open bug, )
Details
Attachments
(3 files, 13 obsolete files)
(deleted),
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
Steps To Reproduce:
1. Open http://www.insecam.org/en/byrating/
2. Some IP cameras images periodically flicker if WebRender is enabled.
http://www.insecam.org/en/view/511950/
http://www.insecam.org/en/view/522101/
http://www.insecam.org/en/view/439726/
etc...
Assignee | ||
Comment 2•7 years ago
|
||
It looks like the src is changing. This may be related to:
https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/layout/generic/nsImageFrame.cpp#1551
On the non-WebRender path we would paint the previous image if the new image isn't ready. We probably want to do something similar for the WebRender path.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → aosmond
Updated•7 years ago
|
Blocks: webrender-site-issues
Has STR: --- → yes
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox61:
--- → unaffected
status-firefox-esr52:
--- → unaffected
OS: Windows 10 → All
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
These have gotten ugly as I've learned about and covered the corner cases. It is starting to feel like I should just expose the actual draw result and modify the GetImageContainerAtSize API (or add another API to get the latest draw result).
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e006d83783cd696d91771f386a9dcee8e2fd8c1f
It does however solve the flickering at the given links.
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8960718 -
Attachment is obsolete: true
Attachment #8960954 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•7 years ago
|
||
This implementation approach makes more sense.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3319922e844f421829f1535ffbc7d52a16d40577
Attachment #8960721 -
Attachment is obsolete: true
Attachment #8960955 -
Flags: review?(tnikkel)
Comment 7•7 years ago
|
||
Comment on attachment 8960954 [details] [diff] [review]
0001-Bug-1446309-Part-1.-Add-imgIContainer-GetImageContai.patch, v2
Can we return the draw result from the getImageContainer(AtSize) call somehow? An out param? Return a tuple? This seems a little hacky.
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #7)
> Comment on attachment 8960954 [details] [diff] [review]
> 0001-Bug-1446309-Part-1.-Add-imgIContainer-GetImageContai.patch, v2
>
> Can we return the draw result from the getImageContainer(AtSize) call
> somehow? An out param? Return a tuple? This seems a little hacky.
Okay. In parallel, I've been looking at stopping OrientedImage and ClippedImage from using fallback with WebRender. It looks like I need something a little more like CreateWebRenderCommands (to receive the stacking context and builder, to make appropriate adjustments...) for each image type anyways, so I'll try to incorporate that into the updated patches.
Assignee | ||
Updated•7 years ago
|
Attachment #8960954 -
Flags: review?(tnikkel)
Assignee | ||
Updated•7 years ago
|
Attachment #8960955 -
Flags: review?(tnikkel)
Updated•7 years ago
|
Blocks: stage-wr-trains
Priority: -- → P1
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Version: 61 Branch → 63 Branch
Assignee | ||
Comment 10•6 years ago
|
||
Incorporate review feedback. The CreateWebRenderCommands path is proving tricky given there is state inside nsDisplayFrame (mPrevImage) that needs to be managed by imagelib now, and it may perform several passes of CreateWebRenderCommands to get there. For now I'm just going to return the draw result with GetImageContainerAtSize as originally suggested. This will get this working. OrientedImage and ClippedImage not using fallback isn't as important because they appear fine with blob images -- if perhaps not as efficient as we could be.
Attachment #8960954 -
Attachment is obsolete: true
Assignee | ||
Comment 11•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e6af9dfeeb0aba580d0df088a9313ee73bc1ace
Attachment #8960955 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
The image crashes don't really make any sense from the try run. It might be related to strict IDL ordering requirements and the fact I had to convert GetImageContainerAtSize to a {%C++ .. %} entry because of some macro expansion weirdness with the Tuple datatype. I moved it to the end with the rest of the native C++ declarations, hopefully that solves it.
Attachment #8998862 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
Comment 14•6 years ago
|
||
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f41992ab77e
Store a shared surface's dirty rect update if we cannot process it immediately. r=nical
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Pulsebot from comment #14)
> Pushed by aosmond@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3f41992ab77e
> Store a shared surface's dirty rect update if we cannot process it
> immediately. r=nical
Ugh I checked this in with the wrong bug in the description.
Comment 16•6 years ago
|
||
Backout by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44b01a42d8c
Backed out changeset 3f41992ab77e because wrong bug number. r=backout
Assignee | ||
Comment 17•6 years ago
|
||
Comment on attachment 8998863 [details] [diff] [review]
0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v3
Should incorporate your feedback.
Attachment #8998863 -
Flags: review?(tnikkel)
Assignee | ||
Updated•6 years ago
|
Attachment #8998939 -
Flags: review?(tnikkel)
Comment 18•6 years ago
|
||
Comment on attachment 8998939 [details] [diff] [review]
0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v4
So the thing I don't like about this approach is that generally we want to &= draw results and combine them instead of assign to them. Using a tuple for the return type makes this take an extra step/line.
We could return just a draw result and make the imgcontainer and outparam? Or pass the draw result as an in/out param? Although that seems kinda bad since GetImageContainerAtSize seems more like the origin of a draw result rather than a place that updates one.
What do you think we should do?
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #18)
> Comment on attachment 8998939 [details] [diff] [review]
> 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v4
>
> So the thing I don't like about this approach is that generally we want to
> &= draw results and combine them instead of assign to them. Using a tuple
> for the return type makes this take an extra step/line.
>
> We could return just a draw result and make the imgcontainer and outparam?
> Or pass the draw result as an in/out param? Although that seems kinda bad
> since GetImageContainerAtSize seems more like the origin of a draw result
> rather than a place that updates one.
>
> What do you think we should do?
I will make it return the draw result and yield the ImageContainer as an outparam. That seems the most natural.
Assignee | ||
Comment 20•6 years ago
|
||
Attachment #8998939 -
Attachment is obsolete: true
Attachment #8998939 -
Flags: review?(tnikkel)
Attachment #8999599 -
Flags: review?(tnikkel)
Assignee | ||
Comment 21•6 years ago
|
||
Attachment #8998863 -
Attachment is obsolete: true
Attachment #8998863 -
Flags: review?(tnikkel)
Attachment #8999600 -
Flags: review?(tnikkel)
Comment 22•6 years ago
|
||
Comment on attachment 8999599 [details] [diff] [review]
0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v5
Review of attachment 8999599 [details] [diff] [review]:
-----------------------------------------------------------------
::: image/Image.cpp
@@ +121,5 @@
> "Unsupported flag passed to GetImageContainer");
>
> IntSize size = GetImageContainerSize(aManager, aSize, aFlags);
> if (size.IsEmpty()) {
> + return ImgDrawResult::NOT_READY;
Looking at the reasons GetImageContainerSize returns an empty size not all of them seem like they are NOT_READY. A lot of them seem like BAD_ARGS or NOT_SUPPORTED.
::: image/ImgDrawResult.h
@@ +57,5 @@
> NOT_READY,
> TEMPORARY_ERROR,
> BAD_IMAGE,
> + BAD_ARGS,
> + NOT_SUPPORTED
Seems like we should handle NOT_SUPPORTED in the same way as BAD_ARGS here:
https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/widget/nsBaseDragService.cpp#792
::: image/imgIContainer.idl
@@ +321,5 @@
> + [noscript, notxpcom] ImgDrawResult getImageContainerAtSize(in LayerManager aManager,
> + [const] in nsIntSize aSize,
> + [const] in MaybeSVGImageContext aSVGContext,
> + in uint32_t aFlags,
> + out ImageContainer aContainer);
aOutContainer might be more clear.
Attachment #8999599 -
Flags: review?(tnikkel) → review+
Comment 23•6 years ago
|
||
Comment on attachment 8999600 [details] [diff] [review]
0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v4
Review of attachment 8999600 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/generic/nsImageFrame.cpp
@@ +1816,5 @@
> ImgDrawResult drawResult =
> mImage->GetImageContainerAtSize(aManager, decodeSize, svgContext,
> flags, getter_AddRefs(container));
> +
> + // While we got a container, it may contain a fully decoded surface. If that
may *not*
Attachment #8999600 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 24•6 years ago
|
||
I made GetImageContainerSize return a draw result so that we can return that instead of NOT_READY. Renamed aContainer to aOutContainer.
Attachment #8999599 -
Attachment is obsolete: true
Attachment #8999932 -
Flags: review+
Assignee | ||
Comment 25•6 years ago
|
||
The changes to GetImageContainerSize returning a draw code are a good correction, and now I need run the tests to make sure I didn't break anything as a result...
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56c97b6ee68c8746c87da7b424b844c5346aca25
Attachment #8999600 -
Attachment is obsolete: true
Attachment #8999934 -
Flags: review+
Assignee | ||
Comment 26•6 years ago
|
||
I say it might break something, and then realize that if we check that the image has a size *after* checking if the given size is valid, we will return BAD_ARGS when the caller just used the image's own (but unknown) size for GetImageContainerImpl.
Swap the order of those:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=193c18a87d87d75342cb770bd8ddece974e0491f
Attachment #8999932 -
Attachment is obsolete: true
Attachment #8999937 -
Flags: review+
Comment 27•6 years ago
|
||
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a4c2a0aac6
Part 1. Return draw result from imgIContainer::GetImageContainerAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e3f5d8e294
Part 2. Make nsDisplayImage fallback to the previous image to avoid flickering. r=tnikkel
Comment 28•6 years ago
|
||
Backed out for build bustages at builds/worker/workspace/build/src/image/RasterImage.cpp
Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=45e3f5d8e29402e8fda2fea772dc5d08d8866fd7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193884247&repo=mozilla-inbound&lineNumber=22548
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5589d9ec31aabd93c3b1b20f2517b96ec78ef752
Flags: needinfo?(aosmond)
Assignee | ||
Comment 29•6 years ago
|
||
Looking into the backout, the draw result really ought to be propagated up to the display item so that it can cache the value via UpdateDrawResult. This might explain some intermittent reftest failures wrt to images, as this is used to force sync decoding in that case. I also wonder if we are triggering fallback when we needed as an intermediate state in some cases, so it looks like this bug will have a part 3...
Flags: needinfo?(aosmond)
Assignee | ||
Comment 30•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a9c26c72250f6ef62a55b2712ab542f2cec0531
I'll put it up for review when it/if finishes green.
Assignee | ||
Updated•6 years ago
|
Attachment #9001250 -
Flags: review?(tnikkel)
Comment 31•6 years ago
|
||
Comment on attachment 9001250 [details] [diff] [review]
0003-Bug-1446309-Part-3.-Properly-handle-ImgDrawResult-fo.patch, v1
Review of attachment 9001250 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/xul/nsImageBoxFrame.cpp
@@ +571,2 @@
> CreateWebRenderCommands(aBuilder, aResources, aSc, aManager, this, ToReferenceFrame(), flags);
> + if (result == ImgDrawResult::NOT_SUPPORTED) {
Based on this it seems that NOT_SUPPORTED needs to take precedence over all others in the DrawResult combining rules.
It feels like we are bundling up a lot of meaning into ImgDrawResults now, it gives me a vague uneasy feeling.
Updated•6 years ago
|
Attachment #9001250 -
Flags: review?(tnikkel)
Comment 32•6 years ago
|
||
We can't release this to the field, but we can let this ride to beta. Someone changing the src value on an image isn't that common.
I know Andrew is close to landing this. I'd certainly like to have it for Beta, and in the interest of not letting these patches bitrot, Andrew should probably push forward after his pto and finish this off.
Priority: P1 → P2
Assignee | ||
Comment 33•6 years ago
|
||
Rebase.
Attachment #8999937 -
Attachment is obsolete: true
Attachment #9008910 -
Flags: review+
Assignee | ||
Comment 34•6 years ago
|
||
Rebase.
Attachment #8999934 -
Attachment is obsolete: true
Attachment #9008911 -
Flags: review+
Assignee | ||
Comment 35•6 years ago
|
||
As discussed offline, I added an assert to nsImageGeometryMixin::UpdateDrawResult to ensure NOT_SUPPORTED is never received (as it should have been handled by fallback, always). I also updated the ImgDrawResult operator& method to always prefer NOT_SUPPORTED if either aLeft or aRight is set to it.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1fd31402c496a949db504e39dbb417eac80c99d
Attachment #9001250 -
Attachment is obsolete: true
Attachment #9008913 -
Flags: review?(tnikkel)
Assignee | ||
Comment 36•6 years ago
|
||
Whoops, it didn't like my try syntax: https://treeherder.mozilla.org/#/jobs?repo=try&revision=708ee279ba2e901955125534f3122fa88dca6aee
Updated•6 years ago
|
Attachment #9008913 -
Flags: review?(tnikkel) → review+
Comment 37•6 years ago
|
||
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d10ab3f7b4ea
Part 1. Return draw result from imgIContainer::GetImageContainerAtSize. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ca405997ed5
Part 2. Make nsDisplayImage fallback to the previous image to avoid flickering. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b053d52632
Part 3. Properly handle ImgDrawResult for WebRender display list generation. r=tnikkel
Comment 38•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d10ab3f7b4ea
https://hg.mozilla.org/mozilla-central/rev/2ca405997ed5
https://hg.mozilla.org/mozilla-central/rev/86b053d52632
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•