Closed Bug 854287 Opened 9 years ago Closed 9 years ago

Partial images are drawn during redecode on tab switch.


(Core :: ImageLib, defect)

Windows 8
Not set





(Reporter: rclick, Assigned: seth)



(Keywords: regression)


(1 file, 2 obsolete files)

1. load
2. Switch to a different tab and wait 20 seconds.
3. Switch back to the image.

I'm seeing a partial image getting drawn immediately and the rest of the image being drawn a bit later (presumably when the decode finishes). Prior to Bug 716140's patches, we would initially display nothing then draw the entire image all at once.
Confirmed with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130324 Firefox/22.0 ID:20130324105508 CSet: 87b2de1db6fa.
Keywords: regression
This should be very easy to solve by simply not doing subframe invalidations if mHasBeenDecoded. Plumbing that through to the appropriate spot (imgStatusTracker::SyncNotifyDiff) might be fun, though.
I am unable to reproduce this issue.  The image appears so fast I see no painting here, unless the image is never being unloaded for some reason.  Don't know how to check that.

tested with latest hourly build cset:

Win7 x64 

        Adapter Description
        ATI Radeon HD 3200 Graphics

        Adapter Drivers
        aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64

        Adapter RAM

        ClearType Parameters
        DISPLAY1 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 300 ] DISPLAY4 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 50 ]

        Device ID

        Direct2D Enabled

        DirectWrite Enabled
        true (6.2.9200.16492)

        Driver Date

        Driver Version

        GPU #2 Active

        GPU Accelerated Windows
        1/1 Direct3D 10

        Vendor ID

        WebGL Renderer
        Google Inc. -- ANGLE (ATI Radeon HD 3200 Graphics)




Also tested in clean profile, with Azure.content enabled and still image appears instantly when switching back from another tab.
Duplicate of this bug: 853564
Is this actually a bug? I am going ahead and cooking up a patch anyway, but really: is there a problem here? Is displaying nothing until the full image snaps into view really in any way better than displaying the image incrementally as it decodes?
(As an aside, although it does load _very_ quickly, I can reproduce this on OS X. I see the partial image only for a split second.)
Alex, I'd love to get an opinion on this from a UX perspective.
Flags: needinfo?(limi)
Here's a proposed patch. Before we decide to apply this, though, I'd really like to hear from the UX folks.
Assignee: nobody → seth
Attachment #734146 - Flags: review?(joe)
Comment on attachment 734146 [details] [diff] [review]
Don't send partial invalidations for images which have already been decoded.

Review of attachment 734146 [details] [diff] [review]:

I would much, much rather this be something the imgStatusTracker "knows" inherently. It should be able to know it, simply because it does know when decodes are done. And that should clean things up nicely and make this more self-contained.

::: image/src/imgStatusTracker.h
@@ +205,5 @@
>    };
>    // Calculate the difference between this and other, apply that difference to
>    // ourselves, and return it for passing to SyncNotifyDifference.
> +  StatusDiff CalculateAndApplyDifference(imgStatusTracker* other, bool aIgnoreInvalidations);

In general, booleans aren't optimal for this; I'd much rather have an enumerated type with a default value. But that's only FYI, due to other feedback.
Attachment #734146 - Flags: review?(joe) → review-
We explicitly made this choice before, in a lot of places, and with UX (specifically Beltzner)'s specific request; it's a bug that this regressed.
Flags: needinfo?(limi)
Thanks for the review, Joe. Reimplemented along the lines you requested. I agree that this is much cleaner. The only downside is that I have a harder time convincing myself it's 100% correct. Let me know if you find any issues.
Attachment #738840 - Flags: review?(joe)
Attachment #734146 - Attachment is obsolete: true
Comment on attachment 738840 [details] [diff] [review]
Don't send partial invalidations for images which have already been decoded.

Review of attachment 738840 [details] [diff] [review]:

::: image/src/imgStatusTracker.cpp
@@ +559,5 @@
> +  // decoding right now. This avoids displaying partially decoded images to the
> +  // user if they're being re-decoded due to discarding.
> +  bool skipInvalidations = mHasBeenDecoded &&
> +                           !(mImageStatus & imgIRequest::STATUS_ERROR) &&
> +                           !(mImageStatus & imgIRequest::STATUS_DECODE_COMPLETE);

It might be nicer if this was "doInvalidations" instead of "skipInvalidations", but only change it if you agree.
Attachment #738840 - Flags: review?(joe) → review+
Thanks for the review!

(In reply to Joe Drew (:JOEDREW! \o/) from comment #14)
> It might be nicer if this was "doInvalidations" instead of
> "skipInvalidations", but only change it if you agree.

Yeah, I think that might read a bit nicer. I'll get an updated patch up shortly.
Updated patch. Will run through try one more time.
Attachment #738840 - Attachment is obsolete: true
Hm, there are some unfortunate timeouts there. It seems very unlikely that this could cause them, but let's be extra-special-careful:
And let's give that a third try since infra problems ruined all our test results:
That looks much better. I'll push that today if Seth doesn't.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 866959
You need to log in before you can comment on or make changes to this bug.