Closed GrainyTV closed 3 months ago
Nice catches! I will investigate and roll the fixes ASAP
I tried out the new 2.3.0 Windows version of the executable, and it looks like all the issues I reported have been resolved. I'm going to go ahead and close this ticket now.
Bug Description
I wanted to check out this browser extension but I came across multiple issues with using it. I did a bit of an investigation on my own and collected what I could for 3 separate bugs. Each of them is detailed below, along with the steps I've taken to resolve them. This was done on Windows platform.
How to Reproduce
Issue 1
I found this one accidentally. I am not a 100% sure if it is a bug, but basically if mpv is installed through the chocolatey package manager, then two separate mpv applications will be visible on the path, namely:
So if we just specify mpv without file extension in the config.yml file under the executable property, it will implicitly call the wrong one with .com and open a terminal window unnecessarily before invoking the actual media.
Issue 2
Now the next one is 100% a valid bug. Basically the built-in button for starting a video in fullscreen would not start the video in fullscreen unless I specified it under the flag_overrides property.
Issue 3
The third issue is related to the picture in picture mode. I copied the default values for my pip property but it failed to open in mpv. From the mpv logs I could see that the whole pip property is passed to mpv as a single value which ruins the command.
I did not run into this problem with the fullscreen option because it only has one parameter as '--fs'.
Expected Behavior
Fix 1
The documentation should emphasize to use the .exe file extension for the executable property under Windows to call the appropriate mpv application.
Fix 2
This one is caused by a small ambiguity in the codebase. Some parts of the repository like the example and default configuration file show to use a property called fullscreen in one word. While the protocol section shows that it should be full_screen. Now this would only be an issue with the docs but it appears in the source code as well.
common.js - Line 82
params.push("full_screen=1");
options.go - Line 98
o.Fullscreen = u.Query().Get("fullscreen") == "1"
Basically a parameter is added, that is not read and lost during the parsing process.
Fix 3
For this one, the different arguments for pip needed to be separated so it does not think that the whole line is a command line flag. I managed to make it work by splitting the command by whitespaces.
options.go - Line 162 [Original]
ret = append(ret, playerConfig.Pip)
options.go - Line 162 [New]
ret = append(ret, strings.Fields(playerConfig.Pip)...)
Not sure how valid this solution is, but temporarily it fixed the issue from happening for me because each option were passed individually.
Version
latest release v2.2.2
Configuration
Relevant log output (optional)
What browsers are you seeing the problem on? (optional)
Chromium-based (Brave, Chrome, etc.)
Checklist: