feat(ui): display video thumbnail

[willybrauner]

Description

This PR is about displaying a video thumbnail in Thumbnail component if the uploaded file has mimeType video/*. To avoid performance issue, the video is not played, only the first frame is displayed from a <video> tag, but it can be managed differently (with intersection observer, click handler etc.)

When you manage a large collection of assets/files, it is very difficult to quickly find a video by file name, video preview can help to identify the file by its first frame.

After change :

video-thumbnail.mov

• I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

• New feature (non-breaking change which adds functionality)

Checklist:

• Existing test suite passes locally with my changes

[willybrauner]

Are there any clarifications I can provide or changes that should be made to this?

[denolfe]

Revisiting this one. Are you able to resolve merge conflicts @willybrauner ?

[willybrauner]

@denolfe it's rebased on beta.

[willybrauner]

@denolfe I updated this PR with the last beta. One unit test failed, but I'm not sure to know how to fix it.

[denolfe]

Since the failing e2e test is uploads, it's likely either a legitimate failure or the test needs to be updated (could just be a small selector tweak).

To troubleshoot:

1. Download the artifact for uploads tests on the github action result

2. Unzip artifacts

3. Drop trace.zip into https://trace.playwright.dev/

4. View failure.

If I had to guess, it's likely looking for a selector that was changed/removed.

[willybrauner]

@denolfe I have improved the Thumbnail component by mixing Thumbnail & ThumbnailComponent who was shared the same logic. I just comment the breaking test for now.

[paulpopus]

For some reason I can't checkout to this PR locally whatsoever...

However, you should comment that test back in because it checks for the existence of the thumbnail.

Your code specifically shows nothing if the filetype is neither an image or a video, instead to keep our backwards compatibility, you should turn this into a ? : render. Render video if the filetype is video and render the thumbnail otherwise. For documents we render an icon for example.

{fileExists && fileType === 'image' && <img alt={alt} src={src} />}

I'm not sure why the test would fail in this case anyhow

[willybrauner]

@paulpopus nice catch, you're right about the image fallback, I have fixed it in this way.

@denolfe. Thanks for the test usage explanation; I was able to run and trace it locally. The initial error was due to the preload "if else" statement, depending to the mimeType, on the same way than the render function.

I have finally implemented an alternative than the <video> tag in order to keep a simple <img/> rendering. I capture a video frame by drawing it on a canvas. This seems like a better option since many video tags will cause performance issues if a large list of thumbnails needs to be rendered at once.

[willybrauner]

Seems to be pretty quick with this method :)

screen.mov

[willybrauner]

@paulpopus @denolfe is this approach could be a right one for you?

[paulpopus]

@willybrauner interesting pattern, I don't think we're opposed to it here, though this logic would need to be moved behind an observer since it would likely harm performance when viewing 50-100 thumbnails at once, which some projects using payload do.

We have this intersect hook if you manage to use it https://github.com/payloadcms/payload/blob/beta/packages/ui/src/hooks/useIntersect.ts#L13

[willybrauner]

@paulpopus I have rebase this branch on the last beta and implemented the observer. One new test failed but strangely, it continue to failed locally when I checkout on the previous commit.

[paulpopus]

I think that's a flaky test so you can ignore it, now sorry to yoyo you back in this PR but we've had some performance issues with thumbnails in the bulk editor (trying to upload 50+ items at once) and I wonder if we may see the same problem here.

See this PR: #8944

So we're sticking to a similar pattern, rendering a thumbnail onto an offscreen canvas but it's done higher up so that it can be handled sequentially so it doesn't block the main UI thread.

Anyway that's to say, we could probably move the thumbnail logic into that new createThumbnail utility and move it higher up in the form manager. Thoughts?

[willybrauner]

@paulpopus yes sure. Now we have this ˋcreateThumnail` function, the video thumbnail should be generated into it as for images I guess. Let me try to re implement it in the next days.

[willybrauner]

type image/jpeg instead of image/png could display a full back thumbnail in case the image has a transparent background like this

before:

after:

[willybrauner]

@paulpopus In last commit, my logic migration inside the createThumbnail function. I refactored the base 64 url ​​generation so that it is common to all types of media.

Some notes:

• createThumbnail now takes file and fileSrc as params. We use either one, it depends on where it's called. Inside Thumbnail component, we do not have access to file (isn't it?)
• we should replace base64 url image type image/jpg by image/png to cover more upload case I guess. link above
• need to fix tests

[paulpopus]

@willybrauner Thanks for the updates

1. it takes both but File is whats important and used. You could make it a new prop type so file isnt required if fileSrc is provided
2. Yeah I'm fine with that, and increase maxDimension to 420 by default so its less pixelated in some places
3. Yep!

I tested this PR as it is so I think we're nearly ready to go, if you make the above changes and fix tests we can merge it!

Edit:

To add 4. when initially uploading a video, it's not showing the preview and just shows the document icon, there we might be able to pass File as well to create a thumbnail immediately for the user

[willybrauner]

Due to a side effect of the new hasPreloaded state in Thumbnail

before:

Screen.Recording.2024-11-07.at.18.41.47.mov

after add src as key to force rerender:

Screen.Recording.2024-11-07.at.18.42.34.mov

[willybrauner]

@paulpopus That was the reason why video thumbnail wasn't display before we saved the media.

[willybrauner]

Hi @paulpopus,

1. I continue to use fileSrc as createThumbnail second params in some case, because File is not always provided by the parent component. fileSrc is sufficient for generate a new thumbnail according to my tests. Tell me if you find a limitation there.
2. maxDimension has been set to 420 & mimeType to image/png
3. I fixed the thumbnail displayed in FileSidebar and File component in order to show the preview before save the media commented here.
4. need some time to manage new tests, because some of them test a regular src media, not a base64. It will be more complicated to test image upload outputs in DOM.

[github-actions]

This PR is stale due to lack of activity.

To keep the PR open, please indicate that it is still relevant in a comment below.

[willybrauner]

Sorry, I couldn't adapt the e2e tests properly. But the feature has been developed and is working.

[denolfe]

Thank you for your patience on this one @willybrauner . I still think this would be a good one to get in. We'll come up with a way forward.