JavascriptSubtitlesOctopus icon indicating copy to clipboard operation
JavascriptSubtitlesOctopus copied to clipboard

WASM brotli broke some compressed subs

Open TheOneric opened this issue 3 years ago • 5 comments

While 128be61fe30ec9039192c6dda91c714ef03051e7 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 avatar Jul 10 '22 15:07 TheOneric

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).

WeebDataHoarder avatar Jul 15 '22 16:07 WeebDataHoarder

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 avatar Jul 16 '22 00:07 TheOneric

@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?

TheOneric avatar Jul 29 '22 21:07 TheOneric

This was an update done mainly on my side to see how the new versions behaved, haven't checked recent updates lately. ------- Original Message ------- On Friday, July 29th, 2022 at 11:23 PM, TheOneric @.***> wrote:

@.***(https://github.com/WeebDataHoarder): Looking at the wip(?) commit you pushed to the “lazy font” PR, there's a not 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?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

WeebDataHoarder avatar Jul 29 '22 21:07 WeebDataHoarder

fyi, I just tested master + 128be61fe30ec9039192c6dda91c714ef03051e7 + https://github.com/WeebDataHoarder/JavascriptSubtitlesOctopus/commit/35a92168cd1acb30ca1fc9330f6fa81934305265 and the same issue as before still persists. It appears TextDecoder wasn't the only problem.

TheOneric avatar Sep 10 '22 21:09 TheOneric

I tentatively deprecated in-house brotli decompression in e459c8e6ffa83f2244f9745f54c39a19edd9684f 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 avatar Oct 07 '22 20:10 TheOneric

After the release, manual decompression was now dropped in c975a29e69655a05e3e8f1de05d3725a00e61d33. 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.

TheOneric avatar Dec 04 '22 23:12 TheOneric