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

ASSIGNED
Unassigned

Status

()

Core
ImageLib
ASSIGNED
9 years ago
5 years ago

People

(Reporter: bholley, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
spacer.gif is a good example of this. Should be easy to fix after bug 435296 lands.
(Reporter)

Updated

9 years ago
Summary: disable discarding for images when sizeof(sourceData) > sizeof(decodedFrames) → disable discarding on a per-image basis when it isn't an obvious win
(Reporter)

Comment 1

9 years ago
Created attachment 396832 [details] [diff] [review]
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.
Assignee: nobody → bobbyholley
Status: NEW → ASSIGNED

Comment 2

9 years ago
It is time to flag?
(Reporter)

Comment 3

9 years ago
soon, but not yet.
(Reporter)

Comment 4

8 years ago
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.
Depends on: 513681
This probably should block since it'll fix some annoying behavior with favicons.
blocking2.0: --- → ?
(Reporter)

Comment 6

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

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
This can't be guaranteed to fix favicon issues.

I think I would release without this fix, but I would take the patch.
blocking2.0: ? → -
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.
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.
(Reporter)

Comment 10

8 years ago
Let's move this discussion over to bug 516320.
(Reporter)

Updated

8 years ago
Blocks: 516320
No longer depends on: 435296, 513681
(Reporter)

Updated

7 years ago
Assignee: bobbyholley+bmo → nobody
Joe, do we intend to do anything with this bug? 516320 appears closed.
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.
Whiteboard: [memshrink]

Updated

6 years ago
Blocks: 683284
Whiteboard: [memshrink]
You need to log in before you can comment on or make changes to this bug.