Comments (15)
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
from libjass.
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.
from libjass.
@DoomTay : correct, that's the one :)
from libjass.
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);
}
}
from libjass.
@ownerer the usual procedure is to fork the repository, create a branch in your fork and submit that with a PR.
from libjass.
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:
- parse ASS file into ASS object
- return object to whoever requested it
- requester sets resolution properties
- 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?
from libjass.
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.
from libjass.
Well for this specific issue, I would suggest the following:
- make the setting of the resolution properties public/the responsibility of the requester
- 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 totrue
when whatever logic decides aScript Info
section was found and assert the flag must betrue
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.
from libjass.
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 aScript 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
.
from libjass.
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:
- Both X and Y are set
- Only Y is set
- 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...
from libjass.
Is this on the todo list then?
All open issues, including this one, are implicitly in an "Accepting PRs" state,
from libjass.
All open issues, including this one, are implicitly in an "Accepting PRs" state,
Which means...? I should make the change and PR it?
from libjass.
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.
from libjass.
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.
from libjass.
Per @Arnavion moving my question to new issue: #101
from libjass.
Related Issues (20)
- When PlayResX or PlayResY are defined as zero, tab crashes. HOT 1
- Unexpected style margin collisions HOT 1
- \n being rendered in plain text, wrapping style 2 HOT 2
- Request for new release HOT 2
- How to crate subtitle from JSON Object ? HOT 2
- libjass.deserialize dialogue style changes not work HOT 2
- When playing preroll ads, sometimes the text appears very small HOT 1
- Why the npm package doesn't contains a type definition file. HOT 1
- Line-specific styles do not override style definitions HOT 2
- \fad with a fadeout of 0 is treated as fading out for the duration of the line HOT 1
- Opt-in support for command execution HOT 3
- [Feature Request]Can you convert a .ass subtitle to VobSub format? HOT 2
- dialogues ID will be created next to the last dialogues ID from previous object HOT 3
- when I create new dialogue can I get a dialogue class tag? HOT 1
- Update Typescript version HOT 6
- Serialize back to .ass HOT 1
- implement smart line wrapping
- Lines can be "pushed away" by other lines on the same layer HOT 2
- Lines are moved down when lines underneath it are removed HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from libjass.