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

feat(video osd): Add OSD for video playback #477

Merged
merged 4 commits into from
Jan 2, 2021
Merged

feat(video osd): Add OSD for video playback #477

merged 4 commits into from
Jan 2, 2021

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Jan 1, 2021

image

All Icons, excluding:

  • Play/Pause
  • Prev/Next track
  • Minimize / Exit

are not currently hooked up to anything

AirPlay and PiP icons are hidden if it is not supported.

Top Left Buttons: Close Player, mini player, PiP mode
Top Right Buttons: SyncPlay, AirPlay, ChromeCast
Bottom Left: Prev Track, Play/Pause, Next Track
Bottom Right: Captions, Settings, Full Screen

@camc314 camc314 marked this pull request as draft January 1, 2021 17:57
@codecov-io
Copy link

codecov-io commented Jan 1, 2021

Codecov Report

Merging #477 (b0bf57a) into master (e07615c) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #477      +/-   ##
=========================================
- Coverage    0.62%   0.61%   -0.02%     
=========================================
  Files          77      78       +1     
  Lines        2073    2120      +47     
  Branches      294     302       +8     
=========================================
  Hits           13      13              
- Misses       2059    2106      +47     
  Partials        1       1              
Impacted Files Coverage Δ
components/Players/PlayerManager.vue 0.00% <0.00%> (ø)
components/Players/VideoPlayer.vue 0.00% <0.00%> (ø)
utils/supportedFeatures.ts 0.00% <0.00%> (ø)

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 e07615c...b0bf57a. Read the comment docs.

@@ -83,12 +83,7 @@ export default Vue.extend({
shaka.polyfill.installAll();
if (shaka.Player.isBrowserSupported()) {
this.player = new shaka.Player(this.$refs.videoPlayer);
// TODO: Remove Shaka's OSD and use our own
Copy link
Member

Choose a reason for hiding this comment

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

@camc314 Shouldn't we start using shaka player instead of shaka-player-ui as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

You just need to change the import to have the non-ui version.

Copy link
Member

Choose a reason for hiding this comment

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

@mrtimscampi I know, but that change is not in this PR, that's why I mention it :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's now using
import shaka from 'shaka-player/dist/shaka-player.compiled';

@camc314 camc314 force-pushed the feat/OSD branch 2 times, most recently from c1b453e to 2905a64 Compare January 2, 2021 08:09
@camc314 camc314 marked this pull request as ready for review January 2, 2021 08:35
Copy link
Contributor

@heyhippari heyhippari left a comment

Choose a reason for hiding this comment

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

LGTM and works well. We can clean up and improve some stuff in further PRs.

@camc314
Copy link
Contributor Author

camc314 commented Jan 2, 2021

Now that #399 is merged, I am going to make the slider work properly

@heyhippari
Copy link
Contributor

Now that #399 is merged, I am going to make the slider work properly

Sounds good. Do we do this in another PR or in this one?

I also pushed a small fix for an issue when stopping videos (Some of the references in the OSD became undefined. Moving the v-if so the OSD gets properly removed as well fixes it)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 2, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@camc314
Copy link
Contributor Author

camc314 commented Jan 2, 2021

Now that #399 is merged, I am going to make the slider work properly

Sounds good. Do we do this in another PR or in this one?

I've done it in this one. Should fixed and working now

I also pushed a small fix for an issue when stopping videos (Some of the references in the OSD became undefined. Moving the v-if so the OSD gets properly removed as well fixes it)

Ah thanks, I missed this, I think I was in the middle of rebasing, I've added the change

Comment on lines +109 to +135
<v-slider
min="0"
:max="runtime"
validate-on-blur
:step="0"
:value="sliderValue"
hide-details
thumb-label
@end="onPositionChange"
@change="onPositionChange"
@mousedown="onClick"
@mouseup="onClick"
@input="onInputChange"
>
<template #prepend>
<span class="mt-1">
{{ getRuntime(realPosition) }}
</span>
</template>
<template #thumb-label>
{{ getRuntime(sliderValue) }}
</template>
<template #append>
<span class="mt-1">
{{ getRuntime(runtime) }}
</span>
</template>
Copy link
Member

Choose a reason for hiding this comment

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

@camc314 Why we don't make a custom component for this? Seems useless to repeat the same logic that we use in Audio controls here.

We can name it PlaybackProgressSlider.

We will also need this component for the future Full screen page for music playback, so it makes total sense imo.

Copy link
Member

@ferferga ferferga Jan 2, 2021

Choose a reason for hiding this comment

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

@camc314 Use the one in Audio Controls as a template, there are some changes there that haven't been backported here (like the removal of the min prop).

Copy link
Contributor

Choose a reason for hiding this comment

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

@ferferga We should probably do this in another PR, since it'll touch other components as well. I'll merge this, then we can work on unifying the common components of the various players.

Copy link
Member

Choose a reason for hiding this comment

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

@MrTimscampi okay, will do it in the full screen PR then.

@heyhippari heyhippari merged commit 9d773f2 into master Jan 2, 2021
@heyhippari heyhippari deleted the feat/OSD branch January 2, 2021 18:16
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.

4 participants