Open Bug 511899 Opened 10 years ago Updated Last year

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

Tracking Status
blocking2.0 --- -

People

(Reporter: bholley, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

spacer.gif is a good example of this. Should be easy to fix after bug 435296 lands.
Summary: disable discarding for images when sizeof(sourceData) > sizeof(decodedFrames) → disable discarding on a per-image basis when it isn't an obvious win
Attached patch patch v1Splinter Review
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
It is time to flag?
soon, but not yet.
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: --- → ?
This should be easy to fix for 2.0, since we're already fixing bug 513681. So I agree that it should block.
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.
Let's move this discussion over to bug 516320.
Blocks: 516320
No longer depends on: 435296, 513681
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]
Blocks: image-suck
Whiteboard: [memshrink]
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.