CarterLi / iina

The modern video player for macOS with additional features and bug fixes.
https://iina.io
GNU General Public License v3.0
678 stars 29 forks source link

Contributors Request #4

Open CarterLi opened 2 years ago

CarterLi commented 2 years ago

As IINA is my favorite macOS player, I really hope that IINA can go further. However the official repo seemed to be no longer in active development ( but not dead, hopefully ).

Since then, many features, fixes were made in various personal fork repo. However because they were separated in different repos:

  1. We cannot release binaries that includes all of these features / fixes
  2. Other IINA users are hard to find them

So I decided to create this iina-plus group and want it to be open and easy to contribute ( not open PR, but join the group and push code directly into the repo )

However, to prevent from spam, there is a requirement to prove you have the ability to contribute.

CarterLi commented 2 years ago

@low-batt I really appreciate your PRs with very detailed explanation and most of them have been cherry-picked into this repo. Would you like to join this group?

low-batt commented 2 years ago

I tried forking iina-plus/iina today as I like the ability to look over your submission on GitHub the PR workflow gives you before you push it into the public repository. Yes I'm paranoid about accidentally messing up and pushing something I didn't intend. I found I couldn't fork ina-plus/iina. Seemed like this is a restriction on GitHub free accounts.

So yes, if you grant me access I will add my changes to this fork as well.

What is your vision for coordinating changes given this setup?

low-batt commented 2 years ago

I have a local commit with the fix for the camera housing issue but: low-batt@gag iina (develop>)$ git push ERROR: Permission to iina-plus/iina.git denied to low-batt.

CarterLi commented 2 years ago

Let me have a check

CarterLi commented 2 years ago

Try again please?

low-batt commented 2 years ago

Worked! I haven't had a chance to look through what all you picked up from me. Are there other commits of mine you want me to add?

CarterLi commented 2 years ago

Some commits you made had conflict with my repo. I skipped them. You may have a check.

low-batt commented 2 years ago

I will check into it and where appropriate adjust the commits and get them pushed into this fork. May take me a while.

low-batt commented 2 years ago

Pushed a commit with the fix for https://github.com/iina/iina/issues/3574

low-batt commented 2 years ago

I have just submitted a pull request with a fix for issue https://github.com/iina/iina/issues/3475. I was unable to reproduce the crash, so I can't confirm it will fix the crashes. Do you want to wait for the core developers to review the fix, or do you want me to push it to iina-plus?

CarterLi commented 2 years ago

iina-plus is a repo that experiments new features and bug fixes that haven't been officially accepted by upstream.

It's ok to push your changes directly into iina-plus repo and we may revert it later if necessary. Let's release a new binary for users to test the changes.

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3475

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3584

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3590

Going to have to keep an eye on this one. The fix is correct and needed. But because IINA is now waiting for mpv to shutdown we may find issues in the way mpv is being shutdown that usually were hidden by premature termination of the application. I sometimes see log messages from mpv that makes me think they might have some bugs in their quit command.

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3551

low-batt commented 2 years ago

Pushed fix for iina#3593

This fixes an existing issue in the shutdown sequence that was made more obvious by the fix to properly wait for mpv to terminate.

CarterLi commented 2 years ago

Thanks.

If your PR gets merged, I'd like to rebase everything on top of the official branch. Please don't be surprised if you get any conflicts.

low-batt commented 2 years ago

Pushed a fix to the fix for https://github.com/iina/iina/issues/3558 Simple issue with not closing the mask window if IINA exits directly from full screen. I'm ready to help with resolving any merge conflicts. I'm expecting it will take a while for them to review all my changes and get them merged.

low-batt commented 2 years ago

Pushed a commit to update the copyright year to 2022

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3601

low-batt commented 2 years ago

Pushed a tiny fix for iina#3607

low-batt commented 2 years ago

Pushed small fix for iina#3211

CarterLi commented 2 years ago

Just rebased everything onto the official repo

low-batt commented 2 years ago

Pushed a revision to the fix for iina#3558

CarterLi commented 2 years ago

Pushed a revision to the fix for iina#3558

Since you have closed the old PR in the official repo, please consider amending your addition fixes into the old one, to avoid possible rebase conflicts. @low-batt

low-batt commented 2 years ago

That is beyond my knowledge of git. How would I do that? I've only amended commits that had not been merged.

CarterLi commented 2 years ago

Just remove the old commit using git rebase -i then commit your new changes.

Force-pushing is needed. It's dangerous but necessary to maintain a fork repo

low-batt commented 2 years ago

There are 3 commits related to the camera I had to drop. I had to go way back to get that first one. This is what the rebase looks like. Wanted to make sure you think this is ok before I force push.

low-batt@gag iina (develop $=)$ git rebase -i HEAD~55
Auto-merging iina/Info.plist
CONFLICT (content): Merge conflict in iina/Info.plist
Auto-merging iina.xcodeproj/project.pbxproj
error: could not apply 9d6d4139... Add AppleScript support
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 9d6d4139... Add AppleScript support
low-batt@gag iina (develop *+$%|REBASE 45/55)$ git status
interactive rebase in progress; onto b6e6dedb
Last commands done (45 commands done):
   pick a2900c3f Backout fix for CPU is consumed by OSC while hidden iina#3601
   pick 9d6d4139 Add AppleScript support
  (see more in file .git/rebase-merge/done)
Next commands to do (10 remaining commands):
   pick f0a99d53 Revert `MACOSX_DEPLOYMENT_TARGET` back to `10.12`
   pick a785a18a Update README and ISSUE_TEMPLATE
  (use "git rebase --edit-todo" to view and edit)
You are currently rebasing branch 'develop' on 'b6e6dedb'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
    modified:   iina.xcodeproj/project.pbxproj
    new file:   iina/IINA.sdef
    modified:   iina/MPVPlaylistItem.swift
    modified:   iina/MPVTrack.swift
    modified:   iina/PlayerCore.swift
    modified:   iina/PrefGeneralViewController.swift
    new file:   iina/Scripting/AppDelegate+Scripting.swift
    new file:   iina/Scripting/Commands/NextCommand.swift
    new file:   iina/Scripting/Commands/NextFrameCommand.swift
    new file:   iina/Scripting/Commands/PauseCommand.swift
    new file:   iina/Scripting/Commands/PlayCommand.swift
    new file:   iina/Scripting/Commands/PlayPauseCommand.swift
    new file:   iina/Scripting/Commands/PreviousCommand.swift
    new file:   iina/Scripting/Commands/PreviousFrameCommand.swift
    new file:   iina/Scripting/FourCharCode+Extensions.swift
    new file:   iina/Scripting/MPVPlaylistItem+Scripting.swift
    new file:   iina/Scripting/MPVTrack+Scripting.swift
    new file:   iina/Scripting/PlayerCore+Scripting.swift

Unmerged paths:
  (use "git restore --staged <file>..." to unstage)
  (use "git add <file>..." to mark resolution)
    both modified:   iina/Info.plist

Untracked files:
  (use "git add <file>..." to include in what will be committed)
    deps/include/libavcodec/defs.h
    deps/include/libavdevice/
    deps/include/libavfilter/
    deps/include/libavutil/detection_bbox.h
    deps/include/libpostproc/
    deps/include/libswresample/

low-batt@gag iina (develop *+$%|REBASE 45/55)$ 
low-batt@gag iina (develop +$%|REBASE 45/55)$ git rebase --continue
[detached HEAD b8f271c3] Add AppleScript support
 Author: 李通洲 <carter.li@eoitek.com>
 19 files changed, 1150 insertions(+), 3 deletions(-)
 create mode 100644 iina/IINA.sdef
 create mode 100644 iina/Scripting/AppDelegate+Scripting.swift
 create mode 100644 iina/Scripting/Commands/NextCommand.swift
 create mode 100644 iina/Scripting/Commands/NextFrameCommand.swift
 create mode 100644 iina/Scripting/Commands/PauseCommand.swift
 create mode 100644 iina/Scripting/Commands/PlayCommand.swift
 create mode 100644 iina/Scripting/Commands/PlayPauseCommand.swift
 create mode 100644 iina/Scripting/Commands/PreviousCommand.swift
 create mode 100644 iina/Scripting/Commands/PreviousFrameCommand.swift
 create mode 100644 iina/Scripting/FourCharCode+Extensions.swift
 create mode 100644 iina/Scripting/MPVPlaylistItem+Scripting.swift
 create mode 100644 iina/Scripting/MPVTrack+Scripting.swift
 create mode 100644 iina/Scripting/PlayerCore+Scripting.swift
Successfully rebased and updated refs/heads/develop.
low-batt@gag iina (develop $<>)$ git status
On branch develop
Your branch and 'origin/develop' have diverged,
and have 51 and 54 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

nothing to commit, working tree clean
low-batt@gag iina (develop $<>)$ 
CarterLi commented 2 years ago

It's hard to tell whether it's ok or not. Just make sure that it builds without errors and go on

low-batt commented 2 years ago

Was getting sleepy, so held off until brain was awake again. I confirmed build worked. I compared the git log taken before and after showed only the 3 camera housing fix related commits were removed. I have now force pushed. I always find that operation scary.

I have pushed the replacement fix for the camera housing as well as two additional commits coming out of code review for iina#3334, problems with taking a screenshot.

CarterLi commented 2 years ago

I will have a holiday for about 2 weeks. During the holiday, I will have no access to my mac. Please release new builds yourself if necessary @low-batt

low-batt commented 2 years ago

Relax and have a great time on vacation. :-)

Not sure what the procedure is for creating a release. I've never done that on GitHub. I will try and figure that out.

I was going to be asking you for help on these problems. Seeing some really strange behavior in the logs that I can't explain.

low-batt commented 2 years ago

Pushed fix for iina#3620

I've been abusing this issue and using it for communication. Thoughts on enabling GitHub discussions for this repo?

CarterLi commented 2 years ago

Pushed fix for iina#3620

I've been abusing this issue and using it for communication. Thoughts on enabling GitHub discussions for this repo?

I was enabling GitHub discussion. However I found that I always forgot to check it so I closed it.

low-batt commented 2 years ago

Pushed a commit that tweaks the app and document icons to match up with the latest HIG requirements. Thank you to @Zabriskije who provided the updated icons. More details can be found in discussion https://github.com/iina/iina/discussions/3670.

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3679

IINA already supports playing animated GIFs, but was not associated with .gif files.

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3692

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3695

CarterLi commented 2 years ago

With the latest upstream changes, I got conflicts when rebasing. Can you please resolve it?

low-batt commented 2 years ago

Yes, I will take care of rebasing, probably later today. Reviewing and merging is currently happening in the main IINA repository. I'm about to respond to review feedback.

low-batt commented 2 years ago

Pushed commit that adds OSD messages for file loop and playlist loop, https://github.com/iina/iina/issues/3229

There is other cleanup that might be needed for IINA+. There are commits that were added for debugging when trying to figure out the hang that turned out to be an issue with hardware drivers and FLAC encoded files. I will check over the commits and get IINA+ aligned with the official repository.

low-batt commented 2 years ago

Resolved rebase conflicts. Up to date with upstream for now. They are reviewing PRs, so more merging may be required soon.

low-batt commented 2 years ago

Pushed fix for https://github.com/iina/iina/issues/3462

low-batt commented 2 years ago

More commits have been merged, so IINA+ is behind again. Likely these will trigger merge conflicts as well. I will look into rebasing again tomorrow.

low-batt commented 2 years ago

Starting rebase again. May take a while as I expect merge conflicts with my changes.

@CarterLi Did you notice the request that you post a HDR PR in the official repository? I told the devs to hold off merging my changes to FFmpegController in PR https://github.com/iina/iina/pull/3461 as I know there are lots of changes to that source for HDR. Better to get HDR in first and let me deal with merge conflicts.

low-batt commented 2 years ago

Completed rebase. Noticed HDR PR posted. 😄

low-batt commented 2 years ago

Starting rebase with IINA again.

low-batt commented 2 years ago

Completed rebase with IINA.

CarterLi commented 2 years ago

Did a major rebase

low-batt commented 2 years ago

I pulled the latest IINA+ and rebuilt.

Starting with both File Loop and Playlist Loop enabled in defaults. I bring up IINA and both are now correctly checked in the Playback menu. So that is fixed. I disable Playlist Loop using the menu item. I quit IINA. I check defaults and Playlist Loop is still enabled in the saved defaults.

I also experienced another inconsistency. Starting again with both enabled in defaults. I brought up IINA. Menu correctly shows both enabled. Opening playlist, the Playlist Loop button shows enabled, but the File Loop button is in the disabled state.

On another topic, I am actively working on the issues with the display refresh rate matching feature.