Closed Bug 1062066 Opened 6 years ago Closed 5 years ago

Add downscale-during-decode support for the BMP decoder


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox43 --- fixed


(Reporter: asobolev, Assigned: seth)


(Blocks 1 open bug)



(4 files, 1 obsolete file)

Introduce support for bitmaps to be downscaled during decoding
Attached patch Preliminary patch (obsolete) — Splinter Review
Blocks: ddd
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.
Attachment #8484677 - Attachment is obsolete: true
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.
Attachment #8656359 - Flags: review?(tnikkel)
Assignee: asobolev → seth
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.
Attachment #8656361 - Flags: review?(tnikkel)
Attachment #8656361 - Flags: review?(tnikkel) → review+
Attachment #8656359 - Flags: review?(tnikkel) → review+
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

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.
Attachment #8656743 - Flags: review?(tnikkel)
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.
Attachment #8656751 - Flags: review?(tnikkel)
Try looks good on this one.
Attachment #8656743 - Flags: review?(tnikkel) → review+
Attachment #8656751 - Flags: review?(tnikkel) → review+
Thanks for the review!
No longer blocks: 1220021
Depends on: 1220021
Depends on: 1247034
Depends on: 1249550
You need to log in before you can comment on or make changes to this bug.