Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Do not preload encrypted videos|images unless autoplay or thumbnailing is on #5352

Merged
merged 4 commits into from
Oct 28, 2020

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Oct 23, 2020

partially fixes element-hq/element-web#6710

(The first commit typescriptify's MVideoBody)

This changes MVideoBody so that the file is not immediately downloaded & decrypted in the browser when the element is first shown. Instead, the user must click play on the video player. This should be mostly seamless aside from the incorrect timestamp on the video player.

Preview:
output

@Half-Shot
Copy link
Contributor Author

A surprise along the way, turns out that alt isn't valid on <video> elements. I've changed it to title=, but happy to hear alternatives.

@MatthewCroughan
Copy link

Yes!

@Half-Shot Half-Shot requested a review from a team October 23, 2020 15:55
@MatthewCroughan
Copy link

Does this not also apply to images and other media? This is how my friend encountered this issue, via images, not video preloads.

@Half-Shot
Copy link
Contributor Author

This needs to be done for images and audio too.

@Half-Shot
Copy link
Contributor Author

Latest change includes the same treatment for images, where images will not download IF the user has disabled thumbnailing.

@Half-Shot Half-Shot changed the title Do not preload encrypted videos unless autoplay is on Do not preload encrypted videos|images unless autoplay or thumbnailing is on Oct 23, 2020
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I think this does the right thing?

Comment on lines +157 to +159
this.setState({
error: "No file given in content",
});
Copy link
Member

Choose a reason for hiding this comment

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

this could be collapsed to just 1 line, but it ultimately is personal preference.

@turt2live turt2live merged commit 0adb920 into develop Oct 28, 2020
@Techjar
Copy link

Techjar commented Oct 28, 2020

This is great. I really appreciate you fixing this, because my internet connection is absolutely awful, which means whenever someone sent large media it just got completely trashed without any way to fix it other than to stop using matrix.

One thing I'm confused about: what exactly is thumbnailing? What functionality do I miss out on by disabling it?

@turt2live
Copy link
Member

One thing I'm confused about: what exactly is thumbnailing? What functionality do I miss out on by disabling it?

It's just a smaller version of an image/video to represent it. It should happen automatically in the background.

#element-web:matrix.org is a better place to ask for support.

const content = this.props.mxEvent.getContent();
if (!content.file) {
this.setState({
error: "No file given in content",
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be i18n'd?

Copy link
Member

Choose a reason for hiding this comment

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

good point. I'd be happy for a commit to fix it to land directly on develop

jryans added a commit that referenced this pull request Nov 19, 2020
For Chrome-based browsers, it seems we need to set some non-empty `src` URI for
the video element's play button to be enabled, so this crafts an empty `data`
URI and ensures playing is triggered once the real content has been fetched.

Fixes element-hq/element-web#15694
Regressed by #5352
jryans added a commit that referenced this pull request Nov 19, 2020
For Chrome-based browsers, it seems we need to set some non-empty `src` URI for
the video element's play button to be enabled, so this crafts an empty `data`
URI and ensures playing is triggered once the real content has been fetched.

Fixes element-hq/element-web#15694
Regressed by #5352
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not automatically download large media in encrypted rooms
5 participants