Media: Video Cropping Prototype #12156
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timarney
commented
Aug 23, 2022
packages/story-editor/src/app/rightClickMenu/menus/foregroundMediaMenu.js
Outdated
Show resolved
Hide resolved
timarney
commented
Aug 23, 2022
timarney
commented
Aug 23, 2022
|
squares.mp4 |
|
@miina can you take a look at this ? I believe the cropping / sizing of the element resource seems to be working - but for sure replacing the element on the canvas (sizing) isn't correct. Feel free to push commits. |
|
One of the issues, if poster's are generated here. web-stories-wp/packages/story-editor/src/app/media/useUploadMedia.js Lines 299 to 310 in 9837565 We may have skip this poster generate here. |
…index.js Co-authored-by: Jonny Harris <[email protected]>
Co-authored-by: Jonny Harris <[email protected]>
|
Size Change: +1.06 kB (0%) Total Size: 2.68 MB
ℹ️ View Unchanged
|
We don't want to do this, as this will force all uploads regenerate poster, even what it is not needed. |
Ya --- was just checking to see if it would generate. The poster image now works passing the posterFile as you mentioned. 5d0a166#diff-9c155bd24fb5ad4e011d29436780159da028af415163492b570c5fcdbf065980R425 |
|
Also, the poster image / dimisions should be forced to regenerate earlier. It is weird it jumps in later. @spacedmonkey -- are you still seeing a jump? I've seen it --- but only once in a while. Will there ever be away to restore the original video? No that I'm aware of This says uploading when it should say transcoding. Took a quick look --- do we have other instances where that tool tip changes? I only found the "Uploading media..." code |
Screen.Recording.2022-09-13.at.12.38.46.movWhy does the orignal video disappear? We should keep the original video. |
|
@timarney I have hope you don't mind, but spent some time fixing the poster generation issue. It took ages work it out. Just generation is much cleaner now. |
|
I am seeing the video change position after the transcode is finished. Screen.Recording.2022-09-13.at.14.41.15.movScreen.Recording.2022-09-13.at.14.40.31.mov |
|
Seems to vary depending on the video ? Latest test --- the second video I tried sets itself to a background video after cropping https://www.youtube.com/watch?v=5k1dWqXWHWE ☝️ .... I did some further testing for the issue I was seeing with the background video after cropping. This seems to happen if I don't let the original upload to finish before I try cropping. If I let the upload finish the crop + positioning seems to be working fine https://www.youtube.com/watch?v=Balt8mLZyn8 |
swissspidy
approved these changes
Sep 14, 2022
packages/story-editor/src/app/rightClickMenu/menus/foregroundMediaMenu.js
Outdated
Show resolved
Hide resolved
spacedmonkey
approved these changes
Sep 14, 2022
There was a problem hiding this comment.
Two small nitpics. But apart from that, this is looking good. Things to follow up.
- A rethink of how poster generation happens. While the video is processing, the poster looks wrong and the correct poster image kicks in later.
- Positioning issue after the cropping is finished.
- Post meta, have a link to the original attachment id.
- Possibility to restore the original video?
- Improve testing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
As per #5985, implement video cropping to cut off-screen parts of videos
Summary
Adds cropping for off canvas top, right, bottom, left i.e. hidden or off-canvas video
https://www.youtube.com/watch?v=P9Co7r7Droo
Relevant Technical Choices
To-do
User-facing changes
Testing Instructions
Created this video - each square is 80px X 80px
This PR can be tested by following these steps:
Enable support for cropping cut off-screen parts of videosCrop off-screen video partsfrom the Right Click MenuReviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #12093