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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Lee Salzman [:lsalzman]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

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?

Comment 2

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

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.

Comment 3

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

(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.)

Comment 4

[Lee Salzman [:lsalzman]]

(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.

Comment 5

[Nicholas Nethercote [inactive]]

I can reproduce the failures locally, and I've started looking at how to fix it.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: 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.

Comment 7

[Nicholas Nethercote [inactive]]

Seth: 11 day review ping.

Comment 8

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

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!

Comment 9

[Nicholas Nethercote [inactive]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/951e77fb1760