ffmpeginteropx / FFmpegInteropX

FFmpeg decoding library for Windows 10 UWP and WinUI 3 Apps
Apache License 2.0
211 stars 53 forks source link

ASS Fixes - All-in-One #405

Closed softworkz closed 9 months ago

softworkz commented 10 months ago

The other PRs might not all build and in order to save you from combining all the pieces, this is the full range of ASS fixes combined in a single PR (not for merging).

brabebhin commented 10 months ago

A bit of branch management here: This PR should target winui-build branch, as that's where we split the config file into smaller configuration classes. If you merge this you will break master.

@lukasf I am personally happy with the winui-build branch, been using it in my own app, and I think that can go for the merge, if we find other smaller bugs we could just fix them individually. We can distribute a preview version of the next version in nuget and prepare for stable release maybe around April?

brabebhin commented 10 months ago

I would recommend creating the branches directly in this repo, you should be able to do so, so you do not need to keep your version in sync.

softworkz commented 10 months ago

AFAIK, only members of a repository can create branches and only when they have been assigned a role which allows this.

brabebhin commented 10 months ago

You are a contributor to this repository. You should have the same rights as me when it comes to branching.

Give it a try.

softworkz commented 10 months ago

You are a contributor to this repository. You should have the same rights as me when it comes to branching.

Give it a try.

Pushing submit_ass_fixes_all
Remote: Permission to ffmpeginteropx/FFmpegInteropX.git denied to softworkz.
Error encountered while pushing to the remote repository: Git failed with a fatal error.
Git failed with a fatal error.
unable to access 'https://github.com/ffmpeginteropx/FFmpegInteropX.git/': The requested URL returned error: 403

A contributor doesn't have push rights to a repository. You need to be added as a collaborator with at least Write access:

image

Then you get an invitation which you can accept or deny. Hence I was pretty sure that I don't have the permission (no invitation) 😄

brabebhin commented 10 months ago

Well, then you're gonna have to do this the hard way :)

softworkz commented 10 months ago

Well, then you're gonna have to do this the hard way :)

Why hard? There's not really a difference whether a PR branch is in my forked repo or here.

brabebhin commented 10 months ago

You based the branch on master branch but the code seems to be written to work with the winui-build branch. Can you build your branch in your repo?

lukasf commented 10 months ago

@softworkz I have invited you as member of this repo with write access. Feel free to join. It would make it easier for us to test your PRs and collaborate on them, if they are created directly on this repo, instead of being on a fork of yours. Edit: You don't need to transfer all PRs. But it would help if you could push the all-in-one PR to a local branch over here, for simplified testing.

@brabebhin Why do you think that this needs to be done on the WinUI branch? The WinUI branch only contains some fixes for sub flickering. It should not affect any of these styling/layout related things. I think all the ASS improvements can continue to target the main branch.

For the current state of the WinUI branch: I don't think that this is ready to merge yet. It contains UWP, WinUI and Desktop build configs. But we decided that we don't want/need a WinUI build config, only UWP and Desktop. The Desktop build config should get an optional HWND (IntPtr) parameter, so we can get the WindowId from that, if we are running on WinUI. That way, we can ship without WinUI dependency, but still use WinUI (especially for HDR detection) if we run in a WinUI context. Additionally, the NuGet packages are not how they are planned. So there is still some work to do over there.

brabebhin commented 10 months ago

@lukasf Because the winui-build branch contains the split configuration classes, which are used in this PR. I'm guessing this is a mistake caused by the multi forky repo thing. Which is why i suggested to create a branch here.

The Desktop build config should get an optional HWND (IntPtr)

I thought we already did this. Guess I need to check the code again xD

softworkz commented 10 months ago

@softworkz I have invited you as member of this repo with write access. Feel free to join. It would make it easier for us to test your PRs and collaborate on them, if they are created directly on this repo, instead of being on a fork of yours.

Great. Thanks!

Edit: You don't need to transfer all PRs. But it would help if you could push the all-in-one PR to a local branch over here, for simplified testing.

Transferring is not a big thing with a good Git client. But I guess I'll have to rebase them. I had somehow thought that the configuration split would already be in the main branch, that's why I thought it would be better not to load more and more things onto the winui branch.

Please let me know with which branch you want to proceed with this, then I'll rebase all PRs and the all-in-one one. (one one one... 😆 )

brabebhin commented 10 months ago

Master should be fine, if there's merge conflicts for winui-build they shouldn't be too hard to fix. Besides this has nothing winui specific.

softworkz commented 9 months ago

superceded by #410