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

Update external-player-map.json: Add basic support for external SMPlayer #3771

Merged
merged 5 commits into from
Jul 19, 2023
Merged

Conversation

trostboot
Copy link
Contributor

Add basic functionality to open videos in SMPlayer

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #2231

Description

Adds basic functionality to open videos in SMPlayer.

Adding more parameters (e.g., startOffset or speed) is not possible at this time as SMPlayer expects different formatting compared to what FreeTube currently passes through.

Screenshots

Testing

Testing has been done on Windows 11 x64 only.

Desktop

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

Additional context

Adds basic functionality to open videos in SMPlayer.

Adding more parameters (e.g., startOffset or speed) is not possible at this time as SMPlayer expects different formatting compared to what FreeTube currently passes through.
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 13, 2023 18:51
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 13, 2023
@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jul 17, 2023

How do you normally set the start offset from the cli with SMPlayer for YouTube videos?

@trostboot
Copy link
Contributor Author

trostboot commented Jul 17, 2023

By passing the -start switch with the desired offset, e.g.
smplayer -start 15 %url%
Accepts either seconds or hh:mm:ss timecodes.
Alternatively it also parses standard Youtube timecodes in the URL.

Setting startOffset to "-start " does not work as it's parsed as a separate input URL.

[10:17:47:583] SMPlayer::processArgs: arguments: 3
[10:17:47:583] SMPlayer::processArgs: 0 = C:\Program Files\SMPlayer\SMplayer.exe
[10:17:47:583] SMPlayer::processArgs: 1 = -start 25
[10:17:47:583] SMPlayer::processArgs: 2 = https://www.youtube.com/ [...]
[10:17:47:583] SMPlayer::processArgs: files_to_play: count: 2
[10:17:47:583] SMPlayer::processArgs: files_to_play[0]: '-start 25'
[10:17:47:583] SMPlayer::processArgs: files_to_play[1]: 'https://www.youtube.com/ [...]'

vs invoking it from a terminal:

[10:32:54:948] SMPlayer::processArgs: arguments: 4
[10:32:54:948] SMPlayer::processArgs: 0 = C:\Program Files\SMPlayer\smplayer.exe
[10:32:54:948] SMPlayer::processArgs: 1 = -start
[10:32:54:948] SMPlayer::processArgs: 2 = 25
[10:32:54:948] SMPlayer::processArgs: 3 = https://www.youtube.com/ [...]
[10:32:54:948] SMPlayer::processArgs: initial_second: 25
[10:32:54:948] SMPlayer::processArgs: files_to_play: count: 1
[10:32:54:948] SMPlayer::processArgs: files_to_play[0]: ' https://www.youtube.com/ [...]'

Other options differ more substantially as they would have to be passed via the -actions switch, e.g.
smplayer -actions "set speed 2" %url%
for 2x playback speed. However, in my testing, passing options via -actions was not very reliable. Would have to further investigate potential causes.

@ChunkyProgrammer
Copy link
Member

This patch seems to be getting the start offset to work for me

diff --git a/src/renderer/store/modules/utils.js b/src/renderer/store/modules/utils.js
index 1f4edda9..454c5936 100644
--- a/src/renderer/store/modules/utils.js
+++ b/src/renderer/store/modules/utils.js
@@ -523,7 +523,12 @@ const actions = {

     if (payload.watchProgress > 0 && payload.watchProgress < payload.videoLength - 10) {
       if (typeof cmdArgs.startOffset === 'string') {
-        args.push(`${cmdArgs.startOffset}${payload.watchProgress}`)
+        if (cmdArgs.startOffset.endsWith('=')) {
+          args.push(`${cmdArgs.startOffset}${payload.watchProgress}`)
+        } else {
+          // special handling for SMPlayer and similar
+          args.push(cmdArgs.startOffset, payload.watchProgress)
+        }
       } else if (!ignoreWarnings) {
         showExternalPlayerUnsupportedActionToast(externalPlayer, 'starting video at offset')
       }

diff --git a/static/external-player-map.json b/static/external-player-map.json
index f77ae13c..0885eb40 100644
--- a/static/external-player-map.json
+++ b/static/external-player-map.json
@@ -69,7 +69,7 @@
             "supportsYtdlProtocol": true,
             "videoUrl": "",
             "playlistUrl": "",
-            "startOffset": null,
+            "startOffset": "-start",
             "playbackRate": null,
             "playlistIndex": null,
             "playlistReverse": null,

auto-merge was automatically disabled July 17, 2023 17:42

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 17, 2023 17:43
auto-merge was automatically disabled July 17, 2023 17:45

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 17, 2023 17:45
@trostboot
Copy link
Contributor Author

I can confirm that works. I've added it to this PR, hope you don't mind.

If watchProgress contains a non-integer value, SMPlayer will simply parse it as 0. Thus, truncate it before passing it along.
auto-merge was automatically disabled July 17, 2023 18:24

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 17, 2023 18:24
@ChunkyProgrammer
Copy link
Member

I don't mind at all 😊

src/renderer/store/modules/utils.js Outdated Show resolved Hide resolved
Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
auto-merge was automatically disabled July 18, 2023 05:10

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 18, 2023 05:10
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.

Math.trunc might only be applicable for SMPlayer
But I have no better alternative yet

@trostboot
Copy link
Contributor Author

@PikachuEXE Well, the question that needs to be asked is if there is any value in passing along fractions of a second to begin with.
For one, links within Youtube itself only have a precision of whole seconds.
For another, Freetube itself is currently inconsistent in how it handles this (which is why I initially didn't catch the issue with SMPlayer).

If you open a link to an external player from within the Freetube video player with some amount of progress in a video, it will only pass on integer seconds, so I would assume there is some truncating already going on somewhere.
If you then go back to e.g. your subscription page and open that same video with some saved progress in an external player, Freetube will pass along non-integer seconds.

If indeed the behaviour of the player page is the desired behaviour, then my fix is just a band-aid and this should probably be handled further up the chain.
Either way, the behaviour should probably be consistent across different sections of the app.

@PikachuEXE
Copy link
Collaborator

I have no idea what payload.watchProgress can contain
If it contains non integer numbers and it breaks stuff on external players
I wonder why we don't use it in all conditions... (coz existing external players ignore decimal places?)

But I am fine with a band-aid for one player, we can fix it more properly when more external players added

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

Thanks @trostboot for contributing!

If u are interested there are more issues open to support external players so u might want to look into that.

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]: SMPlayer external player support
6 participants