Closed Bug 1045929 Opened 5 years ago Closed 5 years ago

Add downscale-during-decode support for the JPEG decoder

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached file Patch Preview (obsolete) —
Attached patch Preliminary patch (obsolete) — Splinter Review
Attachment #8481953 - Attachment is obsolete: true
I'll take this over to get this pushed over the finish line.
Assignee: asobolev → seth
Attachment #8484672 - Attachment is obsolete: true
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.
Attachment #8546991 - Flags: review?(tnikkel)
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!
Attachment #8546993 - Flags: review?(tnikkel)
Blocks: 1120056
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?
Attachment #8546991 - Flags: review?(tnikkel) → review+
Attachment #8546993 - Flags: review?(tnikkel) → review+
(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
Blocks: 1121648
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.
Flags: needinfo?(seth)
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.
Flags: needinfo?(seth)
(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.
Attachment #8546991 - Attachment is obsolete: true
Attachment #8546993 - Attachment is obsolete: true
Try job looks OK. This should be ready to push.
https://hg.mozilla.org/mozilla-central/rev/c3ae3c5f147a
https://hg.mozilla.org/mozilla-central/rev/dba129633849
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Depends on: 1125457
No longer blocks: 1120056
You need to log in before you can comment on or make changes to this bug.