Revert incompatible scaling changes

[TheOneric]

This reverts commits 5eee6d9 and 7a83376 from #24 and its follow-up fix.

The dependence on the videos storage resolution described in #17 is actually part of the ASS format and required to be kept for compatibility with existing files. Changing the scaling logic to not take storage resolution into account, breaks existing, real-world files.
Keeping compatible with this beaviour is also why the xy-SubFilter interface in include/SubRenderIntf.h takes an "originalVideoSize" parameter; as unlike xy-VSFilter, xy-SubFilter can render at native display resolution. (While for xy-VSFilter, rendering always happens at the videos original storage resolution, so the rendering dimension already carries the informaton and a separate parameter is not needed).

See libass/libass#641 for a potential, non-breaking way to get video-resolution-indepent subtitles.

---

Unfortunately, I wasn't able to figure out how to make this build in GitHub Actions and I don't have any local VS or Windows dev setup. As it just reverts two commits it should probably be safe though. Iiuc the other commit from #31 is unrelated to sacling and #32 fixes this unrelated commit from #31.

[kedaitinh12]

Can you make binary?? Thanks

[Masaiki]

1. This is a bug, fix scale between subtitle and video on CWord #24 aims to fix the scaling problem of frx fry two tags in the case of inconsistent playres and video resolution, this fix has nothing to do with the scaling of other tags
2. this bug has nothing to do with storage resolution (in another way, the shape of the pixels), and the storage resolution issue should be left to the player
3. The reason of this bug is probably because xy-VSFilter is based on VSFilter2.39, MPC-BE/HC have fixed this problem.

[TheOneric]

1. This is a bug

It is a part of the ASS format. Likely it didn't start out as an intentional design choice, but nonetheless this is how ASS works, ASS files expect renderers to behave and not doing so just breaks existing files. Practically, the old behaviour is the correct one as far as ASS rendering is concerned.

problem of frx fry two tags in the case of inconsistent playres and video resolution

There is no requirement or even indication in ASS itself, that PlayRes has anything at all to do with the video’s storage resolution. The strongest connection is Aegisub by default nudging (not forcing!) users to equate them (and ASS/SSA is older than Aegisub).
Again: it is required for conforming ASS renderers to display those tags differently depending on the video’s storage resolution.

this fix has nothing to do with the scaling of other tags

That just means, even with this ""fix"" some tags still behave differently depending on the resolution of the video the subs are paired with.

2. this bug has nothing to do with storage resolution (in another way, the shape of the pixels)

The change doesn't consider storage resolution, yes — but for correct rendering one must consider storage resolution. Also storage resolution is not the same as the pixel shape (PAR), but a full resolution like 1920×1080.

MPC-BE/HC have fixed this problem.

They didn't "fix" it, MPC-HC (before -BE split and before clsid2 took over -HC iinm) broke compatibility with existing files in more than just this instance, which is why barely any complex subs I'm aware of target those renderers; installing xy-SubFIlter or xy-VSFilter is the usual recommendation.

---

Thanks to your hints over in libass/libass#641 I was now able to actually build this PR and the current master state. This allowed me to confirm that the behaviour introduced in #24 is indeed what I expected and thus not conforming to ASS expectations:

Current master

After the revert (matching previous behaviour of pinterf/xy-VSFilter and current behaviour of Cyberbeing/xy-VSFilter, guliverkli(2)-VSFilter and libass):

Also see pinterf-incompat-demo.tar.gz for the sample file for the screenshots above and another anamorphic sample (for which storage resolution ≠ native display resolution).

[TheOneric]

Can you make binary?? Thanks

The incompatible change mentioned here, was added only after the last pinterf/xy-VSFilter release. So the last release binaries should still be safe to use. If you really want to get the other commits since the last release though, see the build artefact here: https:/TheOneric/xy-VSFilter/actions/runs/2899636750 for the binaries resulting from current pinterf master + this patch. (The artefact will eventually expire and no longer be available, I will not do any rebuilds when that happens.)

[Masaiki]

@TheOneric
The xy-VSFilter (master) here does display inconsistently with the others (before that pr and libass)
I'm not sure if this is a problem with that pr (if so, we should fix it rather than revert it) or if libass is simulating the wrong behavior from before
My opinion is that the subtitle renderer should focus on display (editing subtitle) to display (playing video with the subtitle), the storage size is sufficient and should be set by the player

A list of tags specified by the script indicating which tags are scalable might solve more problems

[TheOneric]

The xy-VSFilter (master) here does display inconsistently with the others (before that pr and libass)

Is “before that pr” above referring, to the current PR, #34, or the one introducing the bugs, #24? If the latter, i.e. there were already incompatibilities before #24, can you please elaborate on what they are?

I'm not sure if this is a problem with that pr (if so, we should fix it rather than revert it)

Here “that pr”, seems very unlikely not to refer to #24 to me.
#24 tries to "fix" #17, but the ""issue"" described in #17 is correct behaviour. Thus the only correct way to go about it, is to not do anything, or rather now that #24 was already merged to revert it. Quoting myself:

Again: it is required for conforming ASS renderers to display those tags differently depending on the video’s storage resolution.

If #24 also tries to do something else at the same time, please explain what and why, but either way #24 needs to be reverted first.

or if libass is simulating the wrong behavior from before

The previous behaviour of depending on the storage resolution is not wrong, it is the only correct behaviour for conforming softsub ASS renderers.

Just like, you wouldn't expect photos and videos (e.g. jpeg and MPEG-2) taken in 2002, to suddenly display different, distorted or no more content after upgrading your media viewers, softsub ASS renderers must not break existing real-world sub releases (i.e. ASS subtitles, all required fonts, video and optionally audio muxed into a multimedia container like MKV).

[pinterf]

(I'm watching the discussion here and at the libass repo but I don't have any live knowledge on subtitles so I cannot judge the correctness of a decision. When you, experts, reach a consensus, I'm gonna do whatever you propose)

[clsid2]

Do you have a similar demo without rotation? Because in that case the two scaling steps are correct, right?

So to be clear, in case of \frx and \frx, there should be no scaling from PlayRes to StorageRes/LayoutRes? Not because that is logical, but simply because that is how the renderers historically did it.

If I remember correctly, the fix in discussion above was done to fix a real-life sample. So it is really very unclear for subbers as well what is correct due to the lacking of an official specification.

[TheOneric]

So to be clear, in case of \frx and \frx, there should be no scaling from PlayRes to StorageRes/LayoutRes?

The perspective transform matrix involved in 3D-rotations is derived directly from storage resolution parameters, without considering PlayRes, so basically yes. Though, there's no easy way to get a multiplier for the tag’s values for this.

Not because that is logical, but simply because that is how the renderers historically did it.

With “simply” in the meaning akin of “naturally”, yes.
If it was intended in its other meaning of “merely”, I 'll have to protest, that intentionally breaking existing, real-world subtitle releases is no minor matter for softsub renderers. Precisely because SSA and ASS wasn't created as/from a specification but implemented directly as a renderer in “SubStation Alpha” (SSA, hardsub only) and VSFilter (ASS, softsub) and classic VSFilter was for a long time the only or only relevant implementation, authors did and do use and rely on all sorts of VSFilter quirks.

Do you have a similar demo without rotation? Because in that case the two scaling steps are correct, right?

Sorry, I'm not sure which two scaling steps you are referring to and if its after or before the current patch is merged. But, e.g. here is the issue which first made libass aware of the storage res stuff in 2009 and it refers to a sample where blur is noticeably different between ancient not-storage-aware libass and VSFilter, so for blur too real-world files rely on VSFilter’s storage res scaling. (The linked full sample is no longer available, but excerpts are posted and it might be possible to find the original release by the cited name)

I don't think #24 did directly affect blur and even if it indirectly did, just reverting the change as done here should resolve this again.

So it is really very unclear for subbers as well what is correct due to the lacking of an official specification.

From my experience sub authors just work with what they've got and that is a quirky VSFilter in Aegisub (or libass, but libass tries to faithfully emulate VSFilter and we maintain subs should always be checked in VSFIlter too before publishing as discrepancies usually result in libass adjusting to match VSFilter).
Ofc, MPC-HC/BE’s ISR differs, but ime there are virtually no releases specifically targeting those renderers (and there are no offical DLLs available for use in editors like Aegisub; the only ISR DLLs I know and only since recently, is an unofficial, since abandonded project from the same GH org the incompatible xy-* scaling change origianted from.).
If an incompatible change like this were to spread though, this would most likely cause confusion and result in a bunch of borked files.

If I remember correctly, the fix in discussion above was done to fix a real-life sample.

If with "fix" you mean the revert at hand, then yes.
If you meant #24, then it doesn't necessarily seem so to me an I can elaborate why if you wish. Even if it were, the change from #24 would still break a bunch of other real-world files so it’s not a good idea; especially not in xy-* when xy-* was historically known for ensuring compatibility. (But LayoutRes could help.)

[clsid2]

With two scaling steps I mean:

1. From PlayRes to StorageRes/LayoutRes
2. From StorageRes/LayoutRes to DisplayRes. Both ISR and XySubFilter can render the sub at the final display resolution of the video. So instead of scaling the subtitle bitmap, that scaling is instead done directly during sub rendering.

[TheOneric]

As explained in more detail here, \frx and \frz aren't the only tags not considering PlayRes. blur, be and if ScaledBorderAndShadow wasn't set to a valid and true value all border- and shadow-related tags also do not scale from PlayRes at all.
So in short, no, doing PlayRes → storage res → render res scaling is not always correct for non-rotation tags.

As for pinterf/xy-VSFilter, #24 only affected the perspective transform matrix and before #24 xy-VSFilter was fully compatible, so the revert here is sufficent to get correct behaviour again. Unless ofc ther are more merged incompatible changes I'm not aware of... (but even if this were the case, this revert would still be a necessary step)

---

@Masaiki do you still have questions about why the revert is required?

[Masaiki]

\be, \blur and the 3D distance to the screen plane (relevant for the perspective transform involved in \frx and \fry) are not scaled from PlayRes at all. (This is the bigger pain point that truly prompts the introduction of LayoutRes.)

@TheOneric #24 just want to solve this (frx fry not scaled), but you think #24 is not incompatible, right?
So why not solve the whole problem completely instead of just reverting #24
And compare libass (current master & with layoutres), xy-VSFilter (current master & #24 reverted and layoutres) in different resolutions (subtitles and video), the comparison result may be more convincing

[TheOneric]

@Masaiki, thanks for your reply.

#24 just want to solve this (frx fry not scaled), but you think #24 is not incompatible, right?

Yes, #24 it is without a doubt incompatible with classic ASS and breaking existing real-world subtitle releases.

So why not solve the whole problem completely instead of just reverting #24

There is no solution without a revert of #24, as #24 is plain wrong.

As an additional discussion, separate from unbreaking xy-*, libass/libass#641 proposes LayoutRes as a new, retroactive format extension enabling subtitle authors to create new files not depending on video storage resolution. And also allow users and authors creating derivatives of existing files to easily remove this external factor. Crucially, unedited old files (i.e. files without valid LayoutRes headers) will retain the correct, pre-#24 behaviour of using video storage resolution; this must be done to not break subtitle releases. Therefore, even with LayoutRes, #24 still needs to be reverted. (And if LayoutRes were to be rejected, the same would be true, as softsub renderers must at least not break existing releases. Reverting #24 is thus necessary, regardless of how LayoutRes turns out)

[Masaiki]

#24 is plain wrong.

I don't think so, #24 was created for multi-resolution distribution and is compatible with the VSFilter in MPC-BE/HC.

By the way, if you think you are right and want to be the new standard and try to convince others, then at least you should have a standard, complete documentation

Since I am fully aware that we are all stubborn on this topic, I won't make any more comments under this PR, do whatever you want.

[TheOneric]

[#24] is compatible with the VSFilter in MPC-BE/HC

See my prior comments on this. Also, if the goal were compatibility with ISR while disregarding all existing releases, then #24 won't suffice and all it achieves is creating a third rendering target different from both MPC-HC/BE ISR and also classic ASS (as implemented in e.g. xy-* releases, guliverkli(2) and mostly libass).

By the way, if you think you are right and want to be the new standard

There's nothing “new” about depending on storage resolution; this has been a property of ASS ever since the original guliverkli-VSFilter, and as mentioned numerous times, real-world subtitle releases do depend on this and PlayRes* can have any arbitrary value. E.g. to give just one example: prior to floating point font sizes etc being supported in xy-*, libass, ThreadedVSF, etc (but still not supported in ISR), using a scaled up PlayRes* was a not-unusual way to emulate them (this technique is mentioned in e.g. Aegisub's blog). Any such file also containing one of the non-PlayRes-aware tags is broken by #24.

It may very well be the fault of lacking communication skills on my end, but unfortunately I do not know how to make the breakages #24 causes and why it needs to be reverted any more clear than I already did. The essential point is, that it breaks existing real-world subtitle releases and breaks with longstanding ASS semantics. (And as an additional point, it breaks with the expectation of xy-* being fully backwards-compatible which is a renowned charateristic of xy-*)

I won't make any more comments under this PR, do whatever you want.

I regret, that I could not better illustrate the reasoning to you to get to a common understanding, but as mentioned I unfortunately don't know how to.

@pinterf: has the reason (breaking existing releases) become clear to you or should I ask someone else, with likely better explaining skills, to illustrate why the revert is necessary?

[clsid2]

I agree that having proper documentation is essential. Just copy any existing one, add it to the libass project, and add clear descriptions of how things should be scaled, plus any other quirks that may exist in the original VSFilter (which implicitly dictates the standard).

[TheOneric]

The documentation bit seems to better fit into libass#641 imho, best repost it there then I or someoneelse can reply. This PR is just about fixing the recent breakage and for xy-* specifically I guess a rough guideline is: “if a patch changes rendering results, it is most likely wrong” (crashing is not a valid result) since xy-* is both directly derived from the original implementation and historically took care to stay compatible.

[TheOneric]

@pinterf: can you merge this? Nobody seems to want to block this anymore and the revert itself is a blocker for a LayoutRes{X,Y} implementation as without reverting the backwards-incompatible change rotations would never use LayoutRes{X,Y}.

---

Although I personally still think maintaining compatibility by leaving the storage res reliance for non-LayoutRes files is the right thing to do, reverting this and then adding LayoutRes{X,Y} is also beneficial for those who believe there should not be such a reliance.
The patch being reverted here, doesn't actually fully remove the influence of storage resolution. It supplants storage res by PlayRes only for 3D rotations; \blur, \be and all borders and shadows if no ScaledBorderAndShadow: yes was set continue to rely on and scale with storage resolution.
Once the LayoutRes{X,Y} patches are merged though, it would be possible to globally supplant storage res by PlayRes (if there are no valid LayoutRes headers) by changing just a few lines in one function.

Thus moving along with this revert and then LayoutRes is a good idea regardless of what one thinks about storage res and we can continue to discuss this after LayoutRes went through.
I also since remembered that XySubFilter (but not xy-VSFilter iinm) has user config options. Making the use of storage res or PlayRes for LayoutRes-less files a user options would be a possible compromise. (But let's wait with this after the revert and LayoutRes are dealt with.)

[TheOneric]

@pinterf, friendly ping.

[pinterf]

Huge sorry for my delayed reaction. So the last word from Masaiki is that he won't comment this PR any more, though he's not convinced. The debate origins mainly from the fact that this application area was not properly documented?
I wish this repo was maintained by someone with more interest, knowledge and time.
A question: O.K. this PR (revert) unblocks the possibility of a future fix. But who will do this LayoutRes fix, or it is not a fix but a future addition for supporting a non-documented quasi-standard?

[TheOneric]

@pinterf :

But who will do this LayoutRes fix

I have a LayoutRes{X,Y} PR ready to go as soon as the revert is merged and already tested the resulting pinterf-xy-VSFilter binary (build here). The patch is basically the same as the one for Cyberbeing/xy-VSFilter PR#18 apart from some whitespace changes, but Cyberbeing appears to be inactive.

The debate origins mainly from the fact that this application area was not properly documented?

The debate comes from whether it is worth keeping backwards compatibility with a stupid, but well-known and intentional preserved during XySubFilter creation (and in libass) property of ASS, namely depending on video storage res to render some things.

or it is not a fix but a future addition

LayoutRes{X,Y} is an addition to ASS, which allows to remove all dependencies on the video storage resolution in backwards-compatible way by specifying the new headers. See libass/libass#641 for details. (As soon as LayoutRes{X,Y} is merged here, I'll also merge the LayoutRes{X,Y} implementation in libass.)

The patch being reverted aimed at removing the existing (and intentionally preserved) dependency on storage res by substituting it by PlayRes. However, it actually is incomplete and only did this for 3D rotations; blur and some other features remained dependent on storage res (likely by oversight). Once the LayoutRes{X,Y} patches are merged, it would be just a simple patch of a few lines in just one function in order to globally use PlayRes{X,Y} instead of video storage res if no LayoutRes{X,Y} headers were defined. Thus enabling achieving this goal fully and more simple.
I continue to believe breaking compatibility is a bad idea, but I recently remembered XySubFilter has user config options. So we could please everyone, by just making this choice between video storage res or PlayRes{X,Y} for non-LayoutRes{X,Y} files selectable by such an option. (But this requires LayoutRes{X,Y} which in turn requires this revert first)