transforms with clips are not parsed correctly

[line0]

While this correctly transforms clip and color over the duration of the line

{\c&HFFFFFF&\clip(0,0,1000,1000)\t(\c&H000000&\clip(0,0,0,0))}Test

this only transforms the clip and the color is black for the whole duration of the line:

{\c&HFFFFFF&\clip(0,0,1000,1000)\t(\clip(0,0,0,0)\c&H000000&)}Test

@torque suspects the cause of the issue to be the parser stopping after the first ) is encountered and subsequentally treating the \c as a regular override tag.

This bug also affects xy-vsfilter, but not ISR.

[rcombs]

If it also happens in VSFilter, then this is a bit of format stupidity, but not a libass bug. ASSv5 should improve this behavior.

[line0]

Considering how ASSv5 isn't happening anytime in the foreseeable future, you might want to reconsider blindly replicating each and every vsfilter bug, no matter how unreasonable it is to rely on them (and we currently know of exactly 0 scripts that rely on this one). As you know, xy-vsfilter is dead and ISR may become the de facto gold standard quite soon.

[rcombs]

Reopening because there seems to be some interest in fixing this everywhere:

 <Cyberbeing> @rcombs about line0's issue I'd say to just ask astiob if he's willing to come up with a sensible fix for both libass & vsfilter which ensures behavior is identical
 <Cyberbeing> who knows, I mean it seems something like this would be as likely to fix scripts as it would break them
 <Cyberbeing> I'd just defer to whatever astiob thinks in terms of Issue #151

[Cyberbeing]

Looking back, it seems this issue was fixed in MPC-HC 1.4 when they did their initial merge from VSFilterMOD r20 , seemingly as a side-effect of the following diff which added support for complex tags with \t:

Libass and xy-VSFilter currently behave like VSFilter 2.39 (guliverkli2) in terms of this issue, which is somewhat expected. As mentioned on IRC, I'm open to the idea of seeing this parser quirk fixed, but I'm interested in hearing the opinion of @astiob about potential impact and side-effects and will defer to his opinion. Unfortunately since xy-VSFilter currently does not have an active dev, we would require a pull request if Libass decides to fix this and maintaining unified parsing behavior across renderers is desired.

[ghost]

Don't you love compatibility with ancient vsfilter? What the fuck are you complaining about?

[astiob]

I appreciate the trust! To be honest, as with all compatibility issues, I’m somewhat conflicted, but currently I’m inclined to say this should stay the backwards-compatible way.

---

I’ve actually been meaning to fix this in MPC-HC to restore backwards compatibility ever since I fixed it in libass as part of #94. I’m sorry I haven’t yet; any bad things you may think about me in this regard are probably true.

As @Cyberbeing mentioned, nested parenthesis support—for all tags, not just \t—was added in VSFilterMod r11, somewhat broken at first, then fixed in r17. VSFilterMod was then imported into MPC-HC wholesale, only to stay disabled and be removed from the codebase two years later.

Unfortunately, the removal wasn’t a complete, automatic revert, so two pieces of code that affected compatibility but weren’t explicitly marked as such were left over: one is the nested parenthesis support, while the other is &0xFF for alpha override values, which eventually caused xy-VSFilter issue 80 to be reported when Doki abused it.

---

Now, I want to bring everyone’s attention to this little detail: note that I said “for all tags, not just \t”. For example, consider \org((). guliverkli2, xy-VSFilter and libass parse this is as a complete override tag and proceed to throw it away, because it is a pretty rubbish tag. VSFilterMod, on the other hand, sees an open parenthesis that hasn’t been closed and keeps reading arguments for \org until it happens upon a closing parenthesis. Are you sure you want this behaviour?

(This affects unknown tags as well, except in MPC-HC starting from version 1.7.1.308, which introduced a (probably) accidental breaking change that sees unknown tags skipped before the arguments are parsed.)

Also, the nested parentheses that VSFilterMod recognizes can span multiple arguments and generally don’t affect argument splitting. So, \t((0,0),\bord5), \t((0,)3,\bord5), \t(0,(3,\bord5),0) and \t(0,0,(\bord1)(5)) are all equivalent to \t(0,0,\bord5), except the last one is \t(0,0,\bord1) in MPC-HC 1.7.1.308+ due to another separate breaking change (but \t(0,0,(\pos1)(7,8)) is \t(0,0,\pos(7,8))). In guliverkli2, these overrides mean respectively \t(0,0), \t(0), \t(0,0,\bord5) and \t(0,0,\bord1) (and \t(0,0,\pos(7,8))).

Of course, it is also possible to change all renderers, including MPC-HC, to support nested parentheses but in a more intuitive manner than this and optionally only in the last argument of \t.

---

we currently know of exactly 0 scripts that rely on this one

Alas, we’ve seen again and again that “having seen 0 scripts abusing the quirk so far” isn’t an indication that can be trusted. There’s a huge lot of scripts out there, and many of them are broken. We keep bumping into more and more bizarre compatibility issues, and I won’t be surprised in the slightest when we finally spot a script that relies on this.

(While I’m at this, let me just point out that when we, the English-speaking community, look for broken scripts, we tend to do it only in anime releases with English subtitles, but ASS is used much more widely than that.)

it seems something like this would be as likely to fix scripts as it would break them

Scripts that were authored for VSFilterMod-flavoured MPC-HC… yes, I suppose. Then again, you could say this about any renderer quirk. But then, I do agree that this one makes more sense the backwards-incompatible way, so it’s tempting to say it’s more likely to be used that way in practice than other quirks.

It is precisely these potential benefits and the lack of confirmed uses of the traditional parsing that make me waver, but I think doing this might just be too risky.

ask astiob if he's willing to come up with a sensible fix for both libass & vsfilter which ensures behavior is identical

I think it would be pretty easy to make libass and xy-VSFilter to behave like VSFilterMod, and it probably wouldn’t be too hard to make all renderers do it only for \t if that option seems more appealing (I think it’s trivial for libass but more involved for VSFilter). A more sensible parsing might be harder. I’m willing to (try to) implement any of the options everywhere if the consensus is that supporting nested parentheses is the way to go. I’m just not convinced personally that it is.

Considering how ASSv5 isn't happening anytime in the foreseeable future, you might want to reconsider blindly replicating each and every vsfilter bug, no matter how unreasonable it is to rely on them

A while ago, @wm4 publicly suggested doing the only thing that would allow us to keep perfect compatibility with old scripts while making new scripts sane, and it’s nothing ASS hasn’t seen before: a new Boolean header. Like we have “ScaledBorderAndShadow”, have something like “IntuitiveSyntaxAndBehavior” (name subject to change). Then we could fix the few most inconvenient or problematic parser and renderer quirks (which are easy to fix in all renderers), while keeping everything else as is. Sort of like standards-mode HTML4 to quirks-mode HTML4. If I recall correctly, @Cyberbeing strongly opposed this idea.

[Cyberbeing]

What the fuck are you complaining about?

@wm4 if that comment was in reply to me, I'm not complaining about anything and would be perfectly fine with no change. I just felt this issue should be properly discussed before assuming all parties involved were opposed by default.

it probably wouldn’t be too hard to make all renderers do it only for \t if that option seems more appealing

If something was done, I do think it would make most sense to try to restrict it to this particular testcase of \clip breaking additional transforms when not placed as the last argument of \t, if it were possible to not affect the behavior of other tags and tag transforms in the process.

Scripts that were authored for VSFilterMod-flavoured MPC-HC… yes, I suppose.

At this point, I'm of opinion the any examples of this in circulation are more likely than not to be accidental result of scripting or manual editing since it is non-obvious nor documented that \clip must be the last argument in a \t tag if you require additional tag transforms. If we naively assume that valid overrides in \t are always desired by the script author to be used if added, then not functioning because of incorrect ordering is unexpected. I have no doubt this would result in breakage to some degree if fixed, but is the worse case scenario severe enough to be concerned? I don't have the answer to that.

If I recall correctly, @Cyberbeing strongly opposed this idea.

The only thing I remember off-hand was @wm4 desire for “ScaledBorderAndShadow” to also include Blur scaling by default rather than maintain compatibility with VSFilter layout behavior. In general, I am opposed ideas changing the behavior of how PlayResX/Y layout is handled, or any other changes introducing new tags/behavior/functionality which would yield horribly incorrect output when encountered by existing renderers. Any major compatibility breaking changes should be restricted to a new subtitle format like the ASSv5 being discussed, which will break backwards compatibility existing ASS renderers by design.

Fixing minor parsing bugs such as line0's example in retrospect don't seem too bad, as long as the fix does not enable new tag behavior which was not previously possible. Though from the examples @astiob gave, matching the current MPC-HC/VSFilterMod behavior exactly seems like a bad idea without greatly reducing the scope.

[astiob]

I'm of opinion the any examples of this in circulation are more likely than not to be accidental result of scripting or manual editing

Well, I would just like to hope that script authors actually test their scripts with at least one renderer. If this renderer is anything but MPC-HC (or VSFilterMod or possibly old libass), then they’ll see that \t with nested parentheses doesn’t work and presumably edit the script to work around it. So the only scripts that expect nested parentheses to work should be those that were tested with only MPC-HC.

Not that this per se means those scripts can be dismissed, but I think, statistically, they should form a small minority.

[Cyberbeing]

Not that this per se means those scripts can be dismissed, but I think, statistically, they should form a small minority.

This was actually what I was getting at, since I am only considering possibilities for scripts breaking after fixing this rather than working.

The case of existing script which may have been VSFilterMod/MPC-HC targeted are not a concern, since they would start functioning to some extent if this issue was resolved. If there were few complaints that such scripts were broken, there should be even less when they are fixed.

The more concerning possibilities are when VSFilterMod/MPC-HC was not targeted, and this was done by accident. If such scripts were targeting VSFilter 2.39 behavior and never fixed, you would assume these transforms being broken would be minor. Yet it could also mean that that broken tag behavior was actually the intended effect, and would actually break in some strange way when this was fixed.

Along this line of thought, you would need to consider future scripts which intentionally conform to the fixed behavior. Is the potential for breaking tags which could be paired with a rectangular \clip transform serious enough to be concerned about in terms of backwards compatibility? @line0 do you have an opinion on this? How significantly would your existing scripts utilizing transformed \clip break, if you manually reproduced this issue?

@astiob I'll leave the decision to you at this point. My stance now and going forward will remain more open to any compatibility breaking changes which may be desired by consensus for Libass. As long as someone is willing to create a pull request for xy-VSFilter so unified behavior can be maintained, I'll have no complains. Without an active developer for the past 6+ months, we no longer have the means to match any changes in behavior even if we desired to.

[line0]

I'd say the chances of a fix breaking or fixing existing scripts is negligible. it's highly unlikely that anyone would accidentally insert a clip into a transform which would be the prerequisite for relying on the bug. It's also not very likely that fixing the bug would fix any existing scripts, because (a) stuff made with VSFilterMod in mind is usually hardsubbed and (b) if the transform didn't work as expected, the typesetter/kara guy would usually take steps to get the desired behavior (e.g. by reordering tags or using separate transforms).
I personally don't have any scripts that rely on the bug or break on it, however i do have a tag modification module for Aegisub that would have to make sure to always output tags in the bug-free order. Incidentally, a (hypothetical) broken macro could the one thing that might throw off our predictions about a fix not breaking existing scripts.

As for backwards-compatibility of scripts authored using fixed versions of libass and xy-vsfilter with older versions of the renderers - yeah that certainly could be a problem. Transforming clips is exceedingly rare in typesetting, but some karaoke makes use of it. The tags that could follow a clip in this situation are most likely scale, color and maybe alpha, so I'd expect the breakage to be mild to moderate. Using a header flag is obviously useless in this case. However, we don't usually have any qualms forcing new versions of players, renderers and filters down the throats of the users, so i see no point in caring about that now.