Closed Bug 1253995 Opened 4 years ago Closed 4 years ago

Flickering/background showing during image rendering

Categories

(Core :: ImageLib, defect)

47 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 + verified
firefox48 --- verified

People

(Reporter: bugzilla, Assigned: eflores)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

Attached file 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.
Keywords: regression
[Tracking Requested - why for this release]: Regression
Status: UNCONFIRMED → NEW
Has Regression Range: --- → no
Has STR: --- → yes
Component: General → ImageLib
Ever confirmed: true
I can reproduce on Windows7. 
But I got a different regression window.
So, I filed a new Bug 1254318.
See Also: → 1254318
See Also: → 1253386
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.
Assignee: nobody → edwin
Recent regression in 47, tracked.
Attached patch 1253995.patch (obsolete) — Splinter Review
Something like this?
Attachment #8729012 - Flags: feedback?(seth)
(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 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.
Attachment #8729012 - Flags: feedback?(seth) → feedback+
Attached patch 1253995.patchSplinter Review
Attachment #8729012 - Attachment is obsolete: true
Attachment #8730660 - Flags: review?(seth)
Duplicate of this bug: 1254318
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.
Attachment #8730660 - Flags: review?(seth) → review+
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(edwin)
(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.
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?
Flags: needinfo?(seth)
(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.
Flags: needinfo?(seth)
FYI. From description of this bug, it looks like the behavior I see introduced in Firefox version 45.
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.
https://hg.mozilla.org/mozilla-central/rev/3e12df7cefe4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi Edwin, Seth, this is a recent regression in Fx47. Is this something that is stable and ready to be uplifted to Beta47?
Flags: needinfo?(seth)
Flags: needinfo?(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
Flags: needinfo?(seth)
Flags: needinfo?(edwin)
Attachment #8730660 - Flags: approval-mozilla-beta?
Comment on attachment 8730660 [details] [diff] [review]
1253995.patch

Recent regression in Fx47, baked in Nightly for a week, Beta47+
Attachment #8730660 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Depends on: 1286836
Depends on: 1327048
You need to log in before you can comment on or make changes to this bug.