Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rendering with libass to match xy-VSFilter #134

Merged
merged 1 commit into from
May 4, 2022

Conversation

TheOneric
Copy link

@TheOneric TheOneric commented Feb 24, 2022

Since ASS rendering depends on the storage size of the video libass
needs to know about it to render the subtitles correctly. If it isn't
told about the storage size libass uses the value from PlayRes{X,Y} as
a guess, but this isn't always correct.
With Aegisub currently always rendering at storage resolution
this ends up the same as the frame size.


I'm confident the “rendering size == storage size” assumption holds true for Aegisub’s subtitle preview. From a glance it also appears to hold true for Aegisub’s csri frontends for CLI and AviSynth, but I'm less confident about those two. However, since csri currently only gets one size: if it isn't correct, this should also affect VSFilter-based backends.

You can see the different rendering on the following sample. Load it into Aegisub without resampling to video resolution and switch between xy-VSFilter or vsfilter_textsub (not VSFilterMod!) and libass; without the patch the result differs, with the patch it will match.

[Script Info]
ScriptType: v4.00+
WrapStyle: 0
ScaledBorderAndShadow: yes
YCbCr Matrix: None
PlayResX: 640
PlayResY: 360

Video File: ?dummy:25.000000:40000:1920:1080:47:191:225:
Video Position: 39

[Aegisub Project Garbage]
Video File: ?dummy:25.000000:40000:1920:1080:47:191:225:
Video Position: 39

[V4+ Styles]
Format: Name, Fontname, Fontsize, PrimaryColour, SecondaryColour, OutlineColour, BackColour, Bold, Italic, Underline, StrikeOut, ScaleX, ScaleY, Spacing, Angle, BorderStyle, Outline, Shadow, Alignment, MarginL, MarginR, MarginV, Encoding
Style: Default,DejaVu Sans,65,&H007F67D0,&H00187DC1,&H00000000,&H00D4AA86,-1,0,0,0,100,100,0,0,1,0.4,0,7,10,10,10,1

[Events]
Format: Layer, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text
Dialogue: 0,0:00:00.00,0:00:06.99,Default,,0,0,0,,{\pos(52,72)\an7\1c&HB3B3B3&\fscx100\fscy100\p1}m 2 34 l 8 60 112 37 107 24
Dialogue: 0,0:00:00.00,0:00:06.99,Default,,0,0,0,,{\frx18\fry32}{\fs36\pos(53.7385,101.846)\frz2.645}Some Text in a Sign

A furhter example can be found in Aegisub/Aegisub#269, but this is also affected by another bug where Aegisub doesn't de-squeeze anamorphic content before display in the subtitle preview.


Corresponding merge request for TypesettingTools/Aegisub: TypesettingTools#147

Since ASS rendering depends on the storage size of the video libass
needs to know about it to render the subtitles correctly. If it isn't
told about the storage size libass uses the value from PlayRes{X,Y} as
a guess, but this isn't always correct.
With Aegisub currently always rendering at storage resolution
this ends up the same as the frame size.
@TheOneric
Copy link
Author

To clarify: the behaviour with patch will match not just xy-VSFilter but also classic guliverkli- and guliverkli2-VSFilter. VSFilterMod differs from both patched and unpatched Aegisub with libass iirc but VSFilterMod is explicitly only meant for hardsubbing and thus intentional incompatible with softsub renderers. Using libass without calling ass_set_storage_size to render genuine ASS subtitles is considered incorrect usage.
Applying this patch cannot make the output worse than currently, the remarks about the different csri interfaces are just sidenotes and if anything is wrong with them it will also already affect the VSFilter-backend just as much.

If you want to test this against historic builds, Aegisub r8250 has xy-VSFIlter bundled and will render the sample like follows (and patched Aegisub with libass will do the same):
correct_rendering

Currently, without the patch applied though, Aegisub with libass renders this, which is clearly incorrect:
missing_storage_size

@vxzms
Copy link

vxzms commented May 4, 2022

It looks fine if use 1920x1080 video, but display will change if use other resolution, such as 1280x720:
dummy_001_0

This is very similar as HomeOfVapourSynthEvolution/xy-VSFilter#1. I don’t think priority match xy-VSFilter is correct, maybe they plan to solve it through LayoutResX/Y in the future, but maintain the consistency of rendering effects at different resolutions is more important.

@TheOneric
Copy link
Author

TheOneric commented May 4, 2022

I don’t think priority match xy-VSFilter is correct

Doing what XySubFilter, xy-VSFilter, guliverkli2 and guliverkli all do identical is the most correct correct thing one can do with regards to ASS and what libass strives to emulate.

It looks fine if use 1920x1080 video, but display will change if use other resolution, such as 1280x720

If you meant Aegisub+libass without the patch: It shouldn't if you followed the instruction to not resample the resolution.
If you meant it looks fine with the patch on 1080p, but looks different when paired with a different resolution video then this is intended. \fry, \frx, \blur and others depending on the video resolution is part of the ASS format and must be done for compatibility with existing real-world subs and for compatibility with other softsub renderers.

Changing the format in an incompatible way as you seem to suggest is simply not an option open for debate - period.

Moreover, Aegisub without the patch won't even match other libass-using renderers, as they by now mostly set ass_set_storage_size. So there's no value at all in not rectifying Aegisub's misuse of the libass library.

As for “consistency of rendering effects at different resolutions” the correct way to achieve this is via a coordinated introduction of a new distinct header (pair) in all major softsub ASS renderers. See libass/#597 for a current discussion on this.

See also libass docs, if you still have doubts:
https://github.com/libass/libass/blob/b77c58cd43281a05cd242888a8aac39668d67266/libass/ass.h#L391-L395

@IbarakiKasen
Copy link
Collaborator

IbarakiKasen commented May 4, 2022

... but also classic guliverkli- and guliverkli2-VSFilter.

In my test the current behavior matches what MPC(-HC) does on a 1920x1080 video:

clsid2/mpc-hc (1.9.21.2):
img

guliverkli2/mplayerc_20100214 (6.4.9.1 (revision 107)):
img

guliverkli/mpc2kxp6490 (6.4.9.0):
img

@vxzms
Copy link

vxzms commented May 4, 2022

If you meant Aegisub+libass without the patch: It shouldn't if you followed the instruction to not resample the resolution. If you meant it looks fine with the patch on 1080p, but looks different when paired with a different resolution video then this is intended. \fry, \frx, \blur and others depending on the video resolution is part of the ASS format and must be done for compatibility with existing real-world subs and for compatibility with other softsub renderers.

My tests are based on builds with the patch.

Moreover, Aegisub without the patch won't even match other libass-using renderers, as they by now mostly set ass_set_storage_size. So there's no value at all in not rectifying Aegisub's misuse of the libass library.

Indeed, I realized this when using mpv. And I’ve seen this issue and libass/libass#597. I can also understand why matching VSFilter series even though VSFilter sucks.

I am very supportive of fix the Aegisub's misuse of libass to maintain compatibility with other players, but I don't understand why not make changes in libass after LayoutResX/Y script headers done, then repair downstream software.

As for “consistency of rendering effects at different resolutions” the correct way to achieve this is via a coordinated introduction of a new distinct header (pair) in all major softsub ASS renderers. See libass/#597 for a current discussion on this.

After patch whether only videos with the same resolution (storage size) between subtitle makers and softsub viewers can maintain the same rendering effect, then only set LayoutResX/Y = Subitle makers’ storage size can resolve it.

@wangqr
Copy link
Owner

wangqr commented May 4, 2022

Moreover, Aegisub without the patch won't even match other libass-using renderers, as they libass/libass#591 mostly set ass_set_storage_size. So there's no value at all in not rectifying Aegisub's misuse of the libass library.

I agree. Aegisub+libass should always match other libass-based tools/players. I guess I'll merge this after verifying with ffmpeg and VLC.

@TheOneric
Copy link
Author

TheOneric commented May 4, 2022

I guess I'll merge this after verifying with ffmpeg and VLC.

Just a note: unlike e.g. mpv which used ass_set_storage_size since it was added to libass in 2013, ffmpeg and VLC fixed this only more recently. Regarding ffmpeg you should be fine with any of the most recent (since it has been backported to some of those) releases of the 4.x and 5.x series. As for VLC, 3.0.17 doesn't have this yet, but you can use the nightly 3.0.x or 4.0 builds.

@wangqr wangqr merged commit bc8ed62 into wangqr:dev May 4, 2022
@TheOneric
Copy link
Author

TheOneric commented May 4, 2022

In my test the current behavior matches what MPC(-HC) does on a 1920x1080 video:
clsid2/mpc-hc (1.9.21.2):
guliverkli2/mplayerc_20100214 (6.4.9.1 (revision 107)):
guliverkli/mpc2kxp6490 (6.4.9.0):

Are you sure you are using the correct VSFilter.dll in your guliverkli and guliverkli2 tests?
Using the last 32bit VSFilter release of guliverkli2 (guliverkli2-i386_VSFilter-2.39_20090624) I get a rendering matching patched Aegisub + libass and xy-VSFilter, see below. Unfortunately Aegisub doesn't like the last guliverkli(1) VSFilter.dll and guliverkli-mplayer doesn't like the virtual GPU of my VM, so I couldn't test with that.
guliverkli2-i386_VSFilter-2 39_20090624

If you can somehow reproduce your results with the original guliverkli(1|2) VSFilter.dll, I'd be interested in hearing the details of it.


As for MPC-HC's internal subtitle render (ISR): it is known to deviate in several (i.e. not only the torage size issue) incompatible ways from classsic VSFilter behaviour and the expectation is that xy-VSFilter/XySubFilter is used for native ASS subtitles. I'm not sure if its ISR can even be used in Aegisub; is there some dll for it?

Even so, when looking closely at your screenshots you'll notice that none of them match unpatched Aegisub+libass nor does MPC-HC match the supposed guliverkli renderings:
unpatched-libass_vs_MPCHC-ISR


To also address vxzms’ concern: Once LayoutRes is hopefully adopted by major renderers, the idea is to make Aegisub set the headers automatically (the exact semantics of when and to what still being discussed) so all new subs would become invariant under the video's storage size while old subs continue to be rendered as intended.

@TheOneric TheOneric deleted the libass_storageSize branch May 4, 2022 12:55
@vxzms
Copy link

vxzms commented May 4, 2022

Thx TheOneric, I wish it could be discussed and implemented as soon as possible.

Vapousynth plugin https://github.com/HomeOfVapourSynthEvolution/VSFilter can be used in Aegisub, it based MPC-BE 1.5.3, and display same as MPC-HC's internal subtitle render.

dummy_003_39

@IbarakiKasen
Copy link
Collaborator

IbarakiKasen commented May 4, 2022

Are you sure you are using the correct VSFilter.dll in your guliverkli and guliverkli2 tests?

Nope. I was using the MPC executable directly, downloaded from these 2 links:
https://sourceforge.net/projects/guliverkli/files/Media%20Player%20Classic/MPC%206.4.9.0/
https://sourceforge.net/projects/guliverkli2/files/Media%20Player%20Classic/6.4.9.1/

I assumed that these MPC players behave the same way as VSFilter.dll, because I think that VSFilter.dll is just a wrapper of MPC's RTS.cpp. The VSFilter.dll from MPC-HC's release (https://github.com/mpc-hc/mpc-hc/releases/tag/1.7.13, in standalone_filters) matches the behavior of MPC-HC's internal subtitle renderer. Turns out this is not the case for guliverkli and guliverkli2's MPC and VSFilter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants