image/test/reftest/bmp/bmpsuite/b/badrle.bmp and shortfile.bmp reftests fail under Skia content

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lsalzman, Assigned: njn)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
When the gfx.content.azure.backends is set to skia, the badrle.bmp and shortfile.bmp reftests in image/test/reftest/bmp/bmpsuite/b/ fail.

This is presumably because the remaining uninitialized pixels in the image do not have their alpha channel properly filled with the value 255. Since Skia does not actually have a RGBX surface format - we essentially fake it with a RGBA format where the alpha channel is always filled opaque - the alpha channel unfortunately matters and must be filled appropriately.

It might be possible to notice this somehow when using Skia canvas which we do currently deploy on a few platforms, but I have not tested that, nor do I think we have any reftests other than bmpsuite that would have shown it.
(Assignee)

Comment 1

3 years ago
I'll take a look at this once bug 1215361 and its chain of blockers (currently waiting on reviews) is done.

Interestingly, we currently (except for the Skia back-end) show the unfilled-in parts of these images as black, while Chromium shows them as transparent. We need to be consistent, but I could be persuaded either way as to which is preferable.

I just tried to reproduce this myself but I'm still seeing black for the unfilled-in parts. This is on Linux -- is that expected?
So there are two cases where we rely on the surface format to ignore 0-valued alpha pixels:

(1) If the BMP is truncated.
(2) If it's a BMP-in-ICO and all the alpha bytes are 0.

In either case, we need to walk over the affected pixels and set their alpha value to 0xFF. The only difference is that in case (1), we only need to walk over the pixels that we haven't written yet, while in case (2) we need to walk over the entire image.
(This assumes that we want to retain the current approach of making the unwritten pixels in truncated images opaque. I don't feel strongly about this either way.)
(Reporter)

Comment 4

3 years ago
(In reply to Nicholas Nethercote [:njn] from comment #1)
> I'll take a look at this once bug 1215361 and its chain of blockers
> (currently waiting on reviews) is done.
> 
> Interestingly, we currently (except for the Skia back-end) show the
> unfilled-in parts of these images as black, while Chromium shows them as
> transparent. We need to be consistent, but I could be persuaded either way
> as to which is preferable.
> 
> I just tried to reproduce this myself but I'm still seeing black for the
> unfilled-in parts. This is on Linux -- is that expected?

If you directly view the images, then the problem won't show. If you run them using the reftest harness, and then look at them in the reftest analyzer, you will then see the "BMP" pixel junk instead. So presumably there is some interaction with the test harness/canvas going on here that highlights the problem.
(Assignee)

Comment 5

3 years ago
I can reproduce the failures locally, and I've started looking at how to fix it.
(Assignee)

Comment 6

3 years ago
Created attachment 8681034 [details] [diff] [review]
Fill in missing pixels caused by truncated BMP files

I've fixed case (1) from comment 2, which fixes the two test failures with the
Skia backend.

I haven't fixed case (2) from comment 2. While attempting to fix it I hit bug
1220021, so I'd prefer to handle it as a follow-up once bug 1220021 is fixed.
Attachment #8681034 - Flags: review?(seth)
(Assignee)

Comment 7

3 years ago
Seth: 11 day review ping.
Comment on attachment 8681034 [details] [diff] [review]
Fill in missing pixels caused by truncated BMP files

Review of attachment 8681034 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8681034 - Flags: review?(seth) → review+
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/951e77fb1760a8f2047fe7f83217f69f480181a2
Bug 1217465 - Fill in missing pixels caused by truncated BMP files. r=seth.

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/951e77fb1760
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.