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

Media Upload Queue #6541

Merged
merged 14 commits into from Mar 9, 2021
Merged

Media Upload Queue #6541

merged 14 commits into from Mar 9, 2021

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Feb 26, 2021

Context

Videos that need to be transcoded during upload cannot be previewed in the editor/library until they have been processed by FFmpeg. In that scenario, we currently don't display anything until transcoding is finished, which means the user has to wait a long time for feedback.

For improved UX around video optimization during upload, we want to display a placeholder image first and replace it with an actual preview as soon as it gets available—all while the upload is still progressing behind the scenes.

Unfortunately, the current media upload queue is not very stateful in that regard and does not allow for such updates in the middle of the process, nor does the consumer even know what state an individual file is currently in.

Summary

This PR introduces a new hook to act as a queue for media uploads where we know the state (e.g. pending/transcoding/uploading/finished/cancelled) of each file being uploaded at all times.

Not only does this allow us to update media resources mid-progress, but we can now also do things like displaying tooltips for items still being processed (#6089).

Regular image and video uploads should not be affected by this change

The most significant change is when uploading video files that need to be transcoded, for example a MOV file from iPhone.

For such files, the process looks a bit like follows:

  1. Drop video into editor
  2. A placeholder gray square is displayed on the editor (because we don't know its dimensions yet)
  3. Video is being transcoded
  4. Two things now happen in parallel:
    1. Try to replace placeholder with an actual preview of the image and update its dimensions once they become known
    2. Upload video and replace any placeholder or preview with the uploaded file

Relevant Technical Choices

  • Media Provider: new prependMedia action to add items to the beginning of the media library. This avoids having to replace the whole list of items with setMedia, which is error prone if using outdated items
  • Media Provider: update updateMediaElement to allow overriding the resource's ID. This is necessary because local resources (i.e. still being uploaded) now have an ID already, but after upload they will have a WordPress attachment ID.
  • Resource Type: a resource now has a new isPlaceholder property to identify whether it's a blank
  • Story Provider: adds a new deleteElementsByResourceId action to delete elements by resource ID across all pages (deleteElementsById only works on the current page) in case upload failed. This is needed because the user could have duplicated/copied elements/pages while upload was in progress.
  • useTranscodeVideo: moves the time & error tracking closer to the source
  • Canvas: do not display video playback button for placeholders
  • useUploader: separate file validation and actual uploads into two separate validateFileForUpload and uploadFile functions
  • useUploader: move the useTranscodeVideo usage to useUploadMedia because it's actually only used and needed by that. useUploader should not be concerned with transcoding.
  • useUploadMedia: validate files for upload before doing anything, so the user has more instant feedback
  • useUploadMedia: try to generate preview once at the beginning (as it works for most files) and again once transcoding has finished

To-do

  • More tests for the media upload queue
  • Improve/clarify position and behavior for placeholder resources on canvas / in media library
  • Testing & bug fixes as we go

User-facing changes

Uploading a regular image

(no visible change)

PNG.file.mov

Uploading a regular video

(no visible change)

MP4.File.mov

Uploading a video requiring additional transcoding

(see placeholder element before preview later on)

MOV.HEVC.mov

Uploading a video requiring additional transcoding but with poster

(see poster image before preview later on)

MOV.File.H264.mov

Testing Instructions

QA

  • This is a non-user-facing change and requires no QA

Prerequisites

  1. Download some test files here:
    test files.zip

  2. Turn on video optimization experiment and enable video optimization in settings

ZIP file, dropped on media library

  1. Drop ZIP file onto media library
  2. See snackbar message saying that the file type is not supported

ZIP file, dropped on editor

  1. Drop ZIP file onto editor
  2. See snackbar message saying that the file type is not supported

PNG image, dropped on editor

  1. Drop PNG image onto editor
  2. See an instant preview of the image on the canvas and in the media library
  3. See a progress bar for the image in the media library while it's being uploaded
  4. Verify that you get a warning trying to close the window while upload is in progress

PNG image, dropped on media library

  1. Drop PNG image onto editor
  2. See an instant preview of the image in the media library
  3. See a progress bar for the image in the media library while it's being uploaded
  4. Verify that you get a warning trying to close the window while upload is in progress

MP4 video, dropped on editor

  1. Drop MP4 video onto editor
  2. See an instant preview of the video on the canvas and in the media library
  3. Verify that you can play the video on the canvas while it's being uploaded
  4. See a progress bar for the video in the media library while it's being uploaded
  5. Verify that you get a warning trying to close the window while upload is in progress

MP4 video, dropped on media library

  1. Drop MP4 video onto editor
  2. See an instant preview of the video in the media library
  3. See a gray, rectangular placeholder element on the canvas
  4. See a progress bar for the video in the media library while it's being uploaded
  5. Verify that you get a warning trying to close the window while upload is in progress

MOV video (H.264), dropped on editor

  1. Drop MOV video onto editor
  2. See an instant preview of the video in the media library
  3. See a progress bar for the video in the media library while it's being uploaded
  4. Verify that you get a warning trying to close the window while upload is in progress

MOV video (H.264), dropped on media library

  1. Drop MOV video onto editor
  2. See an instant preview of the video in the media library
  3. See a progress bar for the video in the media library while it's being uploaded
  4. Verify that you get a warning trying to close the window while upload is in progress

MOV video (HEVC), dropped on editor

  1. Drop MOV video onto editor
  2. See a gray, rectangular placeholder element on the canvas and in the media library
  3. Verify that you cannot play the video on the canvas (no play button)
  4. See a progress bar for the video in the media library while it's being uploaded
  5. Verify that you get a warning trying to close the window while upload is in progress
  6. See how the placeholder gets replaced with an actual preview after some time, and that its dimensions and position on the canvas change.

MOV video (HEVC), dropped on media library

  1. Drop MOV video onto editor
  2. See a gray placeholder element in the media library
  3. See a progress bar for the video in the media library while it's being uploaded
  4. Verify that you get a warning trying to close the window while upload is in progress

UAT

  • UAT should use the same steps as above.

This PR can be tested by following these steps:

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

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 PR contains automated tests (unit, integration, and/or e2e) to verify the code works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #6088

Also partially addresses #6400 by using URL.revokeObjectURL() as needed

Also fixes #6427

@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Group: Media Pod: WP & Infra Group: Video Optimization Media transcoding, compression and cropping labels Feb 26, 2021
@google-cla google-cla bot added the cla: yes label Feb 26, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2021

Size Change: +2.91 kB (0%)

Total Size: 1.22 MB

Filename Size Change
assets/js/edit-story.js 338 kB +1.41 kB (0%)
assets/js/stories-dashboard.js 269 kB +1.51 kB (+1%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 267 B 0 B
assets/css/stories-dashboard.css 284 B 0 B
assets/css/vendors~edit-story.css 702 B 0 B
assets/css/web-stories-embed-block.css 615 B 0 B
assets/js/chunk-fonts-********************.js 45 kB 0 B
assets/js/chunk-web-stories-template-0-********************.js 11 kB +1 B (0%)
assets/js/chunk-web-stories-template-1-********************.js 11.3 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 10.9 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 10.6 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 12.7 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 7.14 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 10.3 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 10.1 kB 0 B
assets/js/chunk-web-stories-textset-0-********************.js 5.23 kB 0 B
assets/js/chunk-web-stories-textset-1-********************.js 6.79 kB 0 B
assets/js/chunk-web-stories-textset-2-********************.js 7.81 kB 0 B
assets/js/chunk-web-stories-textset-3-********************.js 15.3 kB 0 B
assets/js/chunk-web-stories-textset-4-********************.js 4.38 kB 0 B
assets/js/chunk-web-stories-textset-5-********************.js 5.64 kB 0 B
assets/js/chunk-web-stories-textset-6-********************.js 5.43 kB 0 B
assets/js/chunk-web-stories-textset-7-********************.js 10.3 kB 0 B
assets/js/vendors~chunk-ffmpeg-********************.js 5.61 kB -1 B (0%)
assets/js/vendors~edit-story-********************.js 61.3 kB -9 B (0%)
assets/js/vendors~edit-story~stories-dashboard-********************.js 267 kB -6 B (0%)
assets/js/web-stories-activation-notice.js 68.2 kB 0 B
assets/js/web-stories-embed-block.js 19.8 kB 0 B

compressed-size-action

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #6541 (d5e94bd) into main (6029ddc) will increase coverage by 32.97%.
The diff coverage is 64.37%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6541       +/-   ##
===========================================
+ Coverage   49.83%   82.81%   +32.97%     
===========================================
  Files         700     1127      +427     
  Lines       12013    20061     +8048     
===========================================
+ Hits         5987    16613    +10626     
+ Misses       6026     3448     -2578     
Flag Coverage Δ
karmatests 73.46% <21.45%> (+32.78%) ⬆️
unittests 65.73% <64.06%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
assets/src/edit-story/app/media/local/reducer.js 48.48% <0.00%> (+45.25%) ⬆️
...t-story/app/media/local/useContextValueProvider.js 78.04% <ø> (+78.04%) ⬆️
...ets/src/edit-story/app/media/pagination/reducer.js 45.00% <0.00%> (+40.00%) ⬆️
...s/src/edit-story/app/media/utils/createResource.js 100.00% <ø> (+100.00%) ⬆️
...it-story/app/media/utils/getResourceFromMedia3p.js 84.74% <ø> (+84.74%) ⬆️
.../edit-story/app/media/utils/useUploadVideoFrame.js 37.50% <ø> (+37.50%) ⬆️
.../components/library/panes/media/local/mediaPane.js 65.00% <0.00%> (ø)
assets/src/edit-story/elements/video/controls.js 89.41% <ø> (+78.82%) ⬆️
assets/src/edit-story/types.js 100.00% <ø> (ø)
...-story/app/media/utils/getResourceFromLocalFile.js 16.32% <15.00%> (+0.53%) ⬆️
... and 828 more

@swissspidy swissspidy marked this pull request as ready for review February 26, 2021 17:05
@spacedmonkey
Copy link
Contributor

I am seeing this bug more while testing this - #6427

@spacedmonkey
Copy link
Contributor

I think we need to change these lines

https://github.com/google/web-stories-wp/blob/b851f3ca8c24d50ec327b4c5121d338d64ef7cbc/assets/src/edit-story/components/library/panes/media/common/innerElement.js#L107-L112

With this PR, we are adding a state where we know that poster is going to be null. This just makes the averageColor issue worse.

@swissspidy
Copy link
Collaborator Author

I am seeing this bug more while testing this - #6427

I think we need to change these lines

https://github.com/google/web-stories-wp/blob/b851f3ca8c24d50ec327b4c5121d338d64ef7cbc/assets/src/edit-story/components/library/panes/media/common/innerElement.js#L107-L112

With this PR, we are adding a state where we know that poster is going to be null. This just makes the averageColor issue worse.

While I am not seeing the bug at the moment, I've made useAverageColor more robust now in 75f2d75, fixing this once and for all.

@spacedmonkey
Copy link
Contributor

Screenshot 2021-03-02 at 10 54 05

I was unable to upload a 164MB MP4. This should be have been transcoded.

Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

I found a regreation and some repeated code. I also have some other feedback.

Copy link
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

LGTM. Excited on doors this opens in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Media Group: Video Optimization Media transcoding, compression and cropping Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
2 participants