"PlayResX" is not treated as optional! -> breaking

[ownerer]

Hi,

today I uncovered what I would categorize as a bug.
The Emby media server project uses your library to render subtitles without having to resort to transcoding.
However the bulk of my library failed to play.
Turns out that was because your library says my subtitles are malformed.
Emby has a fallback system in place that didn't work and should be fixed with the next release, so that's good.
However I want to tackle the root of this problem, which lies with you.
You can view the relevant part of the thread that finally lead to uncovering this here.

This is my subtitle's script info section:

[Script Info]
Title:
Original Script:
Script Type: v4.00+
Collisions: 0
WrapStyle: 0
PlayResY: 2160
PlayDepth: 32
Timer: 100

Note there is only the "PlayResY" property, no "PlayResX".

This is the piece of code (beautified, I didn't bother with the source) from your library that throws the malformed error:

if (null !== this.y && (this.f.addAttachment(this.y), this.y = null), this.v === n.ScriptInfo && e !== n.ScriptInfo && this.d.resolve(this.f), e === n.EOF) {
    var t = this.f.properties;
    void 0 === t.resolutionX || void 0 === t.resolutionY ? (this.d.reject("Malformed ASS script."), this._.reject("Malformed ASS script.")) : (this.d.resolve(this.f), this._.resolve(this.f))
}

Note that if either "resolutionX" or "resolutionY" is undefined, the error is thrown.
So it makes sense my subtitles fail, since the "PlayResX" property is missing in the script info section.

However, the way I see it, your library is not following the Substation Alpha specs!
I quote the wiki:

PlaResX & PlayResY (useful for graphics and subtitle positioning. If only one present, the other is calculated with real video proportions)

Clearly this library does not calculate the other if only one is present, it just breaks.

Straight from matroska.org:

The first, "[Script Info]" contains some information about the subtitle file, such as it's title, who created it, type of script and a very important one : "PlayResY". Be carefull of this value, everything in your script (font size, positioning) is scaled by it. Sub Station Alpha uses your desktops Y resolution to write this value, so if a friend with a large monitor and a high screen resolution gives you an edited script, you can mess everything up by saving the script in SSA with your low-cost monitor.

They explicitly talk about "PlayResY", but never about "PlayResX", the examples also never include it.

This leads me to conclude that it is in fact a bug in your library as you do not respect the standard.

I temporarily fixed my problem by just hardcoding the required calculation myself based on a 16:9 aspect ratio when only one of X and Y is given.
Obviously this will be overwritten any time a new version is released.

I would suggest at the very least implementing what I already hardcoded for now:
When only one is given, calculate the other based on a 16:9 aspect ratio, or maybe the monitor's aspect ratio, whatever, but DON'T let it break.
Alternatively maybe you could accept optional parameters allowing external libraries to pass in the desired resolution or aspect ratio from the video file that is being rendered.

Thoughts?

[DoomTay]

If it helps any, here's what seems to be the unminified part of the source, based on searching for the error message

libjass/src/parser/stream-parsers.ts

Lines 91 to 116 in e0c6300

	private set currentSection(value: Section) {
	if (this._currentAttachment !== null) {
	this._ass.addAttachment(this._currentAttachment);
	this._currentAttachment = null;
	}
	if (this._currentSection === Section.ScriptInfo && value !== Section.ScriptInfo) {
	// Exiting script info section
	this._minimalDeferred.resolve(this._ass);
	}
	if (value === Section.EOF) {
	const scriptProperties = this._ass.properties;
	if (scriptProperties.resolutionX === undefined || scriptProperties.resolutionY === undefined) {
	// Malformed script.
	this._minimalDeferred.reject("Malformed ASS script.");
	this._deferred.reject("Malformed ASS script.");
	}
	else {
	this._minimalDeferred.resolve(this._ass);
	this._deferred.resolve(this._ass);
	}
	}
	this._currentSection = value;
	}

[ownerer]

@DoomTay : correct, that's the one :)

[ownerer]

I don't have any rights to create a new branch for a suggested fix, so here is the code to replace the "if" on current line 102:

if (value === Section.EOF) {
			const scriptProperties = this._ass.properties;
			if (scriptProperties.resolutionX !== undefined && scriptProperties.resolutionY === undefined) {
				scriptProperties.resolutionY = Math.floor(scriptProperties.resolutionX / 16 * 9);
			} else if(scriptProperties.resolutionX === undefined && scriptProperties.resolutionY !== undefined) {
				scriptProperties.resolutionX = Math.floor(scriptProperties.resolutionY / 9 * 16);
			} else if (scriptProperties.resolutionX === undefined && scriptProperties.resolutionY === undefined) {
				// Malformed script.
				this._minimalDeferred.reject("Malformed ASS script.");
				this._deferred.reject("Malformed ASS script.");
			}
			else {
				this._minimalDeferred.resolve(this._ass);
				this._deferred.resolve(this._ass);
			}
		}

[FichteFoll]

@ownerer the usual procedure is to fork the repository, create a branch in your fork and submit that with a PR.

[Arnavion]

However, the way I see it, your library is not following the Substation Alpha specs!

This leads me to conclude that it is in fact a bug in your library as you do not respect the standard.

Just to be clear, the closest thing to an ASS "spec" or "standard" is the doc file (linked in the matroska page) that is incomplete in general, let alone that it doesn't say anything about the optionality of PlayResX / PlayResY.

I quote the wiki:

The Wikipedia page is presumably talking about how it's implemented in existing renderers, so it's more de facto than de jure. (That's fine. I'm just saying it just isn't as clearcut as "violating the standards".)

When only one is given, calculate the other based on a 16:9 aspect ratio, or maybe the monitor's aspect ratio, whatever, but DON'T let it break.

Assuming 16/9 is not ideal. Monitors need not exist.

Alternatively maybe you could accept optional parameters allowing external libraries to pass in the desired resolution or aspect ratio from the video file that is being rendered.

Video metadata need not exist at the time the ASS is parsed - the demo page (and I suspect users in general) intentionally exploits this by loading both in parallel. It's not impossible to make video metadata a dependency, just not ideal.

---

I think a better fix will be to remove that assert that ScriptProperties must have resolutionX and resolutionY set when the Script Info section is done being parsed. Rather the caller who receives the ASS object should check populate them if necessary (or fail) before passing them to a renderer.

[ownerer]

Sure, I hear you on the whole standard thing, perhaps my choice of words was a bit harsh, fair enough.

So essentially you're saying the flow would change to:

1. parse ASS file into ASS object
2. return object to whoever requested it
3. requester sets resolution properties
4. pass to renderer?

The outcome of that would pretty much come down to the same thing I was suggesting, without making the video metadata a dependency at parse-time.

Did I get that right?

[Arnavion]

Yeah, exactly. The requester in your case is the Emby library.

---

Also, the assert was introduced in this commit that actually tried to catch scripts with no Script Info section. It would be good to check if this check is not required at all (ie scripts without Script Info are accepted by existing renderers just fine) or if it needs to be tweaked to detect that something in the section was specified, just not PlayResX or PlayResY.

Note that it needs to continue to handle the case where the [Script Info] line isn't is missing - I remember there was a commit to assume a header-less section at the start of the script is a Script Info section.

[ownerer]

Well for this specific issue, I would suggest the following:

1. make the setting of the resolution properties public/the responsibility of the requester
2. change the assert to check for any property on the ASS object properties instead of checking for the resolution properties specifically, or better yet: add an extra flag in the parser that's set to true when whatever logic decides a Script Info section was found and assert the flag must be true or otherwise the script is malformed. (checking all properties in the properties object isn't very scalable after all...)

This way any existing implementation would continue to work as long as the resolution properties are externally set. Ie, the parser would still say a script is malformed when no Script Info section is found, which is essentially what the current assert comes down to...

Whether or not the assert is needed at all is definitely a valid question going on the information you mentioned, but could/should be treated as a separate issue in my opinion.

[Arnavion]

make the setting of the resolution properties public/the responsibility of the requester

That is already the case.

change the assert to check for any property on the ASS object properties instead of checking for the resolution properties specifically

ASS.properties is always initialized to a ScriptProperties. To make it otherwise would require threading more state through the stream parser.

add an extra flag in the parser that's set to true when whatever logic decides a Script Info section was found and assert the flag must be true or otherwise the script is malformed.

Yes, that is what I was talking about, and why I mentioned the fix needs to be robust against the case where there is no section named Script Info.

[ownerer]

That is already the case.

Oh ok, my bad, I really haven't been very awake these last couple of days, I seem to be missing a lot of things :')

Well in that case I guess it really does just come down to adding that flag in the parser class?
Is this on the todo list then?

I took the liberty to test Emby in my case with 3 scenarios:

1. Both X and Y are set
2. Only Y is set
3. Neither is set

This to test your remark:

It would be good to check if this check is not required at all (ie scripts without Script Info are accepted by existing renderers just fine)

You can find the screenshots here.
In Emby's case it does not handle the lack of one or both properties.
This is of course not your problem, but I just wanted to point out that not all renderers will handle this properly.

So I don't really feel like changing the assert is a breaking change (ie: subs are still rendered), but it should definitely be communicated very clearly on the next release.
Also note I can only speak for my findings with Emby of course, who knows what other renderers may or may not do...

[Arnavion]

Is this on the todo list then?

All open issues, including this one, are implicitly in an "Accepting PRs" state,

[ownerer]

All open issues, including this one, are implicitly in an "Accepting PRs" state,

Which means...? I should make the change and PR it?

[joshuabrown-ellation]

Should this library be hardened against .ass subtitle files that have zero for PlayerResX? Would that also be a desired enhancement? @Arnavion @ownerer? Seems like it's related to this topic, or should I create another issue? The break it causes is dramatic, heats up computers, locks browsers, in some cases crashes the user computer. To be fair, we shouldn't be allowing people to send us busted .ass files anyhow, but that's a different issue.

[Arnavion]

It gets into an infinite loop, I assume? Better to make another issue with a repro. The fix may or may not be the same as this one - depends on how (or if) vsfilter / libass handle it.

[joshuabrown-ellation]

Per @Arnavion moving my question to new issue: #101