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

In encrypted room, full-size image downloaded immediately when thumbnail exists #28311

Open
richvdh opened this issue Oct 28, 2024 · 7 comments
Labels
A-E2EE A-Media O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Needs-Product More input needed from the Product team

Comments

@richvdh
Copy link
Member

richvdh commented Oct 28, 2024

  1. Open browser dev tools
  2. Receive an encrypted m.image message with a regular image and a thumbnail; do not yet open the full-size image.
  3. Note that the full-size image is downloaded as soon as the message appears on the timeline. This seems a bit redundant, when only the thumbnail is shown on-screen.
@richvdh richvdh added T-Defect S-Minor Impairs non-critical functionality or suitable workarounds exist A-E2EE A-Media O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience labels Oct 28, 2024
@dosubot dosubot bot added A-File-Download O-Uncommon Most users are unlikely to come across this or unexpected workflow labels Oct 28, 2024
@t3chguy
Copy link
Member

t3chguy commented Nov 5, 2024

This is done intentionally when the mimetype may be animated (e.g. image/png), we have to load the blob to check if it is animated to render the GIF overlay. Related #26467

@richvdh
Copy link
Member Author

richvdh commented Nov 5, 2024

This is done intentionally when the mimetype may be animated (e.g. image/png), we have to load the blob to check if it is animated to render the GIF overlay.

I'm afraid I don't really follow. Can you explain like I'm 5?

(naively: if clients have to download the full-sized image, maybe we don't need thumbnails?)

@t3chguy
Copy link
Member

t3chguy commented Nov 5, 2024

Design has asked that we show a "GIF" label in the corner of animated media, so that a user can hover/open it to play the animation.

When we encounter an image with a mimetype which may be animated ("image/gif", "image/webp", "image/png", "image/apng", "image/avif") we have to fetch the blob (because thumbnails of animated media are often not animated, e.g. Synapse) to inspect the file itself to see if it contains an animation rather than a static image to be able to do so. (Otherwise all PNGs would be shown as animations, because image/png can be animated)

@t3chguy t3chguy added the X-Needs-Product More input needed from the Product team label Nov 5, 2024
@richvdh richvdh removed O-Uncommon Most users are unlikely to come across this or unexpected workflow A-File-Download labels Nov 5, 2024
@t3chguy
Copy link
Member

t3chguy commented Nov 5, 2024

FTR the codepath isn't specific to encrypted rooms:

let isAnimated = mayBeAnimated(content.info?.mimetype);
// If there is no included non-animated thumbnail then we will generate our own, we can't depend on the server
// because 1. encryption and 2. we can't ask the server specifically for a non-animated thumbnail.
if (isAnimated && !SettingsStore.getValue("autoplayGifs")) {
if (!thumbUrl || !content?.info?.thumbnail_info || mayBeAnimated(content.info.thumbnail_info.mimetype)) {
const img = document.createElement("img");
const loadPromise = new Promise((resolve, reject) => {
img.onload = resolve;
img.onerror = reject;
});
img.crossOrigin = "Anonymous"; // CORS allow canvas access
img.src = contentUrl ?? "";
try {
await loadPromise;
} catch (error) {
logger.error("Unable to download attachment: ", error);
this.setState({ error: error as Error });
return;
}
try {
const blob = await this.props.mediaEventHelper!.sourceBlob.value;
if (!(await blobIsAnimated(content.info?.mimetype, blob))) {
isAnimated = false;
}
if (isAnimated) {
const thumb = await createThumbnail(
img,
img.width,
img.height,
content.info?.mimetype ?? "image/jpeg",
false,
);
thumbUrl = URL.createObjectURL(thumb.thumbnail);
}
} catch (error) {
// This is a non-critical failure, do not surface the error or bail the method here
logger.warn("Unable to generate thumbnail for animated image: ", error);
}
}
}

@richvdh
Copy link
Member Author

richvdh commented Nov 5, 2024

This feels a bit of a cumbersome solution to the problem. How about having the sender flag the media as animated or not?

@t3chguy
Copy link
Member

t3chguy commented Nov 5, 2024

Sure that would be better, but for an indefinite transition period until the majority of bots, bridges, and clients do so we would need to keep this solution

@richvdh
Copy link
Member Author

richvdh commented Nov 5, 2024

FTR the codepath isn't specific to encrypted rooms:

It really didn't seem to happen in plaintext rooms, though it's possible I was just failing at computers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-E2EE A-Media O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Needs-Product More input needed from the Product team
Projects
None yet
Development

No branches or pull requests

2 participants