This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for retina thumbnails being massive #2439
Fix for retina thumbnails being massive #2439
Changes from 7 commits
577c411
8511bc2
f1b00df
e002a59
45f3282
20aedce
d400ebd
15ba24f
ea71970
774314b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to define the macOS header here? The spec defines pretty clear behaviour for how to handle the chunk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there isn’t anything macOS specific here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not macOS specific, but the resolution has to be an exact match due to line 134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@turt2live what were you thinking of here? we're already parsing the png correctly into chunks, and then checking whether the
pHYs
chunk contains hidpi metrics of any flavour.The only slight cheekiness is that it only applies this fix if the DPI is 5660 pixels-per-metre (which macOS uses for its screenshots) rather than trying to support arbitrary resolutions, but the reason for this is that absolute DPI values for screen use (as opposed to print use) are illdefined at the best of times. After all, who's to say that 1024x768 should be viewed on a 15" or 17" monitor, etc?
We could refine this perhaps by assuming that anything higher than 96dpi (3780ppm) should be randomly halved in size when being viewed in Riot/Web, but i can easily see there may be some source of higher-DPI images out there where the human intention is to be viewed at 1-image-pixel = 1-screen-pixel rather than being halved, as is the intent for macOS screenshots. So let's start like this, and then broaden it when we find other images getting mishandled, rather than overshoot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not check that the pHYs header has hipdi metrics of any flavour, only that they have the flavour we want (5669 pixels per metre). I don't think it's impossible for us to handle other sizes. and I do think we should even if other sources of hidpi images are uncommon. We definitely do not need to handle a unit of zero (unknown) though - that's just a nightmare.