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

Fix crash when single dash ("-") arg is supplied to IINA via cmd line #4032

Merged
merged 1 commit into from
Nov 6, 2022

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Nov 3, 2022


Description:
Adds a simple check to avoid a fatal error at startup, if a single dash (-) is included in the args.

It was easy to confirm the crash, and also confirm the fix, by just supplying an arg in XCode's Scheme Editor:
Dash

@low-batt
Copy link
Contributor

low-batt commented Nov 3, 2022

Thanks for taking this one!

Pulled PR locally and verified that piping to iina-cli - no longer crashes and plays the piped media. Yeah!

However the code is still a bit messed up as can be seen by these log messages:

low-batt@gag MacOS$ cat ~/Movies/resume-failure.mp4 | ./iina-cli -
…
14:09:57.567 [iina][d] Got arguments ["--stdin", "-"]
14:09:57.567 [iina][d] IINA arguments: ["--stdin"]
14:09:57.567 [iina][d] Filenames from arguments: []

So iina-cli is totally ignoring "-", detecting that standard input can be opened and has bytes available and then is adding the option "--stdin". That means it specifies standard input twice when calling IINA. That seems wrong to me.

Then IINA instead of translating "-" into "--stdin" or "/dev/stdin" just ignores the "-"? Because of what iina-cli did, that is helpful in this case. But also seems wrong.

Thoughts?

@low-batt low-batt linked an issue Nov 3, 2022 that may be closed by this pull request
1 task
@svobs
Copy link
Contributor Author

svobs commented Nov 4, 2022

Then IINA instead of translating "-" into "--stdin" or "/dev/stdin" just ignores the "-"? Because of what iina-cli did, that is helpful in this case. But also seems wrong.

I think there are two separate points here.

  1. It sounds like you are expecting a lone - to be interpreted as equivalent to --stdin, but I don't see anything documenting that behavior. Sometimes apps will use -- in this way, but IINA seems to be honoring this correctly:
msvoboda|17:25:59|MacOS$ cat ~/LocalHome/Downloads/bbb_sunflower_2160p_60fps_normal.mp4 | ./iina-cli --
...
17:44:20.057 [iina][d] Got arguments ["--stdin"]
17:44:20.057 [iina][d] IINA arguments: ["--stdin"]
17:44:20.057 [iina][d] Filenames from arguments: []
  1. It looks like iina-cli ignores all tokens it doesn't recognize. While not strictly wrong, it certainly goes against standard practice for command line tools, which preaches the need for "fail fast" behavior. But it's not treating - in any kind of special way by ignoring it. Though I too have issue filing fatigue, this should probably be logged as a separate issue as it's out of the scope of this defect.

@low-batt
Copy link
Contributor

low-batt commented Nov 4, 2022

I believe in unix it is up to commands as to how they want to interpret "-", so yes, it does not always mean read from stdin.

For IINA we always want to match up with mpv unless we have a good reason to deviate. From the mpv manual, section usage:

One exception is the lone - (without anything else), which means media data will be read from stdin. Also, -- (without anything else) will make the player interpret all following arguments as filenames, even if they start with -. (To play a file named -, you need to use ./-.)

I don't see a reason for IINA to not match up with mpv in this case.

I didn't request changes as the PR fixes a crash and supports reading from stdin like the user requested. Just wanted to hear your thoughts on this aspect of the behavior. We can definitely treat this as a separate issue. There are definitely other issues in regard to CLI behavior.

…, and add checks to deduplicate occurrences of it
@svobs
Copy link
Contributor Author

svobs commented Nov 4, 2022

OK, I dug deeper into it, and you are right.

I pushed an updated version which should handle the single dash correctly, and also should handle --mpv--.

Seems like it would be easier & less error prone to just use a stricter form of parsing which requires that all non-file arguments are in the form "--key=value", and anything that doesn't start with dashes is an input file. And especially since mpv has indicates that's where they are trying to go in the future. But this isn't really a feature I use anyway.

Copy link
Contributor

@low-batt low-batt left a comment

Choose a reason for hiding this comment

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

Pulled PR, built and confirmed running IINA like this works now and does not crash:

low-batt@gag MacOS$ cat ~/Movies/resume-failure.mp4 | ./iina-cli -

@uiryuu uiryuu merged commit 21510e3 into iina:develop Nov 6, 2022
@uiryuu
Copy link
Member

uiryuu commented Nov 6, 2022

Thanks!

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.

Crash when passed - in parameter list
3 participants