1253995 - Flickering/background showing during image rendering

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[MA]

Attachment: flicker_testcase.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160306030215

Steps to reproduce:

I replace the src-attribute of an img-element using the code in the attached testcase. In this minimal testcase I merely append a question mark to have the browser treat the same image as a new one, but the same result can also be seen repeatedly when switching between multiple different images.


Actual results:

The image flickers during the transition from the first image to the second, leaving the background briefly exposed.

This appears to be a regression in Firefox 47. Firefox pre-47, Safari and Chrome does not show this problem. I used OS X 10.11 for testing.


Expected results:

The transition from the first image to the second should be seamless.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 2

[Alice0775 White]

I can reproduce on Windows7. 
But I got a different regression window.
So, I filed a new Bug 1254318.

Comment 3

[Seth Fowler [:seth] [:s2h]]

We don't want to sync decode images in this case, because it will force image decoding to run on the main thread for many common patterns. (For example, sites often provide their own "placeholder" image and then switch the src when the image loads; if we sync decoded in this case, we'd sync decode every image on the page on the main thread on such sites.)

In theory what we *could* do is that if the dimensions of the image have not changed, we could continue painting the old image until the new one has decoded. If the dimensions have changed, that's not possible, because we can't allow the rendered page to get out of sync with the DOM.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Recent regression in 47, tracked.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1253995.patch

Something like this?

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Ritu Kothari (:ritu) from comment #4)
> Recent regression in 47, tracked.

FWIW I'm skeptical that this can't be reproduced in versions before 47, but it is probably easier now due to timing changes.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8729012 [details] [diff] [review]
1253995.patch

Review of attachment 8729012 [details] [diff] [review]:
-----------------------------------------------------------------

Some minor tweaks, but looks good!

::: layout/generic/nsImageFrame.cpp
@@ +683,5 @@
>  
>    if (mState & IMAGE_GOTINITIALREFLOW) { // do nothing if we haven't gotten the initial reflow yet
>      if (intrinsicSizeChanged) {
>        if (!(mState & IMAGE_SIZECONSTRAINED)) {
> +        mPrevImage = nullptr;

I think you should move this above the |mState & IMAGE_GOTINITIALREFLOW| check and just do it whenever |intrinsicSizeChanged| is true.

Also, do the same thing in |OnSizeAvailable|.

@@ +1492,5 @@
>  
>    DrawResult result = static_cast<nsImageFrame*>(mFrame)->
>      PaintImage(*aCtx, ToReferenceFrame(), mVisibleRect, mImage, flags);
>  
> +  if (result == DrawResult::NOT_READY || result == DrawResult::INCOMPLETE) {

I'd probably also do it for TEMPORARY_ERROR.

@@ +1777,5 @@
>      nsLayoutUtils::InitDashPattern(strokeOptions, NS_STYLE_BORDER_STYLE_DOTTED);
>      map->Draw(this, *drawTarget, black, strokeOptions);
>    }
>  
> +  if (result == DrawResult::SUCCESS) {

You should clear mPrevImage on DrawResult::BAD_IMAGE, which is a permanent condition as well.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1253995.patch

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8730660 [details] [diff] [review]
1253995.patch

Review of attachment 8730660 [details] [diff] [review]:
-----------------------------------------------------------------

So this looks good, but I think you need one more change: in OnSizeAvailable() and NotifyNewCurrentRequest(), where it says "We no longer have a valid image" and we set mImage to null, I think you want to set mPrevImage to null as well, because "the image" effectively *has* loaded, it's just that "the image" is nothing in this case.

r+ with that change. If you disagree with that change, let me know why and rerequest review.

::: layout/generic/nsImageFrame.h
@@ +409,5 @@
>    nsDisplayImage(nsDisplayListBuilder* aBuilder, nsImageFrame* aFrame,
> +                 imgIContainer* aImage, imgIContainer* aPrevImage)
> +    : nsDisplayImageContainer(aBuilder, aFrame)
> +    , mImage(aImage)
> +    , mPrevImage(aPrevImage) {

Please move this brace down to the next line.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Oh BTW Edwin, I'm somewhat concerned that this patch has the risk of regressing memory usage. It *shouldn't*, but this is the kind of thing that historically *has* when it has second-order effects that we haven't considered.

You should consider pushing an AWSY try run, or if not, at least monitor AWSY and make sure that there's no regression.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Edwin, any news on this patch? It'd be nice to get this landed just because there are a lot of flickering bugs at the moment and I'd like to make sure we don't waste time debugging the same issue in different bugs.

Comment 13

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #12)
> Edwin, any news on this patch? It'd be nice to get this landed just because
> there are a lot of flickering bugs at the moment and I'd like to make sure
> we don't waste time debugging the same issue in different bugs.

Slight bump on AWSY (about 12MB). Will push again today with your review comments and see if that helps at all (*shrug*, might do) or at least for one more data point in case it's just noise.

Comment 14

[Seth Fowler [:seth] [:s2h]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #13)
> (In reply to Seth Fowler [:seth] [:s2h] from comment #12)
> > Edwin, any news on this patch? It'd be nice to get this landed just because
> > there are a lot of flickering bugs at the moment and I'd like to make sure
> > we don't waste time debugging the same issue in different bugs.
> 
> Slight bump on AWSY (about 12MB). Will push again today with your review
> comments and see if that helps at all (*shrug*, might do) or at least for
> one more data point in case it's just noise.

Yeah, definitely try again with my comments addressed.

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Similar story; about +9MB at peak.

The first point is after the patch is applied, the second point is before the patch is applied, and the last point is the patch with review comments:
https://areweslimyet.com/?series=edwin&evenspacing

Thoughts? Is that bump acceptable?

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #15)
> Similar story; about +9MB at peak.
> 
> The first point is after the patch is applied, the second point is before
> the patch is applied, and the last point is the patch with review comments:
> https://areweslimyet.com/?series=edwin&evenspacing
> 
> Thoughts? Is that bump acceptable?

I can see why this patch might cause a regression on benchmarks around the time of page load, but it's a little concerning that we see the same regression after 30s, and especially after tabs are closed.

Also, the regression does not seem to be in image memory that we're tracking explicitly in the memory reporter (where I'd expect it) but rather in heap-unclassified.

I think you should investigate this further, unfortunately.

Comment 17

[Stig Nygaard]

FYI. From description of this bug, it looks like the behavior I see introduced in Firefox version 45.

Comment 18

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Weird. I ran AWSY again for more data and got totally different, somewhat reasonable results. Will have to land and see to get any meaningful data, I think.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e12df7cefe4

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e12df7cefe4

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Edwin, Seth, this is a recent regression in Fx47. Is this something that is stable and ready to be uplifted to Beta47?

Comment 22

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8730660 [details] [diff] [review]
1253995.patch

Approval Request Comment
[Feature/regressing bug #]: Fallout from bug 1254318
[User impact if declined]: Rendering artefacts when scripts are changing image sources 
[Describe test coverage new/current, TreeHerder]: On m-c for over a week
[Risks and why]: Very little
[String/UUID change made/needed]: None

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730660 [details] [diff] [review]
1253995.patch

Recent regression in Fx47, baked in Nightly for a week, Beta47+

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/205aa4a0a839

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

The transition is performed smoothly on Windows 7 x64 and Mac OS X 10.9.5 using the following builds: 
- Firefox 47 beta 4, build ID: 20160509171155
- Latest 48.0a2, build ID: 20160511004106