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 3: Use Cached Regions for Auto-Stacking and add Proper Layer Handling #403

Closed softworkz closed 9 months ago

softworkz commented 10 months ago

SubtitleProviderSsaAss: Parse layer SubtitleProviderSsaAss: Cache regions

softworkz commented 10 months ago

Explanations

"Unknown Fields

The two fields you get from FFmpeg and where you had questionmarks are ReadOrder and Layer.

Layer is the original value from the ASS content.

ReadOrder is a counter which increases strict monotonically and indicates the original order of dialogue lines. libass takes this as an input when the subtitle events are provided subsequently. While the ASS spec doc says that the order of dialog lines doesn't matter, this isn't quite true. One example is that when two lines have the start time, then it surely matters which of the lines came first in the order. This can affect the stacking order of multiple unpositioned elements or the z-order of positioned and overlapping elements. There's at least one more case which I don't remember right now, probably more.

For a correct implementation, it would be required to put incoming events into an ordered map or something and then process them in the right order, this would also allow to prevent processing duplicated events. But maybe this isn't needed here. It seems that you are getting and processing all ASS lines from all subtitle streams even before playback start, right? Is that in all cases? If yes, then there's probably no need to care about the ReadOrder field.

Layer

The Layer field is in fact important for two reasons:

This PR addresses both

Stacking of Text Blocks

I recently asked why elements are overlapping and I could not find any documentation how this is handled and can be controlled. Eventually I figured it out myself: It depends on the TimedTextRegion. Only text elements in the same region are stacked, otherwise they are rendered on top of each other and can overlap. The reason why stacking has been somewhat working before, is that the style parsing was often failing (shadow width is not an integer) and that caused the default style's region to be used in many cases: Same region => Stacking After this was fixed, the styles were applied correctly and that means that elements having different styles were no longer stacked (because each style has its own region).

The problem here for us is this: ASS performs collision detection among all elements which are not absolutely positioned and which are on the same layer, while it allows the elements to have different alignments and different margins.

=> We cannot replicate this. We can set margins (in form of TimedTextPadding) only per region when we use separate regions, we don't get stacking

The only solution for this would be to implement a manual collision handling and I'm not ready for this. It costs a large amount of time to get this right which I don't have. Hence, I could only come up with an approximate approach, which is fine in most cases:

The TimedTextRegions are being put into a cache now. If a region is required with the exact same parameters as a cached region, then the cached region is used. Otherwise A new region is added to the cache and used.

This also reduces resource usage and flickering (in those cases where subs are animated and changes happen at a high frequency).

The subRegion variable in this PR is always copied from the style now. All manipulations are still applied to it, but it only serves as a prototype for the region parameters, while the actual region is retrieved from the region cache at the end.

lukasf commented 10 months ago

Good find about the regions and auto stacking. I also assumed that it might be something about subs having different styles. So the regions instance it is.

Two remarks on this:

  1. I do not see the ZIndex being set in this PR. But it is set in the All-in-one PR.
  2. If the Region's ZIndex is set to the Layer number, we don't need the SSACachedRegion. We can just compare the region's ZIndex (which is already done).
softworkz commented 10 months ago
  1. do not see the ZIndex being set in this PR. But it is set in the All-in-one PR.

Oh yea, seems it got lost when rebasing/isolating this PR.

2. If the Region's ZIndex is set to the Layer number, we don't need the SSACachedRegion. We can just compare the region's ZIndex (which is already done).

Thanks for your careful review. 😉 - and yes that's correct. I had already tried to put the regions into the vector directly, but I must have done something wrong as it ended up in some crashes. I don't really have experience in dealing with those WinRT structs, so it would be great when you could perhaps handle that part?

lukasf commented 10 months ago
  1. If the Region's ZIndex is set to the Layer number, we don't need the SSACachedRegion. We can just compare the region's ZIndex (which is already done).

Thanks for your careful review. 😉 - and yes that's correct. I had already tried to put the regions into the vector directly, but I must have done something wrong as it ended up in some crashes. I don't really have experience in dealing with those WinRT structs, so it would be great when you could perhaps handle that part?

Sure, I can take a look at this, once we have the AIO on a local branch.

softworkz commented 10 months ago

Sure, I can take a look at this, once we have the AIO on a local branch.

I've been busy this week but I'll do the rebase now. Against which branch do you want me to do it - winui or master?