Skip to content

Add basic support for PotPlayer, MPC-HC, MPC-BE #3798

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

Merged
merged 5 commits into from
Jul 29, 2023

Conversation

trostboot
Copy link
Contributor

Add basic support for PotPlayer, MPC-HC, MPC-BE

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #3614

Description

This PR adds basic external player functionality for PotPlayer, MPC-HC and MPC-BE.
It also sorts the list alphabetically, because UX.

Note that MPC-HC requires yt-dlp or similar to be present.
It will also fail to properly play back videos with a startOffset if the resolution is > 1080p (audio will play, video will be black). Unfortunately the default limit (View > Options > Advanced > YDLMaxHeight) MPC-HC ships with is 1440p.
Feedback desired on whether startOffset should stay enabled given the current state.

Note that MPC-BE requires yt-dlp or similar to be present and enabled in order to parse playlists.

Note that both MPC-HC and MPC-BE require #3797 to fix playback from within playlists.

Screenshots

Testing

Desktop

  • OS: Windows
  • OS Version: 11
  • FreeTube version: v0.18.0-nightly-3148

Additional context

Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

e9fae08
Moves entries around

Also the indent alignment is inconsistent with other entries

auto-merge was automatically disabled July 22, 2023 06:33

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 22, 2023 06:33
@trostboot
Copy link
Contributor Author

Thanks, fixed the indents.

As for the reordering, that was intentional so the list would be alphabetical. It just makes more sense to me. Do you have any objections to that?

@PikachuEXE
Copy link
Collaborator

It's harder to review the diff

If we want the display order to be updated it should be done in code not in data (with limited no. of entries)

auto-merge was automatically disabled July 22, 2023 07:30

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 22, 2023 07:30
@trostboot
Copy link
Contributor Author

Well, I've had a look around the relevant code, but I'm afraid that's above my pay grade. Until someone else feels the need to tackle that, here's the unsorted list back :)

@trostboot trostboot requested a review from PikachuEXE July 22, 2023 07:32
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Code looks fine but I will wait for #3797 to be merged first

@FreeTubeBot FreeTubeBot merged commit 01930e4 into FreeTubeApp:development Jul 29, 2023
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 29, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc

Thanks for your contribution @trostboot. If u want to address more external player issues u could take a look at #1975

@trostboot trostboot deleted the ext-mpchc-mpcbe branch July 29, 2023 11:50
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.

[Feature Request]: Open videos in MPC-BE, MPC-HC, or PotPlayer
6 participants