1062066 - Add downscale-during-decode support for the BMP decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Artem Sobolev [:asobolev]]

Introduce support for bitmaps to be downscaled during decoding

Comment 1

[Artem Sobolev [:asobolev]]

Attachment: Preliminary patch

Comment 2

[Seth Fowler [:seth] [:s2h]]

Wow, since this didn't block the ddd bug I didn't notice it until I already rewrote the patch. It's kinda interesting to read this patch and see where our implementations converged on the same solution and where they diverged.

At any rate, the original patch is written against a much different version of the Downscaler API and isn't complete, so I'll go ahead and mark it obsolete.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add support for vertically flipping downscaler output

This preliminary patches adds support to Downscaler for flipping its output.
This is necessary for BMPs (and therefore will eventually be necessary for ICOs
too) because BMPs store their rows in reverse order.

Unlike Artem's original patch, I flip the output, not the input. That means that
we're downscaling the image upside down and *then* flipping it, but this won't
matter, because the scaling kernel is symmetrical.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a Downscaler API to clear a row

This is another new API for Downscaler. We sometimes need to clear a row while
processing BMPs because RLE can make us skip pixels. Rather than duplicate that
code everywhere we need it, it makes sense to introduce an API on Downscaler.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Only mark BMP surfaces as transparent if they actually have alpha data

So there are a couple of difficult cases when it comes to downscaling BMPs
during decode. This part solves the most serious issue.

The problem is that right now, the BMP decoder ignores zero alpha bytes on 32bpp
BMPs. If it ever encounters a non-zero alpha byte, it goes back over the image
data and rewrites all the alpha bytes. That's not great for DDD, though, since
we're not storing the input data once we process it. It's possible to rewrite
the already-scaled output data, but the problem is that the wrong values will
remain in the Downscaler's "window" of rows and will be used when subsequent
rows are downscaled.

This patch reworks the design to use a different approach: we *always* use the
alpha value specified in the input data, but we change the surface format to
have no alpha if the image doesn't contain any (via the Opacity value passed to
PostFrameStop).

The only sticking point is that nsICODecoder may apply alpha via post-processing
on the output data, because ICO files can contain an alpha mask after the image
data. That code runs before the nsICODecoder's contained nsBMPDecoder's
FinishInternal() method gets called, so all we need to do is let the
nsBMPDecoder know that it actually *does* have alpha data (because of the ICO
alpha mask), and everything will work fine. For that reason, I've added a new
method, nsBMPDecoder::SetHasAlphaData(), and updated nsICODecoder to call it.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Add downscale-during-decode support for the BMP decoder

This patch actually adds downscale-during-decode support to the BMP decoder. In
general, this is straightforward; the patch is similar to the ones for the JPEG
and PNG decoders.

There were two tricky cases in the BMP decoder. The first was the fact that zero
alpha values needed to be ignored until a non-zero alpha value was seen. That
problem was eliminated by part 3.

The other tricky case is the "delta" mode available for RLE-encoded BMPs. This
mode allows the encoded BMP to move the current position to the right (skipping
columns) and/or down (skipping rows). This isn't really problematic for
downscale-during-decode, but because we reuse the same row buffer repeatedly
when downscaling, we need to ensure that we clear pixels that we're skipping
over (so pixels from previous rows don't end up leaking into later rows) and we
need to ensure that we commit blank rows when we skip rows. This patch includes
code that handles both of those issues.

Once this patch lands, we'll have DDD support for both the BMP and PNG decoders,
which means we can enable DDD for the ICO decoder as well.

Comment 7

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f21398f461f

Comment 8

[Seth Fowler [:seth] [:s2h]]

Try looks good on this one.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e79a95104e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f8a2245030
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a56f13c2f75
https://hg.mozilla.org/integration/mozilla-inbound/rev/8904253408ca

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e79a95104e8
https://hg.mozilla.org/mozilla-central/rev/f5f8a2245030
https://hg.mozilla.org/mozilla-central/rev/4a56f13c2f75
https://hg.mozilla.org/mozilla-central/rev/8904253408ca