452125 - libpr0n png transparency tests use preblended image as a reference

Status: RESOLVED FIXED

(Core :: Graphics: Color Management, defect)

Description

[Bobby Holley (:bholley)]

Right now, the reftests in libpr0n/test/reftest/pngsuite-background compare a png with an alpha channel overlaid on top of a blue background with an html table-ized reference image with the blending already performed. This a problem when we turn on color management, because we're comparing the results of blend-then-cmstransform with the results of cmstransform-then-blend. Since the color management operation is nonlinear (and for other reasons), we can't expect the two operations to commute.

One way to fix this would be to generate two html tables, one for the background and one corresponding to the image, and overlay them using css transparency. This would be a bit of a pain, since it would require writing a program to read the png and generate a table.

Vlad thinks that it's unnecessary to test the exact results of the blending, on the grounds that if transparency was screwed up in general a lot of other things would break. As such, he thinks it's sufficient to alter the test such that it just ensures that a transparent png on a blue background isn't the same as that same png on a white background (to make sure the alpha channel is getting read from the png). I'll put together a patch for this today and put it up for review.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch to fix up the tests

Patch added - flagging vlad for review.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 335638 [details] [diff] [review]
patch to fix up the tests

dolske, I think you did these tests originally (or was it sayer?)... does this make sense?  Essentially before we were comparing <png> on sold background to table of opaque pixels; that breaks when color management is enabled, so this changes it to compare it to table of translucent pixels on top of a solid colored background.

Essentially it's testing that png decoding with translucency works, and that images are correctly drawn with translucency, which I think is the same test as before..

Comment 3

[Justin Dolske [:Dolske]]

The img2html.html tool I made the tests will output TD cells with rgba() values where a < 1], so it should be possible to do that and style the whole table with "background-color: blue". I don't remember why I made the tests pre-blend with a background... Maybe I was thinking that if alpha blending was totally busted, the test and reference could both end up flat-black or something. *shrug*

Bobby's going to whip up a version of the HTML references like that, and I'll test on Windows/Linux to see if that works.

Comment 4

[Justin Dolske [:Dolske]]

Blah. This ends up with a bunch of off-by-1 differences in the reftest runs, even with color management disabled. For example, with the test of bgai4a08.png, the pixel at 30,9 (starting from 0,0) ends up as rgb(210,213,218) for the PNG-on-blue, and as rgb(209,212,217) for the table-on-blue.

[The table cell is rgba(217,217,217,0.9647), and the blue is rgb(10,100, 250). The png pixel is rgba(180,180,180,0.9647), which confused us, but it's just the gAMA chunk being annoying again ala bug 405398. *sigh*]

I'm assuming the difference is again rounding errors from the pre/post-multiplied conversions in <canvas>. Really, really, really want a reftest that can be fuzzed to allow off-by-1 differences like this!

I guess the only choice for now is to go with something like the original patch in this bug. :/

Comment 5

[Bobby Holley (:bholley)]

Attachment: patch to force sRGB as an output profile for retests

It turns out that quite a few reference renderings for layout stuff like xul use the pre-blended approach. This means that we'd be loosing more test coverage than we thought with the first proposed patch.

One option would be to have the Talos setup turn off color management for the testing builds. However, this would make reftests fail for ordinary developer runs of the reftests (./firefox -reftest ...), which roc wasn't too happy about.

So after talking with vlad, roc, dbaron, dolske, and some others, I finally settled on the approach of forcing sRGB as an output profile for the reftests. This means that we get a deterministic output space for reference renderings in a way that doesn't inconvenience people and also in a way that doesn't completely bypass the cms machinery (dbaron's concern).

Flagging vlad for review.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 336738 [details] [diff] [review]
patch to force sRGB as an output profile for retests

Looks fine, but

>+static const char *CMForceSRGBPrefName = "gfx.color_management.x-moz-use-srgb";

Call this 'gfx.color-management.force-srgb' instead of x-moz-*, it's already an internal hidden pref.

Comment 7

[Bobby Holley (:bholley)]

roger that vlad.

pushed in 4b2977a03aba.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> So after talking with vlad, roc, dbaron, dolske, and some others, I finally
> settled on the approach of forcing sRGB as an output profile for the reftests.
> This means that we get a deterministic output space for reference renderings in
> a way that doesn't inconvenience people and also in a way that doesn't
> completely bypass the cms machinery (dbaron's concern).

No, my concern was actually that the behavior that we get with CMS enabled is incorrect per specs, and can cause color mismatches relative to what authors expect whenever partial transparency is used.

I'd also note that the patch to reftest doesn't leave the profile unchanged; it resets the preference to default.  This might be good enough, but it's not what the comment says.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #4)
> I'm assuming the difference is again rounding errors from the
> pre/post-multiplied conversions in <canvas>. Really, really, really want a
> reftest that can be fuzzed to allow off-by-1 differences like this!

Hm, I'm not sure what this issue is -- the pre/post-multiplication conversion issues only come up if you call getImageData() and then putImageData().  reftests just call drawWindow, which draws to the canvas buffer, and then they call getData which uses the existing data.  bholley says that this was showing up while you were trying to generate the reference images though, in which case getImageData/etc. was being called?