Closed Bug 854287 Opened 7 years ago Closed 7 years ago
Partial images are drawn during redecode on tab switch
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.
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.
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.
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)
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=3c11f708c9f9
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.
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
New try job here: https://tbpl.mozilla.org/?tree=Try&rev=0e0e05c15122
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
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=64620dd73baa
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.
Seth's on PTO, so: https://hg.mozilla.org/integration/mozilla-inbound/rev/61711e1aab0c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.