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

set 'video/quicktime' to 'video/mp4' for uploads #4495

Merged
merged 1 commit into from Apr 12, 2022
Merged

set 'video/quicktime' to 'video/mp4' for uploads #4495

merged 1 commit into from Apr 12, 2022

Conversation

ghost
Copy link

@ghost ghost commented Feb 24, 2022

video/quicktime only plays in Firefox and Safari.
video/mp4 plays in Firefox, Safari, and Chromium-based browsers.

`video/quicktime` only plays in Firefox and Safari.
`video/mp4` plays in Firefox, Safari, and Chromium-based browsers.
@ghost ghost marked this pull request as draft February 24, 2022 15:57
@ghost ghost changed the title set 'video/quicktime' to 'video/mp4' set 'video/quicktime' to 'video/mp4' for uploads Feb 24, 2022
@ghost
Copy link
Author

ghost commented Feb 24, 2022

Test:

image

The top link was uploaded from The Lounge with this PR. The bottom link was uploaded from stable (4.3.0).

@ghost
Copy link
Author

ghost commented Feb 24, 2022

I suppose this merits some additional discussion.

There are still .mov files that are served with video/quicktime around the internet, primarily Discord does this. In all my testing, a .mov with video/quicktime would not play in any Chromium-based browser (but played fine in Firefox and Safari), but switching it to video/mp4 made it so the files would play in all 3 major browser types.

Should this PR also be expanded so TL embeds video/quicktime even though Chromium-based browsers do not support it?

@ghost
Copy link
Author

ghost commented Feb 24, 2022

Test be8a853:

image

Left: Edge | Right: Firefox

Vivaldi also a no-go on embedding video/quicktime:

image

@ghost
Copy link
Author

ghost commented Feb 24, 2022

I suppose it looks bad for something to indicate it should be embedding...and then not, though that's the existence I'm used to on iOS since it doesn't support video/audio embeds at all, only images. 😅

Edit: I don't think Firefox or Safari users should have to suffer just because Chromium doesn't support something...but that's just my opinion, I guess.

@ghost ghost marked this pull request as ready for review February 24, 2022 16:44
@itsjohncs
Copy link
Member

Took me some time to understand what's happening here, so I want to check my understanding with you @xnaas:

Currently on master we won't show previews for any videos with the video/quicktime mimetype. This change aims to support previewing such videos.

Because the video/quicktime mime type is poorly supported by browsers, we'll avoid serving videos that our filetype detector marks with that mimetype and we'll instead serve them as video/mp4.

External links served with video/quicktime will show previews as well but this will only work on Firefox and Safari. On other browsers the embed arrow will be shown but the embed will not work.

--

Am I understanding everything you're trying to do here @xnaas?

@itsjohncs itsjohncs self-assigned this Mar 1, 2022
@ghost
Copy link
Author

ghost commented Mar 2, 2022

That is the route I went with this, yes, @itsjohncs.

@itsjohncs
Copy link
Member

Thanks. I'm hesitant about trying to preview external video/quicktime links. Chromium browsers are pretty popular and this change would be knowingly introducing a bug for them.

Is there a way to hide the preview on the client once it's clear that the preview can't be loaded?

@ghost
Copy link
Author

ghost commented Mar 2, 2022

Chromium browsers are pretty popular and this change would be knowingly introducing a bug for them.

All video embeds have this bug on mobile Safari, which could be up to 20% of users. 😜 I get the point, though. Would be nice for anyone who cares about Chromium to maybe bump Chromium issue #1120292 and mention this, dunno.

Is there a way to hide the preview on the client once it's clear that the preview can't be loaded?

That, unfortunately, will require feedback from another maintainer. I wouldn't have a clue. 😅 (and per above mention of iOS issue, would love something for this)

Copy link
Member

@itsjohncs itsjohncs 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 we ought to remove the part of this PR that allows external links served with video/quicktime to be served then. That seems like something that ought to be done only after we can handle broken embeds on the client.

If you'd like to pass the torch to someone else to implement that at some point: making an issue and hoping it attracts someone seems like a good enough route.

@ghost
Copy link
Author

ghost commented Mar 2, 2022

I "un-did" the 2nd commit, which contained the "offending" code.

@ghost ghost mentioned this pull request Mar 2, 2022
Copy link
Member

@itsjohncs itsjohncs left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me. Though I didn't test this myself since it seems like the testing you've described is sufficient.

We're waiting to merge anything until after we make a release, but we'll merge this after that.

@itsjohncs itsjohncs added this to the 4.3.2 milestone Mar 2, 2022
@itsjohncs itsjohncs added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Mar 2, 2022
@brunnre8 brunnre8 added the Status: release-after-next reviewed and ready to merge, postponed to release after next (feature freeze of current release) label Mar 9, 2022
@MaxLeiter MaxLeiter removed the Status: release-after-next reviewed and ready to merge, postponed to release after next (feature freeze of current release) label Apr 12, 2022
@MaxLeiter MaxLeiter merged commit 57b1e51 into thelounge:master Apr 12, 2022
@ghost ghost deleted the qt-to-mp4 branch April 12, 2022 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants