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

Integrated the new player in the asset and stream pages #1330

Merged
merged 27 commits into from
Oct 19, 2022

Conversation

clacladev
Copy link
Contributor

@clacladev clacladev commented Oct 6, 2022

⚠️ THIS PR CAN BE MERGED ONLY AFTER #1326 GETS MERGED

What does this pull request do? Explain your changes. (required)
Integrated the new player in the asset and stream pages

Specific updates (required)

  • Updated livepeer/react dependency to the latest
  • Update the AppPlayer component to standardise the look and feel (mainly the controls color)
  • Small refactored on the player box in the stream page

How did you test each of these updates (required)

  • Tested on light and dark mode
  • Tested on chrome and safari

Does this pull request close any open issues?
Fixes #1320

Screenshots (optional):
Screenshot 2022-10-06 at 16 47 58
Screenshot 2022-10-06 at 16 48 08
Screenshot 2022-10-06 at 16 48 30
Screenshot 2022-10-06 at 16 48 18

@vercel
Copy link

vercel bot commented Oct 6, 2022

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

Name Status Preview Updated
livepeer-studio ✅ Ready (Inspect) Visit Preview Oct 18, 2022 at 10:01PM (UTC)

@clacladev clacladev marked this pull request as ready for review October 6, 2022 14:24
@clacladev clacladev requested a review from a team as a code owner October 6, 2022 14:24
@clacladev clacladev changed the base branch from clacladev/progress-asset to master October 6, 2022 14:24
@victorges
Copy link
Member

victorges commented Oct 6, 2022

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.
  • 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.

@victorges
Copy link
Member

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 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.

@clacladev
Copy link
Contributor Author

Hey Victor, this PR is build on top of another one #1326 which is waiting to be reviewed. The other one is the one where the progress has been implemented. So I will address the progress feedback there.

Regarding the player aspect ratio
This is more of a product decision. I am not sure what is the best decision. Keeping the current 16:9 means that the layout of the page is always optimal, but the playback space is not.
If we change it to fit the content, it will maximise the playback space, but it will make the layout a bit more wonky when portrait.
I don't think that the Dashboard is the place where there is the need to optimise for viewing experience. It's not the place where a final user watches a video, but it's a tool for a dev to preview a video and to debug when the things don't work. Just my opinion.

@victorges
Copy link
Member

victorges commented Oct 6, 2022

@clacladev I'm suggesting changing the objectFit property, not the aspectRatio. I agree that it looks better in the layout for the player to always have the same size, but I completely disagree that it is better to have the video cropped and only displaying a small portion of it (the default cover behavior) instead of resizing the video inside the player space and adding black bars on the side (contains).

I don't think that the Dashboard is the place where there is the need to optimise for viewing experience. It's not the place where a final user watches a video, but it's a tool for a dev to preview a video and to debug when the things don't work. Just my opinion.

I believe the Dashboard main focus is for evaluating the platform and inspecting the correct functioning of the services we provide. If I upload a portrait video, then I go to the details page and it shows me only a small portion of it, I'm just going to assume that the processing is broken on the Livepeer side. So I believe it is important for the player to show the entire content of the video, even if it has to scale it a bit down to fit on its aspectRatio and add the black bars.

IMHO this is gonna be the case for the vast majority of applications, which is why I've also opened that discussion to change the default value to be contains. I have never used a player that defaults to modifying the source video instead of doing its best to display all of the video content. This is a bigger discussion tho, but focusing on the dashboard I am even more certain of that. If I'm evaluating the Livepeer platform and I upload a video, I wanna see how that video looks in its entirety, not in an arbitrary portion of it that we are deciding to show.

@pglowacky
Copy link
Contributor

My $0.02 is that we can't crop the video in the dashboard or risk users being confused about why their full video isn't being shown. Ideally, the player dynamically adjusts to the input video aspect ratio so that users can see their video without big black bars (that make it difficult to see the content) or cropping the video.

I vote that the player adapts to the input video as the default (which is shown on the dashboard) and then users can customize the player's size however they'd like in their application.

@victorges
Copy link
Member

victorges commented Oct 6, 2022

Ideally, the player dynamically adjusts to the input video aspect ratio so that users can see their video without big black bars (that make it difficult to see the content) or cropping the video.

While I'd be OK if we did that, I agree that it's not ideal to have the layout wobble around after the video starts playing. So I think the only acceptable implementation here would be if we used the asset.videoSpec to load the player with the nearest aspectRatio regarding the asset resolution. Then it doesn't wobble, loading with a fixed size instead, just a closer (maybe exact) aspect ratio to the video.

The layout would look different for different assets tho, depending on their aspect ratio. There might be an argument that we want that to be consistent as well so the UI is not a "surprise" every time you open the page. @clacladev and @0xcadams would know better about the best practices here.

I believe this is lower priority than to stop cropping the source video though. We could do that on a follow up PR or keep on the backlog as a nice to have, while I don't think we can ship a dashboard that looks like we are cropping the uploaded videos.

@clacladev
Copy link
Contributor Author

Sorry @victorges I had misunderstood what you were trying to tell me. And yes, I agree with you in changing the objectFit="cover" to objectFit="contain". I think it makes a better UX overall. I agree that as a user I should always see the entirety of the video.

I already implemented it and committed it.

Here before a portrait video:
Screenshot 2022-10-07 at 12 26 01

Here after a portrait video:
Screenshot 2022-10-07 at 12 26 20

Here for a cinematic sized video:
Screenshot 2022-10-07 at 12 27 51

I think it's an overall improvement.

@victorges
Copy link
Member

victorges commented Oct 7, 2022 via email

Copy link
Member

@adamsoffer adamsoffer left a comment

Choose a reason for hiding this comment

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

LGTM! @clacladev let's just address those two merge conflicts then good to squash and merge.

@clacladev clacladev merged commit a2b3193 into master Oct 19, 2022
@clacladev clacladev deleted the clacladev/update-player branch October 19, 2022 15:23
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.

Update @livepeer/react dependency to v1.0.4 for new player updates
5 participants