payload icon indicating copy to clipboard operation
payload copied to clipboard

feat(ui): display video thumbnail

Open willybrauner opened this issue 1 year ago • 19 comments

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 :

https://github.com/user-attachments/assets/c43eecbb-8698-4319-85f8-b12e36af2663

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

Type of change

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

Checklist:

  • [x] Existing test suite passes locally with my changes

willybrauner avatar Jul 26 '24 08:07 willybrauner

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

willybrauner avatar Jul 29 '24 10:07 willybrauner

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

denolfe avatar Aug 22 '24 23:08 denolfe

@denolfe it's rebased on beta.

willybrauner avatar Aug 23 '24 19:08 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.

willybrauner avatar Oct 10 '24 08:10 willybrauner

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 CleanShot 2024-10-10 at 15 43 25

  2. Unzip artifacts image

  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. CleanShot 2024-10-10 at 15 44 44

denolfe avatar Oct 10 '24 19:10 denolfe

@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.

willybrauner avatar Oct 14 '24 06:10 willybrauner

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

paulpopus avatar Oct 14 '24 20:10 paulpopus

@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.

Capture d’écran 2024-10-15 à 10 40 36

willybrauner avatar Oct 15 '24 08:10 willybrauner

Seems to be pretty quick with this method :)

https://github.com/user-attachments/assets/3c76bd10-3ef7-4204-8952-377640d62698

willybrauner avatar Oct 16 '24 08:10 willybrauner

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

willybrauner avatar Oct 21 '24 15:10 willybrauner

@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

paulpopus avatar Oct 28 '24 18:10 paulpopus

@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.

willybrauner avatar Oct 30 '24 11:10 willybrauner

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: https://github.com/payloadcms/payload/pull/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?

paulpopus avatar Oct 30 '24 17:10 paulpopus

@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 avatar Nov 02 '24 17:11 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

willybrauner avatar Nov 03 '24 15:11 willybrauner

@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

paulpopus avatar Nov 05 '24 21:11 paulpopus

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.

willybrauner avatar Nov 07 '24 21:11 willybrauner

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.

github-actions[bot] avatar Dec 12 '24 05:12 github-actions[bot]

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

willybrauner avatar Dec 12 '24 08:12 willybrauner

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.

denolfe avatar Dec 24 '24 01:12 denolfe