Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor: Changed video recording trim to happen before insertion #12022

Merged
merged 8 commits into from
Aug 8, 2022

Conversation

barklund
Copy link
Contributor

@barklund barklund commented Jul 26, 2022

Context

This moves the actual trimming of the video recording up so it happens as soon as the user presses "trim", and not only when they press "insert".

Todo

  • Make it work!
  • Fix bug around re-trimming (partially fixed, see comment below)
  • Fix UX around waiting for trim to complete (with a progress bar perhaps?)

User-facing changes

This flow now happens when trimming a new recording - the "cancel trim" button in the footer of course also works.
trimmy

Testing Instructions

Steps to validate:

  1. Start a video recording and record 10+ seconds of video
  2. Click the trim button to start trimming the recording
  3. Move any trim offset and press trim
  4. Observe that trim happens instantly with a dialog informing you of this
  5. Wait until trim completes (the shorter the faster)
  6. Observe that the displayed video is only the trimmed bit
  7. Click the trim button again
  8. Observe that the original video is available to trim from, with the previously trimmed bit "preselected"
  9. Abort trimming
  10. Observe that the original trimmed video still displays correctly
  11. Press insert
  12. Observe that the trimmed video is uploaded and inserted to the page
  13. Observe that the trimmed video has the correct duration and thumbnail

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #11988

@barklund barklund added Type: Enhancement New feature or improvement of an existing feature Pod: Prometheus labels Jul 26, 2022
@barklund barklund self-assigned this Jul 26, 2022
@swissspidy
Copy link
Collaborator

Thanks for working on this 👍

I originally had something simpler in mind without a dialog, but of course I didn't now the technical constraints around this.

Would be good to discuss this with @aaskedall to get his thoughts on this.

@barklund
Copy link
Contributor Author

I originally had something simpler in mind without a dialog, but of course I didn't now the technical constraints around this.

The dialog is of course only temporary to make it obvious that something is happening in the WIP PR. We should definitely find a better solution.

Would be good to discuss this with @aaskedall to get his thoughts on this.

I plan to once he's back from OOO later today(?).

@barklund
Copy link
Contributor Author

barklund commented Aug 3, 2022

There is still an annoying bug with re-trimming an already trimmed video (while still in the recording mode). Sometimes the original blob URL doesn't work anymore, like it has been garbage collected (or window.URL.revokeObjectURL has been called), but I don't see why that should happen?

We need to fix this bug, but it's very inconsistent, so it's pretty hard to track down.

@merapi
Copy link
Contributor

merapi commented Aug 6, 2022

We need to reset the last trimming session data, otherwise, you can end up at a dead end (no start/end markers):

image

The close button should probably work as a cancel button, right now if you don't re-enter the trimming screen before it finishes processing - your video will be lost.

Copy link
Contributor

@merapi merapi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the known re-trimming bug and some edge cases to handle - otherwise looks good 👍🏻

@barklund barklund marked this pull request as ready for review August 8, 2022 10:49
@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented Aug 8, 2022

Plugin builds for a09ede4 are ready 🛎️!

@barklund barklund changed the title [WIP] Editor: Changed video recording trim to happen before insertion Editor: Changed video recording trim to happen before insertion Aug 8, 2022
@merapi merapi mentioned this pull request Aug 8, 2022
8 tasks
Comment on lines +257 to +261
if (
previousBlobUrl &&
previousBlobUrl !== mediaBlobUrl &&
previousBlobUrl !== originalMediaBlobUrl
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm an idiot!

like it has been garbage collected (or window.URL.revokeObjectURL has been called), but I don't see why that should happen

Maybe because it has been revoked right here? 🤦 Well, I guess this fixes it though

const resetTrim = useCallback(() => {
setIsTrimming(false);
setIsAdjustingTrim(false);
setTrimData({ start: 0, end: null });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't actually reset the trim info in the reset function, so that's why it didn't work correctly.

@barklund barklund merged commit c27840b into main Aug 8, 2022
@barklund barklund deleted the add/11988-recorder-early-trim branch August 8, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media Recording - Trimming: Better user experience
5 participants