search

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 #410

Closed softworkz closed 9 months ago

softworkz commented 9 months ago

Targets winui-build and the source branch is local to this repo now :-)

Supercedes #405

brabebhin commented 9 months ago

Thanks @softworkz this is so much simpler to deal with. I think all the other fragment PRs can be closed too.

By eye balling this it looks ok, roughly similar to what I was using to test your full changes on my local branches (I actually missed some changes haha).

softworkz commented 9 months ago

Thanks @softworkz this is so much simpler to deal with.

Last time, @lukasf said, my PR would be all too much at once - that's why I sliced it a bit.

I think all the other fragment PRs can be closed too.

Sure, feel free to do so.

This PR is also the most "trustable" one, because I didn't need to fiddle around with things for the separation. These are the exact commits which I have in my production branch, cherry-picked in the same order :-)

softworkz commented 9 months ago

This looks good on my files.

Great! Thanks a lot for testing.

lukasf commented 9 months ago

I finally found the time to do some testing on this, sorry for being so late on this. Very well done @softworkz, this looks really good. I did not find any rendering issues with the files I have at hand. Also the sizing looks good and very close to how other players render it.

I fixed two things: First, I noticed an issue that the first sub after a seek would not show up. This was not your fault, I introduced this when changing subs for WinUI build. It is fixed now. The second one is an excepetion occuring when the layer tag is empty (stoi throws on empty string), causing such files to not show subs at all. There are a few files out there which do not set layer. I fixed it by checking the next character, similar to how the other items are checked.

I have also tried to remove the CachedSubtitleRegion class. For my files, this works fine, but my test files are not so intense on subtitle styling. So I pushed this on a separate branch "remove_cached_regions". It would be great if you could test this with your files @softworkz, to see if you get any exceptions like you had before. If it works fine, we should include it here.

softworkz commented 9 months ago

I fixed two things: First, I noticed an issue that the first sub after a seek would not show up.

Yup, I've seen this, but I didn't feel that it matters a lot, but I'm still glad that you fixed it!

have also tried to remove the CachedSubtitleRegion class. For my files, this works fine, but my test files are not so intense on subtitle styling. So I pushed this on a separate branch "remove_cached_regions". It would be great if you could test this with your files @softworkz, to see if you get any exceptions like you had before. If it works fine, we should include it here.

I don't have to. I'm quite sure it was a coding mistake. It wasn't dependent on the file - it happened always. So I think you can just include it right away.

Thanks a lot for looking into this.

lukasf commented 9 months ago

Merged. Thank you @softworkz