pystardust / ani-cli

A cli tool to browse and play anime
GNU General Public License v3.0
7.92k stars 544 forks source link

Compatibility with iina? #254

Closed tennyson-mccalla closed 2 years ago

tennyson-mccalla commented 2 years ago

This is basically a rewrite of my Discussion post from 3 weeks ago:

I've tried replacing the mpv in player_fn="mpv" with iina to no success. Though I've only tried one episode of one show it succeeds in mpv and fails in IINA. Can anyone help diagnose?

Screen Shot 2021-12-11 at 12 23 40
RaynardGerraldo commented 2 years ago

This is basically a rewrite of my Discussion post from 3 weeks ago:

I've tried replacing the mpv in player_fn="mpv" with iina to no success. Though I've only tried one episode of one show it succeeds in mpv and fails in IINA. Can anyone help diagnose?

Screen Shot 2021-12-11 at 12 23 40

Maybe try adding a new case for IINA ,https://github.com/pystardust/ani-cli/blob/ed1c1ac2d955ca578533a15d3dccba9e8cb2d490/ani-cli#L333. We can also make a wildcard matching for video players but idk if thats needed right now

tennyson-mccalla commented 2 years ago

I'm running an older version of ani-cli (I haven't updated in about a month) so I'll see what happens if I add a case to the new code. I suspect that nothing will change though as the problem seems to be with iina.

RaynardGerraldo commented 2 years ago

I'm running an older version of ani-cli (I haven't updated in about a month) so I'll see what happens if I add a case to the new code. I suspect that nothing will change though as the problem seems to be with iina.

Oh yea definitely update and try adding the case,any version older than this week doesnt work anymore

tennyson-mccalla commented 2 years ago

I ended up changing the www1 to www2 to get the search to at least work again. I'll try to get current with all of the other changes today.

tennyson-mccalla commented 2 years ago

Ok, as predicted, it still didn't work. Changed player_fn to iina and added a case for it in the switch statement (case statement?). When I wrote my discussion post here for ani-cli a few weeks ago I also wrote a similar one over at the iina repo. That place feels almost dead with 1200 issues, 64 PR, and high priority bugs unresolved after 5 months.

port19x commented 2 years ago

May I ask what makes iina so desireable in that case? It's not like supporting another player would be a huge deal, but we have to be careful where we draw the line, otherwise everyone and their mom wants support for garbage video players

tennyson-mccalla commented 2 years ago

I'll link the website so it can pitch itself: https://iina.io

Basically it's just a better player for MacOS than MPV or pretty much anything else.

RaynardGerraldo commented 2 years ago

Ok, as predicted, it still didn't work. Changed player_fn to iina and added a case for it in the switch statement (case statement?). When I wrote my. discussion post here for ani-cli a few weeks ago I also wrote a similar one over at the iina repo. That place feels almost dead with 1200 issues, 64 PR, and high priority bugs unresolved after 5 months.

Damn, try and remove the output redirection to dev null and see if you can get the error output maybe paste it here

tennyson-mccalla commented 2 years ago

Ok, as predicted, it still didn't work. Changed player_fn to iina and added a case for it in the switch statement (case statement?). When I wrote my. discussion post here for ani-cli a few weeks ago I also wrote a similar one over at the iina repo. That place feels almost dead with 1200 issues, 64 PR, and high priority bugs unresolved after 5 months.

Damn, try and remove the output redirection to dev null and see if you can get the error output maybe paste it here

Will do.

Oh snap, it actually worked! It turns out that redirecting stdout and stderr to /dev/null was what was killing it. I'm going to just try and edit it so that it only puts stderr to /dev/null.

port19x commented 2 years ago

Yeah, definitely seems worth supporting

RaynardGerraldo commented 2 years ago

Ok, as predicted, it still didn't work. Changed player_fn to iina and added a case for it in the switch statement (case statement?). When I wrote my. discussion post here for ani-cli a few weeks ago I also wrote a similar one over at the iina repo. That place feels almost dead with 1200 issues, 64 PR, and high priority bugs unresolved after 5 months.

Damn, try and remove the output redirection to dev null and see if you can get the error output maybe paste it here

Will do.

Oh snap, it actually worked! It turns out that redirecting stdout and stderr to /dev/null was what was killing it. I'm going to just try and edit it so that it only puts stderr to /dev/null.

Nice nice

tennyson-mccalla commented 2 years ago

Alright let me document what I did so I don't leave anyone in the audience hanging.

After changing line 7's player_fn="mpv" to player_fn="iina" I added iina to the case statement with:

"iina")
    if echo "$status_code" | grep -vE "^2.*"; 
        printf "${c_red}\nCannot reach servers!"
    rm "${logfile}.new"
    else
        nohup $player_fn "$play_link" > /dev/null 2>&1 &
    printf "${c_green}\nVideo playing"
    mv "${logfile}.new" "$logfile"
    fi;;

After this I simply commented out the combined redirection above and it finally worked!

nohup $player_fn "$play_link" #> /dev/null 2>&1 &

Trying to change the redirect to nohup $player_fn "$play_link" 2> /dev/null & doesn't seem to work. The return from nohup that I'm getting now says, "nohup: ignoring input and appending output to 'nohup.out'"

I'll keep tinkering with this to see what works.

EDIT: got it to work with that last redirect I wrote so long as I remove the ampersand. Will it work with the combined redirect if I remove the ampersand?

EDIT2: Ok, the issue was the ampersand. The original form works perfectly well so long as I don't have the ampersand there.

And just for completion this is my system: Late 2021 MacBook Pro 16" M1 Max macOS Monterey 12.1 ani-cli version available from git clone today (2022-01-01). iina version available from brew today.

RaynardGerraldo commented 2 years ago

Alright let me document what I did so I don't leave anyone in the audience hanging.

After changing line 7's player_fn="mpv" to player_fn="iina" I added iina to the case statement with:

"iina")
    if echo "$status_code" | grep -vE "^2.*"; 
        printf "${c_red}\nCannot reach servers!"
  rm "${logfile}.new"
    else
        nohup $player_fn "$play_link" > /dev/null 2>&1 &
  printf "${c_green}\nVideo playing"
  mv "${logfile}.new" "$logfile"
    fi;;

After this I simply commented out the combined redirection above and it finally worked!

nohup $player_fn "$play_link" #> /dev/null 2>&1 &

Trying to change the redirect to nohup $player_fn "$play_link" 2> /dev/null & doesn't seem to work. The return from nohup that I'm getting now says, "nohup: ignoring input and appending output to 'nohup.out'"

I'll keep tinkering with this to see what works.

EDIT: got it to work with that last redirect I wrote so long as I remove the ampersand. Will it work with the combined redirect if I remove the ampersand?

EDIT2: Ok, the issue was the ampersand. The original form works perfectly well so long as I don't have the ampersand there.

And just for completion this is my system: Late 2021 MacBook Pro 16" M1 Max macOS Monterey 12.1 ani-cli version available from git clone today (2022-01-01). iina version available from brew today.

Nice! and i tried to track down where that ampersand was added because it wasnt there before ,turns out it was in the commit where setsid was changed to nohup,now i didnt know if it was to make it work with mac but seeing this report confirms that we can probably remove that ampersand in production code,going to test this current ani-cli version and remove the ampersand and see if it works, if it does maybe you could make a pull request. idk if thats fine with the other maintainers tho since its only a 1 character change😂

port19x commented 2 years ago

It is, as long as you remove the ampresand everywhere and confirm it works

RaynardGerraldo commented 2 years ago

yep can confirm that it does work without the ampersand for mpv and vlc. @tennyson-mccalla feel free to open a pr and reconfirm it on ur end

tennyson-mccalla commented 2 years ago

Done (though I messed up the commit a bit before correcting it): https://github.com/pystardust/ani-cli/pull/256

port19x commented 2 years ago

Ahem, this shouldn't be closed yet

tennyson-mccalla commented 2 years ago

Whoops! My mistake 🙈