Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for density-corrected size #23789

Closed
wants to merge 5 commits into from
Closed

Conversation

noamr
Copy link
Contributor

@noamr noamr commented May 27, 2020

These test include images that have EXIF values that specify density-corrected size
And they assert that the intrinsic size for those images is corrected by that EXIF.

WhatWG PR: whatwg/html#5574

@wpt-pr-bot
Copy link
Collaborator

There are no reviewers for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23789 May 27, 2020 11:57 Inactive
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23789 May 27, 2020 12:32 Inactive
@annevk
Copy link
Member

annevk commented May 28, 2020

How does Content-DPR relate to this? I thought this was only about EXIF?

What would be interesting to see:

  • Coverage for a wide variety of popular image formats
  • Out-of-range values for the EXIF fields.
  • Either generating the image EXIF data dynamically or documenting clearly what the EXIF metadata is supposed to be and how it can be inspected.

@noamr
Copy link
Contributor Author

noamr commented May 28, 2020

How does Content-DPR relate to this? I thought this was only about EXIF?`

Oops, probably a copy-paste thing (this is based on old content-dpr tests, as it has a similar purpose)

What would be interesting to see:

  • Coverage for a wide variety of popular image formats
  • Out-of-range values for the EXIF fields.

Sure, I can add that.

  • Either generating the image EXIF data dynamically or documenting clearly what the EXIF metadata is supposed to be and how it can be inspected.

yea I can try to integrate exiftool into WPT-runner... let's see how that works

noamr added 2 commits May 31, 2020 10:29
These test include images that have EXIF values that specify density-corrected size
And they assert that the intrinsic size for those images is corrected by that EXIF.

WhatWG PR: whatwg/html#5574
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23789 June 1, 2020 09:37 Inactive
@wpt-pr-bot wpt-pr-bot requested a review from jgraham June 1, 2020 10:04
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23789 June 1, 2020 10:05 Inactive
@noamr
Copy link
Contributor Author

noamr commented Jun 1, 2020

New patch uses piexif.js to generate density-corrected size images on the fly.
Leaving PNG and other formats for later (after the spec gets some thumbs up) as it's more difficult to implement - some EXIF-related tools only support JPEG.
Also, the spec now does not define ranges, and it is impossible to generate negative numbers in EXIF.

Copy link
Contributor

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

Tests look good! I want someone more tied to the project to verify it's OK to land third party code.

await test_valid({width: 10, height: 20, preferredWidth: 10, preferredHeight: 40, resolutionX: 72, resolutionY: 36, resolutionUnit: 2})
await test_valid({width: 30, height: 30, preferredWidth: 90, preferredHeight: 30, resolutionX: 24, resolutionY: 72, resolutionUnit: 2})

await test_valid({width: 10, height: 20, preferredWidth: 20, preferredHeight: 40, resolutionX: 36, resolutionY: 36, resolutionUnit: 2})
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between those 4 tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lores, hires, only one of the dimension, change in aspect ratio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, forgot to erase some identical tests

@yoavweiss
Copy link
Contributor

@foolip - can you take a look at the third-party code here and make sure this is the right way to land it?

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-23789 June 4, 2020 09:28 Inactive
@foolip
Copy link
Member

foolip commented Jun 5, 2020

@yoavweiss how to include third party JS is the topic of web-platform-tests/rfcs#46, which isn't resolved yet, but I'd recommend following the suggestion in web-platform-tests/rfcs#46 (comment). That is, put it in a third_party directory, and make sure to include a LICENSE file.

@noamr
Copy link
Contributor Author

noamr commented Oct 16, 2020

Already merged as part of Chromium CL.

@noamr noamr closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants