Closed drewbitt closed 1 year ago
To reproduce the crash:
Remove all files from the directory IINA is configured to store screenshots in (default "~/Pictures/Screenshots"). Be certain there aren't any files in the directory the finder hides by default such as .DS_Store
:
low-batt@gag Screenshots$ ls -la
total 16
drwx------ 3 low-batt staff 96 Jul 31 20:26 .
drwx------+ 6 low-batt staff 192 Jul 31 19:48 ..
-rw-r--r--@ 1 low-batt staff 6148 Jul 31 20:26 .DS_Store
low-batt@gag Screenshots$
This directory must be completely empty to reproduce the crash.
An error message will be emitted to the mpv log:
[ 13.600][e][cplayer] Error opening '/Users/low-batt/Pictures/Screenshots/0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789-0001.png' for writing!
[ 13.600][e][cplayer] Error writing screenshot!
IINA will then crash in Utility.getLatestScreenshot
:
The crash occurs on this line of Utility.getLatestScreenshot
:
var latestFile: URL = contents[0]
Since mpv was unable to save the screenshot the screenshot directory is still empty and therefore the contents
array is empty and indexing into it triggers a crash.
Of course IINA should have noticed mpv failed to create the screenshot and should not be calling this method. IINA is submitting a command to mpv to take the screenshot asynchronously. The results of the command are returned as a mpv MPV_EVENT_COMMAND_REPLY
event and processed in MPVController.handleEvent
. The code in handleEvent
does not check the event to see if mpv is reporting the command failed.
The method MPVController.handleEvent
needs to be changed to check if the event reports an error and if so, raise an alert. Unfortunately the mpv API does not provide any details on the failure. The error code returned maps to "error running command". The actual cause of the failure can only be found in the log files. All the alert can report is that IINA can not take a screenshot:
Even though the method Utility.getLatestScreenshot
will no longer be called in this case it should be changed to guard against the directory being empty.
I will shortly post a pull request with these changes. This commit adds new text for the alert that will require localization.
Note that drewbitt encountered this failure when streaming. If it is common for streams to have filenames so long as to trigger this failure then IINA may want to consider how this situation can be improved. Although there is a workaround described below, many users may not think to investigate configuring mpv options as a solution to this failure.
As lizyn showed in issue #3083, the mpv option screenshot-template
can be used to configure the filename generated by mpv for screenshots. To workaround this failure set this option in IINA advanced preferences to something like "shot%n", a template that will does not include the long filename of the video:
Remember to restart IINA after setting this option.
The syntax of a mpv screenshot template is documented here: mpv manual screenshot-template option
Perhaps we should also report this upstream to mpv?
mpv works fine in this case because by default the title is not used in the name of the screen shot: "mpv-shot0001.jpg"
IINA has a screenshotTemplate
preference which I don't think is exposed in the UI? The default for that preference is "%F-%n". So IINA is telling mpv to use the long filename in the filename of the screenshot. The mpv project may view this as you got what you asked for. The use of the generic "command failed" code by mpv in the error reporting is rather disappointing.
Yeah, basically that, although it's not a priority to report I guess since we can handle it in IINA without too much trouble.
IINA 1.3.2 contains the fix for this issue.
This is a bug in mpv in crashing with long video titles since it saves files by default containing the full filename. macOS filename limit is 255 characters. But, streams can often have far more than 255 character filenames. Iina will then crash with no error display. This should be handled better, likely with an error display that it can't save a file. You could attempt to truncate this to 255 characters as well but that approach doesn't seem ideal.
and a crash
Expected Behavior: Tells you if it can't screenshot and doesn't simply crash with no error