Once bug 1045929 lands, we'll have downscale-during-decode for JPEG images. That solves in a more general way the same problem that the -moz-sample-size media fragment solved: we want to display a large JPEG image at a small onscreen size, without wasting huge amounts of memory. So we can now get rid of -moz-sample-size, and we'll do that in this bug. I think it's worth considering, though, whether it might be worth keeping some vestige of -moz-sample-size's behavior around. Using libjpeg's scaling API we can get scales to M/8 sizes more efficiently than the downscale-during-decode code, which is based on Skia's scaling code. It's appealing, but there are two things that make this questionable to me: - Do we really display images at M/8 sizes that often? I'd guess when we're scaling down we're not usually doing it at nice sizes like that, though maybe on B2G our frontend developers might tune performance heavily enough to take this sort of thing into account. - Undoubtedly libjpeg's output is going to be different than the output we get from Skia's scaling code. Is it worth the performance gain to introduce "discontinuities" like this where we use different scaling algorithms at different sizes? I'd guess that this usually wouldn't be noticeable, but web developers are great at exposing these kinds of issues.

Jeff, what's your opinion on the matter? It was easy to write the patch to use libjpeg's scaling support at appropriate sizes, so here it is. =) r+ if you think this is a code path worth having.

Here's the patch to remove -moz-sample-size.

Rebased this patch so it's on top of the other patch, which makes more sense.

I'd suggest getting David Flanagan to port over the existing b2g stuff to the new stuff and see how well it works before making a decision here.

Sure. There's not really any porting necessary, as downscale-during-decode is automatically applied to all JPEG images on platforms with APZ enabled. (I'm sure there is some code in the frontend that can be removed now, though!) But it'd be good to test and make sure everything works well.

I'll need more information on how this works before I can say much here. My typical use case for -moz-samplesize is for decoding images into offscreen <img> elements that I then copy to a canvas and resize to create thumbnails and such. How does this new downscale-during-decode work for offscreen images? Do I just create the <img> element and then set the desired width and height on it before setting the src property? And how does the memory performance compare with these two solutions? Does the new solution have transient memory spikes (like decoding into a large buffer and then downsampling into a small one) that we don't have with -moz-samplesize? If so, those could cause problems for the gallery app. Right now, if you remove -moz-samplesize before we can change Gaia, it will cause all kinds of OOM issues for us, since we don't typically set a desired size for an image before decoding.

So David and I discussed this on IRC, but I want to briefly respond to some of these questions here as well for the benefit of others who may come across this bug. (In reply to David Flanagan [:djf] from comment #6) > How does this new downscale-during-decode work for offscreen > images? Do I just create the <img> element and then set the desired width > and height on it before setting the src property? You don't even need to set the width and height. For offscreen images, currently we will not even decode them until you draw them somewhere. (For example, via canvas.drawImage.) I am considering heuristics that will make us eagerly decode them in some situations, but I will consult with you and other interested parties to make sure it won't break any of your use cases. > And how does the memory performance compare with these two solutions? Does > the new solution have transient memory spikes (like decoding into a large > buffer and then downsampling into a small one) that we don't have with > -moz-samplesize? If so, those could cause problems for the gallery app. No, the downscaling is done in a streaming fashion, so only a few scanlines of the image are kept in memory at a time. There won't be any transient memory spikes. > Right now, if you remove -moz-samplesize before we can change Gaia, it will > cause all kinds of OOM issues for us, since we don't typically set a desired > size for an image before decoding. It shouldn't have any negative effect at all, but I do think it's important that we test that before removing it!

(In reply to Seth Fowler [:seth] from comment #7) > > It shouldn't have any negative effect at all, but I do think it's important > that we test that before removing it! I've filed bug 1121648 for Gaia changes to remove code that uses #moz-samplesize and to take advantage of the new Gecko feature. Because you think that things will continue to work when this patch lands, I have not marked the gaia bug as blocking this one. When 1045929 and this bug land, the Gallery app is the one that will have to be tested the most carefully, including trying to display really large 20mp+ jpeg images and testing the pinch-to-zoom feature when viewing a large jpeg. The SMS app and the Wallpaper app also currently use #-moz-samplesize and should be tested as well.

Is this still waiting for review? Are the patches good still?

(In reply to Ehsan Akhgari (don't ask for review please) from comment #9) > Is this still waiting for review? Are the patches good still? They don't seem to apply at all anymore.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #9) > Are the patches good still? They're not. The code has changed a lot in the time since these patches were posted. At the moment the blocker here is downscale-during-decode support for |canvas.drawImage()|; until that lands, the Gallery app will still need #-moz-samplesize. The metabug for that is bug 1211178. I've made it block this bug.

(In reply to Seth Fowler [:seth] [:s2h] from comment #11) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #9) > > Are the patches good still? > > They're not. The code has changed a lot in the time since these patches were > posted. > > At the moment the blocker here is downscale-during-decode support for > |canvas.drawImage()|; until that lands, the Gallery app will still need > #-moz-samplesize. The metabug for that is bug 1211178. I've made it block > this bug. Since then b2g is essentially dead now. Can we just kill moz-samplesize?

(In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #12) > Since then b2g is essentially dead now. Can we just kill moz-samplesize? Timothy or Nick, can one of you answer this since Seth has moved on?

If B2G is *literally* dead now, we can kill it. Killing it will break the gallery app on low-memory phones, so essentially is not necessarily good enough. Say the word and I'll write a patch to kill it.

(In reply to Seth Fowler [:seth] [:s2h] from comment #14) > If B2G is *literally* dead now, we can kill it. Killing it will break the > gallery app on low-memory phones, so essentially is not necessarily good > enough. > > Say the word and I'll write a patch to kill it. "the word". https://groups.google.com/forum/#!msg/mozilla.dev.fxos/FoAwifahNPY/Lppm0VHVBAAJ Pretty please? :-)

Killing this in bug 1311246.