1446309 - Some images are periodically flickering white in WebRender

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

白くちらつく

Comment 2

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

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.

Comment 3

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

Attachment: 0001-Bug-1446309-Part-1.-Add-support-to-getting-an-image-.patch, v1

Comment 4

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

Attachment: 0002-Bug-1446309-Part-2.-Make-nsDisplayImage-support-usin.patch, v1

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.

Comment 5

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

Attachment: 0001-Bug-1446309-Part-1.-Add-imgIContainer-GetImageContai.patch, v2

Comment 6

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

Attachment: 0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v2

This implementation approach makes more sense.

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

Comment 7

[Timothy Nikkel (:tnikkel)]

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.

Comment 8

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

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

Comment 10

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

Attachment: 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v3

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.

Comment 11

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

Attachment: 0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v3

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

Comment 12

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

Attachment: 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v4

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.

Comment 13

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

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

Comment 14

[Pulsebot]

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

Comment 15

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

(In reply to Pulsebot from comment #14)
> Pushed by [e-mail]:
> 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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44b01a42d8c
Backed out changeset 3f41992ab77e because wrong bug number. r=backout

Comment 17

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

Comment on attachment 8998863 [details] [diff] [review]
0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v3

Should incorporate your feedback.

Comment 18

[Timothy Nikkel (:tnikkel)]

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?

Comment 19

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

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

Comment 20

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

Attachment: 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v5

Comment 21

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

Attachment: 0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v4

Comment 22

[Timothy Nikkel (:tnikkel)]

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.

Comment 23

[Timothy Nikkel (:tnikkel)]

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*

Comment 24

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

Attachment: 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v6 [carries r=tnikkel]

I made GetImageContainerSize return a draw result so that we can return that instead of NOT_READY. Renamed aContainer to aOutContainer.

Comment 25

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

Attachment: 0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v5 [carries r=tnikkel]

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

Comment 26

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

Attachment: 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v7 [carries r=tnikkel]

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

Comment 27

[Pulsebot]

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

[Andreea Pavel [:apavel]]

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

Comment 29

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

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

Comment 30

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

Attachment: 0003-Bug-1446309-Part-3.-Properly-handle-ImgDrawResult-fo.patch, v1

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

I'll put it up for review when it/if finishes green.

Comment 31

[Timothy Nikkel (:tnikkel)]

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.

Comment 32

[Maire Reavy [:mreavy]]

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.

Comment 33

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

Attachment: 0001-Bug-1446309-Part-1.-Return-draw-result-from-imgICont.patch, v8 [carries r=tnikkel]

Rebase.

Comment 34

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

Attachment: 0002-Bug-1446309-Part-2.-Make-nsDisplayImage-fallback-to-.patch, v6 [carries r=tnikkel]

Rebase.

Comment 35

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

Attachment: 0003-Bug-1446309-Part-3.-Properly-handle-ImgDrawResult-fo.patch, v2

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

Comment 36

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

Whoops, it didn't like my try syntax: https://treeherder.mozilla.org/#/jobs?repo=try&revision=708ee279ba2e901955125534f3122fa88dca6aee

Comment 37

[Pulsebot]

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

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d10ab3f7b4ea
https://hg.mozilla.org/mozilla-central/rev/2ca405997ed5
https://hg.mozilla.org/mozilla-central/rev/86b053d52632