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

Showing progress in the Assets list and details page #1326

Merged
merged 23 commits into from
Oct 18, 2022

Conversation

clacladev
Copy link
Contributor

@clacladev clacladev commented Oct 4, 2022

What does this pull request do? Explain your changes. (required)
Add the file upload progress and processing progress to the Assets list and Asset details page.

Specific updates (required)

  • Added File Upload progress and Processing progress to the Assets list page
  • Added File Upload progress and Processing progress to the Assets details page (player position)
  • Refactored some files to break them up more and make them more manageable

How did you test each of these updates (required)
I extensively tested the feature with the different themes.

Does this pull request close any open issues?

Fixes #1297

Screenshots (optional):
Screenshot 2022-10-05 at 14 07 40
Screenshot 2022-10-05 at 14 07 10
Screenshot 2022-10-05 at 14 17 27
Screenshot 2022-10-05 at 14 17 40
Screenshot 2022-10-05 at 14 19 34

@vercel
Copy link

vercel bot commented Oct 4, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeer-studio ✅ Ready (Inspect) Visit Preview Oct 7, 2022 at 9:39AM (UTC)

@clacladev clacladev marked this pull request as ready for review October 5, 2022 12:12
@clacladev clacladev requested a review from a team as a code owner October 5, 2022 12:12
@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Merging #1326 (9c2e16c) into master (7769111) will decrease coverage by 0.08968%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #1326         +/-   ##
===================================================
- Coverage   51.95067%   51.86099%   -0.08969%     
===================================================
  Files             68          68                 
  Lines           4460        4460                 
  Branches         833         833                 
===================================================
- Hits            2317        2313          -4     
- Misses          1848        1851          +3     
- Partials         295         296          +1     
Impacted Files Coverage Δ
packages/api/src/webhooks/cannon.ts 47.36842% <0.00000%> (-2.33918%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7769111...9c2e16c. Read the comment docs.

Impacted Files Coverage Δ
packages/api/src/webhooks/cannon.ts 47.36842% <0.00000%> (-2.33918%) ⬇️

@clacladev
Copy link
Contributor Author

Addressing @victorges review, published erroneously on this other PR #1330 (confusion issue is my fault).

Tested the dashboard around and looks great! Some notes/questions:

The player is configured with the default objectFit, which means that portrait videos won't be rendered properly in the asset preview page. I tested with [this jelly fish video](https://livepeer-studio-git-clacladev-update-player-livepeer.vercel.app/dashboard/assets/f85d588d-9145-49e6-ba47-97c1264d0c7c).
There is a https://github.com/livepeer/livepeer.js/discussions/87, but while we don't change the default, I think in the dashboard we should set the objectFit property of the player to contain so the video is not cropped.
I didn't seem to get the processing progress show up in the asset list a couple of times. It stayed stuck in Uploading 100% and then just switched to ready for some reason. I'm thinking it won't change to Processing X% until some non-zero progress shows up. On that case, I think it's better to show Progressing 0%... than Uploading 100%, so IMO we should switch the notice to "processing" as soon as the asset.status.phase becomes processing. Also, we could show only Processing... when we have a zero percentage, wdyt?
Everything else looks great! I like that the player volume seems to be a global on the dashboard as well.

Oh and a crazy/stretch idea. Since we don't have very granular progress on the processing phase, WDYT of applying some smoothing function to the progress that we read from the API?

Something like... we were showing 0% then we get a 25% from the API. Instead of just changing immediately, we could smooth a transition to that value, maybe as an asymptote or only smoothing it over 10 seconds, which is [the minimum progress reporting interval](https://github.com/livepeer/task-runner/blob/4d1ae1768c01e979969bf7f00b81267db05e299c/task/progress.go#L16) on the backend.

There might be some ready function or lib for that, or maybe we could just implement some logic like "every half a second move the displayed progress 10% closer to the progress reported by the API". Like this, we'd smooth out to ~90% of the API progress in a 10s period, ~99% over 20s, and it might feel nice on the dashboard.

As I said this is kind of a crazy idea tho. Just a stretch, but it'd probably be fun to implement as well if you wanna give it a shot.

Regarding the current UX of moving between Uploading to Processing
As soon as the Upload is completed, I invalidate the asset data, so the asset is fetched again. But it happens that the status of the asset is not yet in "processing". That's why the asset is still "uploading". It would happen after a second or two I imagine. Maybe, I could put a delay of 1-2s before invalidating the data.

The Assets list page has also a refresh interval of 15s. That's why it is jumping from Uploading 100% to Processing 100%. A small video is processed in less than 15s.

FYI, if you start to upload a video and, while it has not finished yet, you tap on it you will see it opening inthe Asset details page still uploading. This one will refresh the asset details every 5s. So you will see a smoother process "uploading -> processing"

I don't know why the refresh intervals are different, but I supposed that's because the two calls are not expensive the same.

Regarding the progress smoothing
I don't think we should do the smoothing. It feels a premature optimisation at this time, I would focus on things that move the needle for the product. I also don't think that many people would even notice.

@victorges
Copy link
Member

@clacladev All makes sense! Yeah I don't think the progress smoothing is a priority, only if it was easy enough to implement for a cleaner UI.

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM

…Progress.tsx


Better managing the UI text when progress percentage is zero

Co-authored-by: Victor Elias <victorgelias@gmail.com>
@adamsoffer adamsoffer merged commit 51a6b5f into master Oct 18, 2022
@adamsoffer adamsoffer deleted the clacladev/progress-asset branch October 18, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add progress bar and status to asset list and detail views for video upload processing
4 participants