Closed Bug 854287 Opened 7 years ago Closed 7 years ago

Partial images are drawn during redecode on tab switch.

Categories

(Core :: ImageLib, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: rclick, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

STR:
1. load https://upload.wikimedia.org/wikipedia/commons/2/23/Drymoreomys_albimaculatus_002.jpg
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: 
https://hg.mozilla.org/mozilla-central/rev/4d3250f3afea

Win7 x64 
Graphics

        Adapter Description
        ATI Radeon HD 3200 Graphics

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

        Adapter RAM
        256

        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
        0x9610

        Direct2D Enabled
        true

        DirectWrite Enabled
        true (6.2.9200.16492)

        Driver Date
        7-28-2011

        Driver Version
        8.881.0.0

        GPU #2 Active
        false

        GPU Accelerated Windows
        1/1 Direct3D 10

        Vendor ID
        0x1002

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

        AzureCanvasBackend
        direct2d

        AzureContentBackend
        none

        AzureFallbackCanvasBackend
        cairo

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
Status: NEW → ASSIGNED
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: https://tbpl.mozilla.org/?tree=Try&rev=cce661ead65a
And let's give that a third try since infra problems ruined all our test results: https://tbpl.mozilla.org/?tree=Try&rev=c48b1ff9b6d5
That looks much better. I'll push that today if Seth doesn't.
https://hg.mozilla.org/mozilla-central/rev/61711e1aab0c
Status: ASSIGNED → RESOLVED
Closed: 7 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.