Show more videos and consolidate media logic #791

carloscheddar · 2023-10-17T03:56:08Z

Given a Lemmy post containing media this will try to display the content inside the app. This uses the existing image and video renderers and adds a new iframe for video embeds.

Features

Render videos via embed_video_url
Automatically transform media urls into one that we can render or embed (.gifv -> .mp4, etc.)
Allow rendering .mov urls as videos
Add error handling for images and videos as well as fallbacks
Consolidate media logic into a single Media component used by both PostDetail and LargePost
Add isUrlMedia to easily check for media instead of manually checking for image, video or embed
Add tests for the url helpers. Helps with the url transformations since these can increase

Fallbacks

When an image url fails to load try rendering the thumbnail instead
When a video fails to load via embed_video_url, try rendering the url
When everything fails to load, display the url

aeharding · 2023-10-20T04:01:02Z

Thanks for the PR! I'll take a look as soon as I can

aeharding · 2023-10-26T17:37:19Z

Hi there, thanks again for the PR!

So, I have a couple concerns about the approach taken in this PR.

We should never load iframes to content such as YouTube in the app without the user requesting to do so (for example, by tapping a static preview picture). This is the approach that Mastodon takes, and its best for security and privacy.
Ideally tapping the preview picture would bring up the lightbox with the same behavior that images have. This is what Apollo does. Note, Apollo doesn't load iframes in the app. It somehow extracts the Youtube video data and puts it into its own player. This is a much better experience, and better for privacy reasons, but may be hard to implement.

Overall this isn't the approach I'd like to take for Voyager, but I do appreciate your efforts here! Let me know if you have any questions on the above.

carloscheddar · 2023-10-27T02:10:15Z

@aeharding I think your concerns about iframes are valid I even forgot to add the sandbox attribute! I'll remove the embed logic from this PR and revisit that in the future.

I think the biggest benefit of this PR is the consolidation and error handling for images and videos as well as being able to render video files from the embed_video_url that Lemmy provides.

Let me know if you have any comments on those.

aeharding · 2023-10-27T03:01:02Z

There was a previous PR playing with support for embed_video_url in the past, see here: https://github.com/aeharding/voyager/pull/571/files

The problem with that at the time was a lot of embed_video_url returned by lemmy is invalid, which is why Voyager doesn't support it at the moment. I don't remember why exactly, maybe browsers had trouble handling the media files. Might be worth checking again though

carloscheddar · 2023-10-27T03:08:23Z

There was a previous PR playing with support for embed_video_url in the past, see here: https://github.com/aeharding/voyager/pull/571/files

The problem with that at the time was a lot of embed_video_url returned by lemmy is invalid, which is why Voyager doesn't support it at the moment. I don't remember why exactly, maybe browsers had trouble handling the media files. Might be worth checking again though

Yup that was my previous PR, this fixes that issue thanks to the fallback logic via the onError events. I just went a little overboard and tried to add embeds too 😆 . Just finished removing those so this is ready for review again.

aeharding · 2023-10-28T19:39:42Z

I see, thanks!

This fixes issues with embed links that could've been rendered using the Video component

This fixes an issue on the LargePost where image would try to render broken thumbnails instead of the working image url.

- This fixes an issue where .gifv would not be considered a video

This checks and renders media types and in the case of videos will try the embed url as well as the normal url if one fails to play. Videos and images render with the gallery created for the LargePost component Embeds are sized in a 16/9 ratio since we can't fetch their size with this implementation. Communicates errors to parent component via callback to hide media if it fails to play.

- Remove embed url helpers - Improve error handling on PostDetail

aeharding · 2023-11-15T00:25:05Z

src/helpers/url.ts

+// Transform known video platform urls into their embed counterparts
+export const transformUrl = (inputUrl: string): string => {
+  // If the URL contains gifv replace it with mp4 in order for the video to play inline
+  const url = inputUrl.replace(/\.gifv/g, ".mp4");


Is this fixing a particular site or set of sites? I would like to avoid changing any instance of .gifv in the url, that feels quite dangerous.

aeharding · 2023-12-12T23:23:51Z

I am closing this because it has become stale and there have been various fixes made to the video player over the past month or so. Thank you though for the PR!

carloscheddar force-pushed the main branch from 4a337cb to 5647e77 Compare October 25, 2023 04:00

carloscheddar force-pushed the main branch from 5647e77 to c61a246 Compare October 27, 2023 03:04

carloscheddar changed the title ~~Show videos and embeds inside the app when possible~~ Show more videos and consolidate media logic Oct 27, 2023

carloscheddar force-pushed the main branch from c61a246 to 6a5eee4 Compare October 30, 2023 01:46

carloscheddar added 16 commits November 6, 2023 21:55

Load video with the embed_video_url if it exists

ca9b7a1

This fixes issues with embed links that could've been rendered using the Video component

Add ability to handle onError for Video component

859679b

Place url helpers in url file and add isUrlMedia function

926c733

Fix testing extend-expect import for vitest

24f7b2d

Return true when checking .mov video urls

08d3ab2

Add support for other youtube url patterns and simplify transformUrl

df4e84c

Replace .gifv with .mp4 to be able to autoplay the video

1db077e

Add thumbnail prop to render the thumbnail or the main image

cdf7127

This fixes an issue on the LargePost where image would try to render broken thumbnails instead of the working image url.

Add tests for the url helpers and test various url transformations

12d1e44

Handle empty url strings when checking media

e34d304

Transform urls before checking media

b17fb66

- This fixes an issue where .gifv would not be considered a video

Consolidate LargePost and PostDetail using the Media component

5eaf8b1

Fallback to thumbnails if the base image fails to load

52f619c

Add error handling to GalleryImg and Media images

63be345

Remove iframe embed functionality from the Media component

87c3acd

- Remove embed url helpers - Improve error handling on PostDetail

carloscheddar force-pushed the main branch from 6a5eee4 to 87c3acd Compare November 7, 2023 02:03

aeharding reviewed Nov 15, 2023

View reviewed changes

aeharding closed this Dec 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show more videos and consolidate media logic #791

Show more videos and consolidate media logic #791

carloscheddar commented Oct 17, 2023 •

edited

Loading

aeharding commented Oct 20, 2023

aeharding commented Oct 26, 2023

carloscheddar commented Oct 27, 2023

aeharding commented Oct 27, 2023

carloscheddar commented Oct 27, 2023 •

edited

Loading

aeharding commented Oct 28, 2023

aeharding Nov 15, 2023

aeharding commented Dec 12, 2023

Show more videos and consolidate media logic #791

Show more videos and consolidate media logic #791

Conversation

carloscheddar commented Oct 17, 2023 • edited Loading

Features

Fallbacks

aeharding commented Oct 20, 2023

aeharding commented Oct 26, 2023

carloscheddar commented Oct 27, 2023

aeharding commented Oct 27, 2023

carloscheddar commented Oct 27, 2023 • edited Loading

aeharding commented Oct 28, 2023

aeharding Nov 15, 2023

Choose a reason for hiding this comment

aeharding commented Dec 12, 2023

carloscheddar commented Oct 17, 2023 •

edited

Loading

carloscheddar commented Oct 27, 2023 •

edited

Loading