Closed Bug 1106252 Opened 10 years ago Closed 10 years ago

Annoying red eye-candy symbols for not loaded/downloaded images

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Version:
36 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox35 --- unaffected
firefox36 + verified
firefox37 + verified

People

(Reporter: Virtual, Assigned: seth)

References

(
URL
)

Details

(Keywords: nightly-community, regression)

Attachments

(3 files)

bug.png
30.80 KB, image/png
Details
no bug.png
7.06 KB, image/png
Details
Make nsImageFrame's icon loading code more robust
2.41 KB, patch
tnikkel
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
1. Go to http://www.wykop.pl/ 2. Look on thumbnails "Ostatnio popularne" on the bottom-right of the page and force fresh reload the site with CTRL+SHIFT+R 3. Enjoy blinking red eye-candy symbols for not loaded/downloaded images which catches your eyes attention
Component: Untriaged → ImageLib
Product: Firefox → Core
Version: 37 Branch → 36 Branch
[Tracking Requested - why for this release]: Annoying regression
tracking-firefox36: --- → ?
tracking-firefox37: --- → ?
Regression, tracking
tracking-firefox36: ? → +
tracking-firefox37: ? → +
this problem described in this thread also seems to arise from this bug http://forums.mozillazine.org/viewtopic.php?f=23&t=2894375&p=13920599#p13920599
(In reply to ElevenReds from comment #9) > this problem described in this thread also seems to arise from this bug > > http://forums.mozillazine.org/viewtopic. > php?f=23&t=2894375&p=13920599#p13920599 Yup, seems like the same thing. I'm investigating this now.
I've narrowed down the cause here. This issue is caused by a combination of two factors: (1) Bug 1060869 made chrome images discardable, which made us not sync decode them automatically. This was a temporary state of affairs until bug 1104622 landed. I was already planning on disabling that again in a followup, which will fix this problem. (2) The icon loading code is not at all smart. Essentially it's written in such a way that it *relies* on ImageLib automatically decoding the icon images and never discarding them, because it won't draw them if they don't have STATUS_FRAME_COMPLETE, but it never requests that they be decoded. Even though fixing (1) will render it unnecessary, in the interest of robustness I think that we should *also* make the icon loading code smarter. In particular, it should: - Call RequestDecode itself. - Draw if the icon has STATUS_LOAD_COMPLETE. - Draw with FLAG_SYNC_DECODE. These changes are harmless with (1) fixed, but will mean that this code is robust enough to handle chrome images being discardable, if we ever want to experiment with that again. I'm going to make chrome images non-discardable in a separate bug, and then make the changes in (2) in this bug.
No longer blocks: 1057904
Depends on: 1109438
Item (1) is being handled in bug 1109438.
(In reply to ElevenReds from comment #9) > this problem described in this thread also seems to arise from this bug > > http://forums.mozillazine.org/viewtopic. > php?f=23&t=2894375&p=13920599#p13920599 Actually, sorry, I misunderstood what was being reported there initially. That is *not* related to this bug. That PNG has an alpha channel - in other words, it *could* be transparent, although it doesn't have any transparent pixels - and we now draw the white noise background we normally draw under images with an alpha channel as soon as we realize that the alpha channel exists. Before, we waited until the image was completely loaded to draw the white noise background, but that caused a "pop" for images with transparency. That change was made in bug 1089880.
Flags: needinfo?(seth)
Here's the patch for item (2).
Attachment #8534171 - Flags: review?(tnikkel)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Attachment #8534171 - Flags: review?(tnikkel) → review+
Thanks for the review! Locally, the patch in this bug is sufficient by itself to fix the problem. Unfortunately it appears that bug 1109438 triggers a large number of oranges on try. So until I figure out what the cause is there, if the patch in this bug is green by itself I think the best course of action is to go ahead and land it. Here's a new try job which does not include the other patch: https://tbpl.mozilla.org/?tree=Try&rev=6058c378eca6
(In reply to Seth Fowler [:seth] from comment #13) > (In reply to ElevenReds from comment #9) > > this problem described in this thread also seems to arise from this bug > > > > http://forums.mozillazine.org/viewtopic. > > php?f=23&t=2894375&p=13920599#p13920599 > > Actually, sorry, I misunderstood what was being reported there initially. > That is *not* related to this bug. That PNG has an alpha channel - in other > words, it *could* be transparent, although it doesn't have any transparent > pixels - and we now draw the white noise background we normally draw under > images with an alpha channel as soon as we realize that the alpha channel > exists. Before, we waited until the image was completely loaded to draw the > white noise background, but that caused a "pop" for images with > transparency. That change was made in bug 1089880. okay thanks for investigating . so that's intended but still feels odd TBH. sorry thought it was caused because of this bug. & also great to see this bug being investigated and getting fixed too. Thanks
(In reply to ElevenReds from comment #17) > okay thanks for investigating . so that's intended but still feels odd TBH. > sorry thought it was caused because of this bug. > & also great to see this bug being investigated and getting fixed too. > Thanks I'm glad I could help! The white noise background on image documents is not something that you should see a lot. You'd need to be looking at an image with an alpha channel or another source of transparency. That means you'll never see it on JPEGs, and you normally shouldn't see it on PNGs or GIFs that don't have transparency. (The one reported in that forum happens to have an alpha channel unnecessarily. It's odd that such an image would be distributed as PNG rather than JPEG anyway, really.) We will definitely be paying attention and learning from the experience people have with the new behavior, though! The goal is to behave in a way that is a good compromise for the many different kinds of images people want to view on the web.
(In reply to Seth Fowler [:seth] from comment #18) > We will definitely be paying attention and learning from the experience > people have with the new behavior, though! The goal is to behave in a way > that is a good compromise for the many different kinds of images people want > to view on the web. That's the whole point right? And a lot of png (as you told alpha transparency) can be found & it feels as the image is corrupt at first.
(In reply to ElevenReds from comment #19) > That's the whole point right? > And a lot of png (as you told alpha transparency) can be found > & it feels as the image is corrupt at first. Not all PNGs have an alpha channel, and PNGs without any transparent pixels generally shouldn't. So you shouldn't see this effect on most opaque PNGs. At any rate, we should stop discussing this issue in *this* bug, since this bug is about a different issue.
Looks like the try job above is green, so I went ahead and pushed the patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/49a918ec025f
Plattform is set to "x86_64 Windows 7", should that be "All All"? I can see it on Mac OS.
https://hg.mozilla.org/mozilla-central/rev/49a918ec025f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox37: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
OS: Windows 7 → All
Hardware: x86_64 → All
Comment on attachment 8534171 [details] [diff] [review] Make nsImageFrame's icon loading code more robust I request uplift to Aurora/Developer Edition as it also affected.
Attachment #8534171 - Flags: approval-mozilla-aurora?
Virtual, could you fill the uplift form? Thanks
Flags: needinfo?(BernesB)
Comment on attachment 8534171 [details] [diff] [review] Make nsImageFrame's icon loading code more robust Approval Request Comment [Feature/regressing bug #]: Bug 1060869 [User impact if declined]: Annoying and irritating red eye-candy symbols will be flashing and blinking for not loaded/downloaded images when page is loading. [Describe test coverage new/current, TBPL]: This code should be tested through our existing tests. [Risks and why]: This should be pretty low risk. [String/UUID change made/needed]: N/A Please double check it. It's my first time writing and filling a uplift form.
Flags: needinfo?(BernesB)
Comment on attachment 8534171 [details] [diff] [review] Make nsImageFrame's icon loading code more robust Looks great. Thanks!
Attachment #8534171 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks for filling out the uplift form, Virtual!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: