Fix for retina thumbnails being massive #2439
Conversation
needs server to support 1600x1200 thumbnails for retina large ones. ideally need to cap maximum thumbnail size to 800x600 rather than expand to arbitrary widths. need to check that luke's funky timeline code doesn't get confused between naturalWidth and infoWidth etc. also need to consider whether to encode a resolution metric in the event rather than lying about resolution.
…used as a thumbnail
|
Ready for review. Checked that
Seems to work well. |
bwindels
changed the title
|
Also looking to write an MSC for a better solution independent of this. |
There was a problem hiding this comment.
Should we also downsize the dimensions when posting hidpi images? Similar to how stickers are handled: the physical image dimensions are 512x512, however we render them at 256x256 by recommendation of the spec (and because the sticker event intentionally halves the dimensions)
|
|
||
| // Polyfill for Canvas.toBlob API using Canvas.toDataURL | ||
| import "blueimp-canvas-to-blob"; | ||
|
|
||
| const MAX_WIDTH = 800; | ||
| const MAX_HEIGHT = 600; | ||
|
|
||
| // scraped out of a macOS hidpi (5660ppm) screenshot png | ||
| // 5669 px (x-axis) , 5669 px (y-axis) , per metre | ||
| const PHYS_HIDPI = [0x00, 0x00, 0x16, 0x25, 0x00, 0x00, 0x16, 0x25, 0x01]; |
There was a problem hiding this comment.
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.
there isn’t anything macOS specific here?
There was a problem hiding this comment.
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.
@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.
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.
isn’t that what this is doing? or do you mean something else by hidpi? |
|
oh it is, I got confused by |
|
Doesn't seem to work in FF, canceling review request for now. |
|
Works now in FF as well |
Yep, plan is to parse resolution and add it as metadata to |
|
see #2439 (comment) for why i think it's correct to hardcode the resolution for now. |
todo:
ideally need to cap maximum thumbnail size to 800x600 rather than expand to arbitrary widths.actually, having fullwidth images are probably fine, as long as we cap the height at 600px (which we do already)need to check that luke's funky timeline code doesn't get confused between naturalWidth and infoWidth etc because we're taking an image which claims to be 1600x1200 and then shrinking it to 800x600 because we realise it's retina. But if the code asks for naturalHeight (which it does) it'll get 1200 rather than 600...
also need to consider whether to encode a resolution metric in the event rather than lying about resolution. <-- in the long run we certainly should.
element-hq/element-web#2985