Video: Allow re-trimming of already trimmed video #9315
Conversation
barklund
added
Type: Enhancement
# Conflicts: # packages/story-editor/src/components/videoTrim/provider.js # packages/story-editor/src/components/videoTrim/useVideoNode.js # packages/story-editor/src/components/videoTrim/videoTrimmer.js
|
Size Change: +839 B (0%) Total Size: 4.05 MB
ℹ️ View Unchanged
|
|
@barklund I'm getting this error currently: With the following REST response (might be since there were some API changes in #8513, I think, but not sure) (cc @spacedmonkey since you implemented the API part of trimming): Also, I wasn't able to test it yet due to the previous error but just wanted to confirm that the same video can be trimmed more than once and the initial video is always used. Part of the logic in the original PR you had questions about (in terms of necessity) was for ensuring that the original video is used for the second and third etc. trim as well. Would be good to add that case to the testing instructions as well (trimming the video more than once and ensuring the same origin video is always used). |
packages/story-editor/src/components/videoTrim/useVideoTrimMode.js
Outdated
Show resolved
Hide resolved
Looks like the PR works in the QA environment so it seems like the error is just happening locally.
|
I think this issue will be fixed in #9301. Do you have the unsplash plugin installed out of interest? |
packages/story-editor/src/components/videoTrim/useVideoTrimMode.js
Outdated
Show resolved
Hide resolved
Hm, I did have it installed and enabled, didn't even remember. After disabling it there's no error locally anymore. |
|
Seeing something weird. I trimmed a video to 1 second. Then retrimmed to 5 seconds. After the upload is complete, I am still seeing the 1 second video when I play it. Screen.Recording.2021-10-07.at.13.35.46.movAm I missing something here? |
@spacedmonkey This is the same bug as reported here: #9315 (comment) |
I'm looking into this now. There's definitely a bug here somewhere, probably something trivial. |
| if (!resource) { | ||
| return; | ||
| } | ||
| const lengthInSeconds = Math.round(endOffset / 1000 - startOffset / 1000); | ||
| trimExistingVideo({ | ||
| resource: { | ||
| ...resource, | ||
| // Preserve the id of the source resource even though we might be trimming from a third resource | ||
| id: element.resource.id, |
There was a problem hiding this comment.
Can you explain this a little more.
There was a problem hiding this comment.
Yeah, it's a bit messy, but we need both IDs in the media processing function.
This ID here is the resource from which to copy the information over to the new element. This is considered the source for all data except the actual raw video. It is used in updateExistingElements many times. It is also the element that is temporarily marked as being "trimmed", though in effect it is not necessarily this element's video that is being trimmed.
The other ID listed a few lines below, trimSourceId, is to be considered the ID of the resource of the raw video, that is to be processed into a new video. It is used as the original key in the trim data for the newly create resource.
I would be good to clean this up a bit more, but I'm not really sure how to do that...
There was a problem hiding this comment.
Different naming might be useful.
There was a problem hiding this comment.
I reorganized it a bit in 9afe07f - does that make it clearer? I separate it between the resource and the canvas resource. The former is the actual resource where working on, where the canvas resource is the element on the canvas that should be cloned into a new element with the resulting video.
| actions.getMediaById = useCallback( | ||
| (mediaId) => getMediaById(mediaId, media), | ||
| [getMediaById, media] | ||
| ); |
There was a problem hiding this comment.
@sayedtaqui Flagging will need to taken in account for #9318
|
What if you do this?
Would the 45 second video be muted? Should it be trimmed? |
Good question! Sounds like a product question that is best resolved in a follow-up ticket. As a user I would expect it to be still muted. And if it was optimized before, it should be still be optimized. Everything else would be super confusing. The question is how we can achieve this? Sounds like we'd need to refactor the media upload queue and perhaps even useFFmpeg quite a bit to basically do trimming & muting in one step... Hence best done in a follow-up. |
Yeah, I was thinking about that too somewhere along the way. All of this would be trivial if we moved all video transcoding to a "rendering" step just before publishing/updating, but that's not where we are right now. In our current setup, this is a bit tricky. But let's file that as a follow-up ticket. |
There was a problem hiding this comment.
LGTM!
One potential improvement:
If the user is trying to re-trim the video to the same length that the original video is, perhaps there's no need to create a new video and just the old video can be used? E.g. the user re-trims a 30s video to 5s and then back to 30s, currently it creates a new 30s video.
Yes, in general if you create a trimmed video, of which we already have an identical copy in the library, it would make sense to re-use that rather than make a new one. But your example is of course even more simple, that if you want to return to the original, we rather than reuse said original instead just make a duplicate, which is pretty stupid. I'll file as a follow-up. |
|
@swissspidy, what do you say about either caching the API response for the get media request here or making a very lightweight API request for just the video source URL, as I think that's the only thing we need here? Currently, the loading time on my setup is about 0.5-1.0 seconds, which is pretty long to wait every time I press to re-trim a video. Or is it just my local virtual environment, that's slow? I'd file it as a follow-up enhancement, but not sure how to best file it, as I don't know what would be the recommended path forward? |
|
@barklund If you only need the source URL and nothing else, you can use the |
swissspidy
changed the title
Context
This implements re-trimming an already trimmed video by abstracting out which video is being trimmed completely from most components.
To-do
Some things that could be added, but don't have to be:
User-facing changes
Trimming a video, that has already been trimmed, now shows the full original video with the handles placed correctly:
Testing Instructions
This PR can be tested by following these steps:
Checklist
Type: XYZlabel to the PRFixes #8892