Closed
Bug 1062066
Opened 10 years ago
Closed 9 years ago
Add downscale-during-decode support for the BMP decoder
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: asobolev, Assigned: seth)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
6.77 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
2.67 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
7.13 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
13.67 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
Introduce support for bitmaps to be downscaled during decoding
Reporter | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8484677 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: asobolev → seth
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8656361 -
Flags: review?(tnikkel) → review+
Updated•9 years ago
|
Attachment #8656359 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 5•9 years ago
|
||
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.
Attachment #8656743 -
Flags: review?(tnikkel)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f21398f461f
Assignee | ||
Comment 8•9 years ago
|
||
Try looks good on this one.
Updated•9 years ago
|
Attachment #8656743 -
Flags: review?(tnikkel) → review+
Updated•9 years ago
|
Attachment #8656751 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 9•9 years ago
|
||
Thanks for the review!
Assignee | ||
Comment 10•9 years ago
|
||
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•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
![]() |
||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•