From 577c411a397e2b14c061e05958cb7370255f5ed9 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Mon, 14 Jan 2019 17:10:20 +0000 Subject: [PATCH 1/8] experimental fix for https://github.com/vector-im/riot-web/issues/2985 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. --- package.json | 1 + src/ContentMessages.js | 34 +++++++++++++++++++-- src/components/views/messages/MImageBody.js | 10 ++++-- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 7b55a099489..82246534a24 100644 --- a/package.json +++ b/package.json @@ -81,6 +81,7 @@ "matrix-js-sdk": "0.14.2", "optimist": "^0.6.1", "pako": "^1.0.5", + "png-chunks-extract": "^1.0.0", "prop-types": "^15.5.8", "qrcode-react": "^0.1.16", "querystring": "^0.2.0", diff --git a/src/ContentMessages.js b/src/ContentMessages.js index f2bbdfafe51..e4c62b5de15 100644 --- a/src/ContentMessages.js +++ b/src/ContentMessages.js @@ -25,6 +25,7 @@ import { _t } from './languageHandler'; const Modal = require('./Modal'); const encrypt = require("browser-encrypt-attachment"); +const png_chunks_extract = require("png-chunks-extract"); // Polyfill for Canvas.toBlob API using Canvas.toDataURL require("blueimp-canvas-to-blob"); @@ -32,6 +33,9 @@ require("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]; /** * Create a thumbnail for a image DOM element. @@ -102,10 +106,34 @@ function loadImageElement(imageFile) { const objectUrl = URL.createObjectURL(imageFile); img.src = objectUrl; + // check for hi-dpi PNGs and fudge display resolution as needed. + // this is mainly needed for macOS screencaps + let hidpi = false; + if (imageFile.type === "image/png") { + // in practice macOS happens to order the chunks so they fall in + // the first 0x1000 bytes (thanks to a massive ICC header). + // Thus we could slice the file down to only sniff the first 0x1000 + // bytes (but this makes png_chunks_extract choke on the corrupt file) + const headers = imageFile; //.slice(0, 0x1000); + readFileAsArrayBuffer(headers).then(arrayBuffer=>{ + const buffer = new Uint8Array(arrayBuffer); + const chunks = png_chunks_extract(buffer); + for (const chunk of chunks) { + if (chunk.name === 'pHYs') { + if (chunk.data.byteLength !== PHYS_HIDPI.length) return; + hidpi = chunk.data.every((val, i) => val === PHYS_HIDPI[i]); + return; + } + } + }); + } + // Once ready, create a thumbnail img.onload = function() { URL.revokeObjectURL(objectUrl); - deferred.resolve(img); + let width = hidpi ? (img.width >> 1) : img.width; + let height = hidpi ? (img.height >> 1) : img.height; + deferred.resolve({ img, width, height }); }; img.onerror = function(e) { deferred.reject(e); @@ -129,8 +157,8 @@ function infoForImageFile(matrixClient, roomId, imageFile) { } let imageInfo; - return loadImageElement(imageFile).then(function(img) { - return createThumbnail(img, img.width, img.height, thumbnailType); + return loadImageElement(imageFile).then(function(r) { + return createThumbnail(r.img, r.width, r.height, thumbnailType); }).then(function(result) { imageInfo = result.info; return uploadFile(matrixClient, roomId, result.thumbnail); diff --git a/src/components/views/messages/MImageBody.js b/src/components/views/messages/MImageBody.js index dc891b86ffd..c0e751431f3 100644 --- a/src/components/views/messages/MImageBody.js +++ b/src/components/views/messages/MImageBody.js @@ -179,10 +179,16 @@ export default class MImageBody extends React.Component { // given we deliberately don't thumbnail them serverside to prevent // billion lol attacks and similar return this.context.matrixClient.mxcUrlToHttp( - content.info.thumbnail_url, 800, 600, + content.info.thumbnail_url, + 800 * window.devicePixelRatio, + 600 * window.devicePixelRatio, ); } else { - return this.context.matrixClient.mxcUrlToHttp(content.url, 800, 600); + return this.context.matrixClient.mxcUrlToHttp( + content.url, + 800 * window.devicePixelRatio, + 600 * window.devicePixelRatio + ); } } From 8511bc27cc4e11479cc0203437536d6e2f430407 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 20 Jan 2019 15:35:16 +0000 Subject: [PATCH 2/8] only request thumbs on retina if the original image is too big to be used as a thumbnail --- src/components/views/messages/MImageBody.js | 67 +++++++++++++++++++-- 1 file changed, 61 insertions(+), 6 deletions(-) diff --git a/src/components/views/messages/MImageBody.js b/src/components/views/messages/MImageBody.js index c0e751431f3..14c8c94f300 100644 --- a/src/components/views/messages/MImageBody.js +++ b/src/components/views/messages/MImageBody.js @@ -151,6 +151,18 @@ export default class MImageBody extends React.Component { if (this.refs.image) { const { naturalWidth, naturalHeight } = this.refs.image; + // XXX: if this is a retina image, the naturalWidth/Height + // are going to be off by a factor of 2. However, we don't know + // this at this point - so until we include a resolution param + // in info, this might cause scroll jumps for retina images? + // + // Something like: + // + // if (content.info && content.info.devicePixelRatio) { + // naturalWidth /= content.info.devicePixelRatio; + // naturalHeight /= content.info.devicePixelRatio; + // } + loadedImageDimensions = { naturalWidth, naturalHeight }; } @@ -167,6 +179,11 @@ export default class MImageBody extends React.Component { } _getThumbUrl() { + // FIXME: the dharma skin lets images grow as wide as you like, rather than capped to 800x600. + // So either we need to support custom timeline widths here, or reimpose the cap, otherwise the + // thumbnail resolution will be unnecessarily reduced. + // custom timeline widths seems preferable. + const content = this.props.mxEvent.getContent(); if (content.file !== undefined) { // Don't use the thumbnail for clients wishing to autoplay gifs. @@ -175,7 +192,7 @@ export default class MImageBody extends React.Component { } return this.state.decryptedUrl; } else if (content.info && content.info.mimetype === "image/svg+xml" && content.info.thumbnail_url) { - // special case to return client-generated thumbnails for SVGs, if any, + // special case to return clientside sender-generated thumbnails for SVGs, if any, // given we deliberately don't thumbnail them serverside to prevent // billion lol attacks and similar return this.context.matrixClient.mxcUrlToHttp( @@ -184,11 +201,49 @@ export default class MImageBody extends React.Component { 600 * window.devicePixelRatio, ); } else { - return this.context.matrixClient.mxcUrlToHttp( - content.url, - 800 * window.devicePixelRatio, - 600 * window.devicePixelRatio - ); + if (window.devicePixelRatio === 1.0 || + (!content.info || !content.info.w || + !content.info.h || !content.info.size)) + { + // always thumbnail. it may look a bit worse, but it'll save bandwidth. + // which is probably desirable on a lo-dpi device anyway. + return this.context.matrixClient.mxcUrlToHttp( + content.url, + 800 * window.devicePixelRatio, + 600 * window.devicePixelRatio + ); + } + else { + // we should only request thumbnails if the image is bigger than 800x600 + // (or 1600x1200 on retina) otherwise the image in the timeline will just + // end up resampled and de-retina'd for no good reason. + // Ideally the server would pregen 1600x1200 thumbnails in order to provide retina + // thumbnails, but we don't do this currently in synapse for fear of disk space. + // As a compromise, let's switch to non-retina thumbnails only if the original + // image is both physically too large and going to be massive to load in the + // timeline (e.g. >1MB). + + if ((content.info.w > 800 * window.devicePixelRatio || + content.info.h > 600 * window.devicePixelRatio) && + content.info.size > 1*1024*1024) + { + // image is too large physically and bytewise to clutter our timeline so + // we ask for a thumbnail, despite knowing that it will be max 800x600 + // despite us being retina (as synapse doesn't do 1600x1200 thumbs yet). + + return this.context.matrixClient.mxcUrlToHttp( + content.url, + 800 * window.devicePixelRatio, + 600 * window.devicePixelRatio + ); + } + else { + // no width/height means we want the original image. + return this.context.matrixClient.mxcUrlToHttp( + content.url, + }; + } + } } } From e002a596505a866b6fa1562c106b971e66e6829a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 5 Apr 2019 15:07:24 +0200 Subject: [PATCH 3/8] mistake during merge --- src/ContentMessages.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ContentMessages.js b/src/ContentMessages.js index 69989734490..dfcfbda3751 100644 --- a/src/ContentMessages.js +++ b/src/ContentMessages.js @@ -26,7 +26,7 @@ import { _t } from './languageHandler'; import Modal from './Modal'; import RoomViewStore from './stores/RoomViewStore'; import encrypt from "browser-encrypt-attachment"; -import png_chunks_extract = from "png-chunks-extract"; +import png_chunks_extract from "png-chunks-extract"; // Polyfill for Canvas.toBlob API using Canvas.toDataURL import "blueimp-canvas-to-blob"; From 45f3282b1b30397fe05df836a3515cc2cad18734 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 8 Apr 2019 14:57:39 +0200 Subject: [PATCH 4/8] cleanup and linting --- src/components/views/messages/MImageBody.js | 66 ++++++++++----------- 1 file changed, 30 insertions(+), 36 deletions(-) diff --git a/src/components/views/messages/MImageBody.js b/src/components/views/messages/MImageBody.js index 3d09fa784a9..9fd42fb31d7 100644 --- a/src/components/views/messages/MImageBody.js +++ b/src/components/views/messages/MImageBody.js @@ -150,19 +150,7 @@ export default class MImageBody extends React.Component { if (this.refs.image) { const { naturalWidth, naturalHeight } = this.refs.image; - - // XXX: if this is a retina image, the naturalWidth/Height - // are going to be off by a factor of 2. However, we don't know - // this at this point - so until we include a resolution param - // in info, this might cause scroll jumps for retina images? - // - // Something like: - // - // if (content.info && content.info.devicePixelRatio) { - // naturalWidth /= content.info.devicePixelRatio; - // naturalHeight /= content.info.devicePixelRatio; - // } - + // this is only used as a fallback in case content.info.w/h is missing loadedImageDimensions = { naturalWidth, naturalHeight }; } @@ -183,6 +171,9 @@ export default class MImageBody extends React.Component { // So either we need to support custom timeline widths here, or reimpose the cap, otherwise the // thumbnail resolution will be unnecessarily reduced. // custom timeline widths seems preferable. + const pixelRatio = window.devicePixelRatio; + const thumbWidth = 800 * pixelRatio; + const thumbHeight = 600 * pixelRatio; const content = this.props.mxEvent.getContent(); if (content.file !== undefined) { @@ -197,23 +188,23 @@ export default class MImageBody extends React.Component { // billion lol attacks and similar return this.context.matrixClient.mxcUrlToHttp( content.info.thumbnail_url, - 800 * window.devicePixelRatio, - 600 * window.devicePixelRatio, + thumbWidth, + thumbHeight, ); } else { - if (window.devicePixelRatio === 1.0 || + // we try to download the correct resolution + // for hi-res images (like retina screenshots). + // synapse only supports 800x600 thumbnails for now though, + // so we'll need to download the original image for this to work + // well for now. First, let's try a few cases that let us avoid + // downloading the original: + if (pixelRatio === 1.0 || (!content.info || !content.info.w || - !content.info.h || !content.info.size)) - { + !content.info.h || !content.info.size)) { // always thumbnail. it may look a bit worse, but it'll save bandwidth. // which is probably desirable on a lo-dpi device anyway. - return this.context.matrixClient.mxcUrlToHttp( - content.url, - 800 * window.devicePixelRatio, - 600 * window.devicePixelRatio - ); - } - else { + return this.context.matrixClient.mxcUrlToHttp(content.url, thumbWidth, thumbHeight); + } else { // we should only request thumbnails if the image is bigger than 800x600 // (or 1600x1200 on retina) otherwise the image in the timeline will just // end up resampled and de-retina'd for no good reason. @@ -223,25 +214,28 @@ export default class MImageBody extends React.Component { // image is both physically too large and going to be massive to load in the // timeline (e.g. >1MB). - if ((content.info.w > 800 * window.devicePixelRatio || - content.info.h > 600 * window.devicePixelRatio) && - content.info.size > 1*1024*1024) - { + const isLargerThanThumbnail = ( + content.info.w > thumbWidth || + content.info.h > thumbHeight + ); + const isLargeFileSize = content.info.size > 1*1024*1024; + + if (isLargeFileSize && isLargerThanThumbnail) { // image is too large physically and bytewise to clutter our timeline so // we ask for a thumbnail, despite knowing that it will be max 800x600 // despite us being retina (as synapse doesn't do 1600x1200 thumbs yet). - return this.context.matrixClient.mxcUrlToHttp( content.url, - 800 * window.devicePixelRatio, - 600 * window.devicePixelRatio + thumbWidth, + thumbHeight, ); - } - else { - // no width/height means we want the original image. + } else { + // download the original image otherwise, so we can scale it client side + // to take pixelRatio into account. + // ( no width/height means we want the original image) return this.context.matrixClient.mxcUrlToHttp( content.url, - }; + ); } } } From 20aedce62f7af022f2b059adbe96244536514836 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 8 Apr 2019 15:26:11 +0200 Subject: [PATCH 5/8] more lint --- src/ContentMessages.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/ContentMessages.js b/src/ContentMessages.js index dfcfbda3751..fa341465b85 100644 --- a/src/ContentMessages.js +++ b/src/ContentMessages.js @@ -26,6 +26,7 @@ import { _t } from './languageHandler'; import Modal from './Modal'; import RoomViewStore from './stores/RoomViewStore'; import encrypt from "browser-encrypt-attachment"; +// eslint-disable-next-line camelcase import png_chunks_extract from "png-chunks-extract"; // Polyfill for Canvas.toBlob API using Canvas.toDataURL @@ -132,8 +133,8 @@ function loadImageElement(imageFile) { // Once ready, create a thumbnail img.onload = function() { URL.revokeObjectURL(objectUrl); - let width = hidpi ? (img.width >> 1) : img.width; - let height = hidpi ? (img.height >> 1) : img.height; + const width = hidpi ? (img.width >> 1) : img.width; + const height = hidpi ? (img.height >> 1) : img.height; deferred.resolve({ img, width, height }); }; img.onerror = function(e) { From d400ebde657585859f3ef486e07376944ca29f24 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 8 Apr 2019 15:34:51 +0200 Subject: [PATCH 6/8] add new dependencies to lock file --- yarn.lock | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/yarn.lock b/yarn.lock index 79e79279c9a..0fb9c1b1684 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1967,6 +1967,11 @@ counterpart@^0.18.0: pluralizers "^0.1.7" sprintf-js "^1.0.3" +crc-32@^0.3.0: + version "0.3.0" + resolved "https://registry.yarnpkg.com/crc-32/-/crc-32-0.3.0.tgz#6a3d3687f5baec41f7e9b99fe1953a2e5d19775e" + integrity sha1-aj02h/W67EH36bmf4ZU6Ll0Zd14= + create-ecdh@^4.0.0: version "4.0.3" resolved "https://registry.yarnpkg.com/create-ecdh/-/create-ecdh-4.0.3.tgz#c9111b6f33045c4697f144787f9254cdc77c45ff" @@ -5211,6 +5216,13 @@ pluralizers@^0.1.7: resolved "https://registry.yarnpkg.com/pluralizers/-/pluralizers-0.1.7.tgz#8d38dd0a1b660e739b10ab2eab10b684c9d50142" integrity sha512-mw6AejUiCaMQ6uPN9ObjJDTnR5AnBSmnHHy3uVTbxrSFSxO5scfwpTs8Dxyb6T2v7GSulhvOq+pm9y+hXUvtOA== +png-chunks-extract@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/png-chunks-extract/-/png-chunks-extract-1.0.0.tgz#fad4a905e66652197351c65e35b92c64311e472d" + integrity sha1-+tSpBeZmUhlzUcZeNbksZDEeRy0= + dependencies: + crc-32 "^0.3.0" + posix-character-classes@^0.1.0: version "0.1.1" resolved "https://registry.yarnpkg.com/posix-character-classes/-/posix-character-classes-0.1.1.tgz#01eac0fe3b5af71a2a6c02feabb8c1fef7e00eab" From ea719702990488c210ed387d4ff78cffd6cfe6c0 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 9 Apr 2019 12:18:06 +0200 Subject: [PATCH 7/8] name fn to camel case --- src/ContentMessages.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/ContentMessages.js b/src/ContentMessages.js index 313e2f1ced3..96c75f1c544 100644 --- a/src/ContentMessages.js +++ b/src/ContentMessages.js @@ -26,8 +26,7 @@ import { _t } from './languageHandler'; import Modal from './Modal'; import RoomViewStore from './stores/RoomViewStore'; import encrypt from "browser-encrypt-attachment"; -// eslint-disable-next-line camelcase -import png_chunks_extract from "png-chunks-extract"; +import extractPngChunks from "png-chunks-extract"; // Polyfill for Canvas.toBlob API using Canvas.toDataURL import "blueimp-canvas-to-blob"; @@ -117,11 +116,11 @@ function loadImageElement(imageFile) { // in practice macOS happens to order the chunks so they fall in // the first 0x1000 bytes (thanks to a massive ICC header). // Thus we could slice the file down to only sniff the first 0x1000 - // bytes (but this makes png_chunks_extract choke on the corrupt file) + // bytes (but this makes extractPngChunks choke on the corrupt file) const headers = imageFile; //.slice(0, 0x1000); readFileAsArrayBuffer(headers).then(arrayBuffer=>{ const buffer = new Uint8Array(arrayBuffer); - const chunks = png_chunks_extract(buffer); + const chunks = extractPngChunks(buffer); for (const chunk of chunks) { if (chunk.name === 'pHYs') { if (chunk.data.byteLength !== PHYS_HIDPI.length) return; From 774314badd7145fe889d4a1bcc572baf922e0bf8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Tue, 9 Apr 2019 12:32:44 +0200 Subject: [PATCH 8/8] prevent png chunk parsing and image loading racing with each other --- src/ContentMessages.js | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/ContentMessages.js b/src/ContentMessages.js index 96c75f1c544..bb0852d3d6b 100644 --- a/src/ContentMessages.js +++ b/src/ContentMessages.js @@ -101,48 +101,48 @@ function createThumbnail(element, inputWidth, inputHeight, mimeType) { * @param {File} imageFile The file to load in an image element. * @return {Promise} A promise that resolves with the html image element. */ -function loadImageElement(imageFile) { - const deferred = Promise.defer(); - +async function loadImageElement(imageFile) { // Load the file into an html element const img = document.createElement("img"); const objectUrl = URL.createObjectURL(imageFile); + const imgPromise = new Promise((resolve, reject) => { + img.onload = function() { + URL.revokeObjectURL(objectUrl); + resolve(img); + }; + img.onerror = function(e) { + reject(e); + }; + }); img.src = objectUrl; // check for hi-dpi PNGs and fudge display resolution as needed. // this is mainly needed for macOS screencaps - let hidpi = false; + let parsePromise; if (imageFile.type === "image/png") { // in practice macOS happens to order the chunks so they fall in // the first 0x1000 bytes (thanks to a massive ICC header). // Thus we could slice the file down to only sniff the first 0x1000 // bytes (but this makes extractPngChunks choke on the corrupt file) const headers = imageFile; //.slice(0, 0x1000); - readFileAsArrayBuffer(headers).then(arrayBuffer=>{ + parsePromise = readFileAsArrayBuffer(headers).then(arrayBuffer => { const buffer = new Uint8Array(arrayBuffer); const chunks = extractPngChunks(buffer); for (const chunk of chunks) { if (chunk.name === 'pHYs') { if (chunk.data.byteLength !== PHYS_HIDPI.length) return; - hidpi = chunk.data.every((val, i) => val === PHYS_HIDPI[i]); - return; + const hidpi = chunk.data.every((val, i) => val === PHYS_HIDPI[i]); + return hidpi; } } + return false; }); } - // Once ready, create a thumbnail - img.onload = function() { - URL.revokeObjectURL(objectUrl); - const width = hidpi ? (img.width >> 1) : img.width; - const height = hidpi ? (img.height >> 1) : img.height; - deferred.resolve({ img, width, height }); - }; - img.onerror = function(e) { - deferred.reject(e); - }; - - return deferred.promise; + const [hidpi] = await Promise.all([parsePromise, imgPromise]); + const width = hidpi ? (img.width >> 1) : img.width; + const height = hidpi ? (img.height >> 1) : img.height; + return {width, height, img}; } /**