511899 - disable discarding on a per-image basis when it isn't an obvious win

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

spacer.gif is a good example of this. Should be easy to fix after bug 435296 lands.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch v1

Added a patch for this. I realized that it was probably worth having a threshold of when we want to keep discarding on, since it's probably not worth all the discard overhead just to save 500 bytes. I set this threshold to 8k, but I'm open to suggestions.

Also, I realized that we weren't throwing away source data for non-discardable decode-on-draw images. I was tempted to add that to bug 435296, but I figured it was probably better to do here.

I'll flag for review when bug 435296 settles.

Comment 2

[Tomas]

It is time to flag?

Comment 3

[Bobby Holley (:bholley)]

soon, but not yet.

Comment 4

[Bobby Holley (:bholley)]

Just as an update - I actually spent some time working on this again at the beginning of last week.

In order for this to work right currently, the check needs to go in two places, because sometimes we call DecodingCompete() first, and sometimes we call SourceDataCompete() first, and this check needs to happen during the second one, whichever it is.

I talked with joe and jeff about it for a bit, and our eventual conclusion was that it would be better to wait until we fix bug 513681, where we can guarantee consistent decoding behavior across all the decoders and thus only put the check in one place.

Marking the dependency.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This probably should block since it'll fix some annoying behavior with favicons.

Comment 6

[Bobby Holley (:bholley)]

This should be easy to fix for 2.0, since we're already fixing bug 513681. So I agree that it should block.

Comment 7

[Joe Drew (not getting mail)]

This can't be guaranteed to fix favicon issues.

I think I would release without this fix, but I would take the patch.

Comment 8

[Jim Jeffery not reading bug-mail 1/2/11]

I thought this was supposed to fix the flicker when switching tabs that was introduced when image.mem.discardable was set as default a week or so ago.

Currently to stop the flash/flicker on tab-switch users are setting the pref back to false. 

It was turned back on in https://bugzilla.mozilla.org/show_bug.cgi?id=435296 apparently with the knowledge that it would cause flicker, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=435296#c17

The flicker and repaints in forums when switching away from the tab and back after s short period of time is annoying and looks bad, where with the pref disabled there is no flicker or repaints.

So, this bug needs to fix the flicker or bug 435296 backed out.

Comment 9

[Joe Drew (not getting mail)]

With the pref disabled we do not discard and use more memory. The "flickers" you talk about are us redecoding images we have discarded. You didn't see this in Firefox 3.x because we redecoded synchronously, meaning your browser was less responsive.

Comment 10

[Bobby Holley (:bholley)]

Let's move this discussion over to bug 516320.

Comment 11

[Bas Schouten (:bas.schouten)]

Joe, do we intend to do anything with this bug? 516320 appears closed.

Comment 12

[Joe Drew (not getting mail)]

This could be an easy win for tiny images. Putting it on memshrink's radar.

Note that, rather than a "minimum win," we should probably check that the source data is "sufficiently" smaller than the decoded data. For example, if the source data is half the size of the decoded data, it's probably not worthwhile to hold on to it.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 12 votes.
:aosmond, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 17

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.