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

Drop custom brotli decompression #151

Closed
TheOneric opened this issue Jul 10, 2022 · 7 comments
Closed

Drop custom brotli decompression #151

TheOneric opened this issue Jul 10, 2022 · 7 comments
Labels

Comments

@TheOneric
Copy link
Member

While 128be61 didn't affect the “Brotli Compressed Subtitles” test, as it turns out it causes issues with more complex brotli-compressed subs like the Railgun S or SnK openings. The former only renders the first karaoke line and nothing afterwards, the latter only shows the colour blobs of the first karaoke line and nothing else. Sorry for not noticing earlier. As a – hopefully temporary – hotfix I reverted the change.

@WeebDataHoarder: do you want to look into this and try to find a fix so it can be reapplied?

@TheOneric TheOneric added the bug label Jul 10, 2022
@WeebDataHoarder
Copy link
Contributor

I believe I have a patch ready for this, but it involves bringing along custom TextDecoder polyfill (which is what brotli.js did on their own).

@TheOneric
Copy link
Member Author

Great! Needing one more polyfill for old engines seems unproblematic. considering we basically already have this and a license-compatible polyfill is available.
Can you explain though what the problem is and why we need to deal with text encodings for brotli subs? The compressed version is opaque binary data and given libass never receives an encoding parameter from JSO, the uncompressed version must always be UTF-8 to be understood.

@TheOneric
Copy link
Member Author

TheOneric commented Jul 29, 2022

@WeebDataHoarder: Looking at the wip(?) commit you pushed to the “lazy font” PR, there's a comment about “future emscripten versions” adding the required polyfill on their own. Did you notice emscripten was updated to 3.1.15 (back then the most recent version, now it’s 3.1.17) before this issue was opened or is this about intended/post-3.1.15 emscripten changes?

@WeebDataHoarder
Copy link
Contributor

WeebDataHoarder commented Jul 29, 2022 via email

@TheOneric
Copy link
Member Author

fyi, I just tested master + 128be61 + WeebDataHoarder@35a9216 and the same issue as before still persists. It appears TextDecoder wasn't the only problem.

@TheOneric
Copy link
Member Author

I tentatively deprecated in-house brotli decompression in e459c8e recommending transparent (de)compression via Content-Encoding to be used instead. If no one complains that Content-Encoding was unusable on some platform, it'll eventually be removed completely and the issues with moving to WASM-brotli will be obsolete.

@TheOneric TheOneric changed the title WASM brotli broke some compressed subs Drop custom brotli decompression Dec 4, 2022
@TheOneric
Copy link
Member Author

After the release, manual decompression was now dropped in c975a29.
If this should prove problematic this can be reverted, otherwise the manual decompression headaches and confusions are now gone for good and it won't interfere with the more flexible Content-Encoding: anymore.


For those who came here in search what to use instead, here’s the relevant bit from 4.1.0’s README:

Brotli Compressed Subtitles (DEPRECATED)

Manual support for brotli-compressed subtitles is tentatively deprecated
and may be removed with the next release.

Instead use HTTP's Content-Encoding: header to transmit files compressed and
let the browser handle decompression before it reaches JSO. This supports more
compression algorithms and is likely faster.
Do not use a .br file extension if you use Content-Ecoding: as this will
conflict with the still existing manual support which tries to decompress any data
with a .br extension.

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

No branches or pull requests

2 participants