The JPEG decoder should support downscaling during decode. We need to support arbitrary sizes (within reason) and use the minimum amount of memory feasible. We already have some support for downscaling JPEGs during decode using the -moz-sample-size media fragment, but it's not flexible enough for our needs in practice, and it's something that I don't think we want to standardize. -moz-sample-size only supports a fixed set of ratios, and we need arbitrary sizes to work.

I'll take this over to get this pushed over the finish line.

This patch adds a new class to ImageLib, Downscaler, which does streaming high-quality downscaling using Skia's downscaling code. The API should be pretty clear. Essentially, decoders do not write their output directly into their output buffer, but instead into a buffer owned by Downscaler, which then uses Skia to do the downscaling and then writes the resulting scaled image to the decoder's output buffer.

Here's the patch where we flip it on for JPEG images. There's not that much here, actually. - We update ImageFactory::ShouldDownscaleDuringDecode to return true for all JPEG content types. - We add an override for SetTargetSize in nsJPEGDecoder that creates a Downscaler. - We use the target size when allocating a frame in nsJPEGDecoder if a Downscaler is present. - The code that actually writes to the output buffer is updated to use the Downscaler API if a Downscaler is present. That's it!

Comment on attachment 8546991 [details] [diff] [review] (Part 1) - Add a streaming downscaler to ImageLib >+Downscaler::DownscaleInputLine() >+ // Shift the buffer. We're just moving pointers here, so this is cheap. >+ mLinesInBuffer -= diff; >+ mLinesInBuffer = max(mLinesInBuffer, 0); >+ for (int32_t i = 0; i < mLinesInBuffer; ++i) { >+ swap(mWindow[i], mWindow[filterLength - mLinesInBuffer + i]); >+ } Isn't |filterLength - mLinesInBuffer| equal to diff here?

(In reply to Timothy Nikkel (:tn) from comment #6) > Isn't |filterLength - mLinesInBuffer| equal to diff here? That seems right but I was scared to make this simplification before pushing. Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4626f6062b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/66a9a3b1aadb Based on this try job, which looks pretty green right now: https://tbpl.mozilla.org/?tree=Try&rev=f3f4d04d1fbe

sorry had to back this out in: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5c20be43873b because it seems backing out alone Bug 1120050 caused a bustage there and so this changeset had to go out as well. Sorry

https://hg.mozilla.org/projects/cypress/rev/6a80f8abd749 https://hg.mozilla.org/projects/cypress/rev/48028fd5c776

Does this patch give us any way to tell gecko to immediately discard all memory used for a decoded image? If img contains an 8mp offscreen image, and I've just copied it into a canvas element for editing, can I do img.width = 0 or something like that to free the memory associated with it? I think currently I set img.src to '', but I am not sure that that will always discard the image immediately. Just looking for optimization opportunities here.

And another question: is there any intention to uplift this to the newly-cut Gecko branch for FirefoxOS v2.2? If so, that will dramatically change the urgency behind bug 1121648.

(In reply to David Flanagan [:djf] from comment #10) > Does this patch give us any way to tell gecko to immediately discard all > memory used for a decoded image? If img contains an 8mp offscreen image, and > I've just copied it into a canvas element for editing, can I do img.width = > 0 or something like that to free the memory associated with it? I think > currently I set img.src to '', but I am not sure that that will always > discard the image immediately. Just looking for optimization opportunities > here. This patch doesn't address that, but I do think we can look at being more aggressive about dropping decoded versions of images once the only reference to them is the image cache. (i.e., we can ensure that the |img.src = '';| approach works) As I mentioned on IRC, I'll file a bug about this later today.

(In reply to David Flanagan [:djf] from comment #11) > And another question: is there any intention to uplift this to the newly-cut > Gecko branch for FirefoxOS v2.2? If so, that will dramatically change the > urgency behind bug 1121648. The plan is to uplift to B2G 2.2, yeah.

This needed a rebase.

This needed a rebase.

We need a new try job: https://tbpl.mozilla.org/?tree=Try&rev=1f74115e24ec

Try job looks OK. This should be ready to push.

Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ae3c5f147a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dba129633849

https://hg.mozilla.org/mozilla-central/rev/c3ae3c5f147a https://hg.mozilla.org/mozilla-central/rev/dba129633849

FWIW, this broke --disable-skia, since MOZ_RELEASE_ASSERT expects a condition to check, not just a string.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 1/22-2/8) from comment #20) > FWIW, this broke --disable-skia, since MOZ_RELEASE_ASSERT expects a > condition to check, not just a string. Doh, thanks for pointing that out. I'll file a followup to fix it.