Skip to content

Conversation

@kenrussell
Copy link
Member

Restructure the tests to use the Fetch API and Promises to make this
addition easier to read.

Make tolerance configurable, and use a large value for compatibility
with Firefox Nightly on macOS.

The new JPEG resource was exported from the included Photoshop
document which was constructed from scratch for this test case, based
on the original report from Figma.

Regression test for http://crbug.com/1058160 .

Restructure the tests to use the Fetch API and Promises to make this
addition easier to read.

Make tolerance configurable, and use a large value for compatibility
with Firefox Nightly on macOS.

The new JPEG resource was exported from the included Photoshop
document which was constructed from scratch for this test case, based
on the original report from Figma.

Regression test for http://crbug.com/1058160 .
@kenrussell
Copy link
Member Author

All CC'd - please review. Thanks.

This change has been tested locally on macOS with Safari Technology Preview, Firefox, and Chromium with the needed bug fix.

  • Firefox Nightly passes all cases except the RGB10_A2 tests.
  • Safari Technology Preview was failing several of the original test cases with red-green-semi-transparent.png, and also fails about the same number of the new test cases.
  • Chromium with the needed bug fix passes all cases.

Copy link
Member

@shrekshao shrekshao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the change

@MarkCallow
Copy link
Collaborator

I don't think a JPEG file with linear gamma is valid. I hope browser vendors aren't trying to encourage this.

@kenrussell
Copy link
Member Author

I don't think a JPEG file with linear gamma is valid. I hope browser vendors aren't trying to encourage this.

Can you provide more details on why this is invalid? A JPEG with an embedded color profile can have the gamma adjusted to 1.0. Figma receives files like this on an ongoing basis.

Not trying to encourage invalid behavior, but a regression test for this is absolutely needed.

@MarkCallow
Copy link
Collaborator

Which file format are we talking about? I was thinking JFIF in which the color space is required to be BT.601 which has a non-linear transfer function. If you are talking about EXIF, although the default color space is sRGB, complete color space information can be provided including a transfer function which is specified as a table.

I got permission denied when I tried to look at the bug this PR is a regression test for.

@lexaknyazev
Copy link
Member

@MarkCallow
ICC profile may be embedded in a JFIF file using APP2 marker as per ICC.1:2010-12, Annex B.4.

@kenrussell
Copy link
Member Author

The Chromium bug was automatically marked restricted view because it's a crash report. It's not a security bug though. You can provoke the same crash by patching this PR into your checkout of the WebGL conformance suite and running any of the conformance[2]/textures/image_bitmap_from_blob/ tests in Chromium.

I don't know the details of the JPEG file which I exported from Photoshop - please feel free to examine the one in this PR. To create it, I defined a custom color profile, set the gamma to 1.0, and exported the file as a JPEG with an embedded color profile. The original PSD file is included.

@MarkCallow
Copy link
Collaborator

I found how the profile is being embedded. Profiles are indeed not part of JFIF. However the ICC spec. in Annex 4, Section B.4 describes how to use JPEG Interchange Format (note the missing "F") APP2 markers to store an ICC profile. .jpg files with profiles are valid JPEG Interchange Format files and valid in some subset of the family of JPEG Interchange Format files, not including the JFIF format.

@kenrussell
Copy link
Member Author

Thanks for your reviews. Merging now.

@kenrussell kenrussell merged commit b696239 into KhronosGroup:master Jun 30, 2020
@kenrussell kenrussell deleted the linear-gamma-profile branch June 30, 2020 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants