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

Remove unbrotli.js and use brotli submodule javascript decompressor #107

Merged
merged 2 commits into from
Aug 3, 2021

Conversation

TFSThiagoBR98
Copy link
Collaborator

As mentioned in #105 (comment), this PR replace the unbrotli.js with the Brotli Javascript decompressor from the submodule.

In the future, a decompressor may be implemented in Wasm instead of using Javascript.

@TheOneric, can you review this?

Copy link
Member

@TheOneric TheOneric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two unbrotli.js commits should be squashed before merge. Apart from a few remarks regarding the Makefile, the diff looks good to me. I assume you verified that this is a working drop-in replacements (apart from the Module patch). Is there an easy, ideally automated way to test JSO changes beyond mere compilation, btw?

@TheOneric TheOneric force-pushed the brotli branch 2 times, most recently from 6f49973 to 253ea56 Compare July 31, 2021 17:58
@TheOneric
Copy link
Member

TheOneric commented Jul 31, 2021

I've taken the liberty to push the changes I suggested myself, @TFSThiagoBR98 does it look alright to you?

A new commit was added for the patchfile dependencies and the diff between your previous version and the first commit of the new version is this:

diff --git a/Makefile b/Makefile
index ee90824..ace025a 100644
--- a/Makefile
+++ b/Makefile
@@ -70,7 +70,6 @@ $(DIST_DIR)/lib/libexpat.a: build/lib/expat/configured
        emmake make install
 
+build/lib/brotli/js/decode.js: build/lib/brotli/configured
 build/lib/brotli/configured: lib/brotli
-       mkdir -p build/lib/brotli
        rm -rf build/lib/brotli
        cp -r lib/brotli build/lib/brotli
        $(foreach file, $(wildcard $(BASE_DIR)build/patches/brotli/*.patch), patch -d "$(BASE_DIR)build/lib/brotli" -Np1 -i $(file) && ) true
@@ -305,7 +304,7 @@ EMCC_COMMON_ARGS = \
 
 dist: src/subtitles-octopus-worker.bc dist/js/subtitles-octopus-worker.js dist/js/subtitles-octopus-worker-legacy.js dist/js/subtitles-octopus.js
 
-dist/js/subtitles-octopus-worker.js: src/subtitles-octopus-worker.bc src/pre-worker.js src/SubOctpInterface.js src/post-worker.js
+dist/js/subtitles-octopus-worker.js: src/subtitles-octopus-worker.bc src/pre-worker.js src/SubOctpInterface.js src/post-worker.js build/lib/brotli/js/decode.js
        mkdir -p dist/js
        emcc src/subtitles-octopus-worker.bc $(OCTP_DEPS) \
                --pre-js src/pre-worker.js \
@@ -315,7 +314,7 @@ dist/js/subtitles-octopus-worker.js: src/subtitles-octopus-worker.bc src/pre-wor
                -s WASM=1 \
                $(EMCC_COMMON_ARGS)
 
-dist/js/subtitles-octopus-worker-legacy.js: src/subtitles-octopus-worker.bc src/pre-worker.js src/SubOctpInterface.js src/post-worker.js
+dist/js/subtitles-octopus-worker-legacy.js: src/subtitles-octopus-worker.bc src/pre-worker.js src/SubOctpInterface.js src/post-worker.js build/lib/brotli/js/decode.js
        mkdir -p dist/js
        emcc src/subtitles-octopus-worker.bc $(OCTP_DEPS) \
                --pre-js src/pre-worker.js \

@TFSThiagoBR98
Copy link
Collaborator Author

I've taken the liberty to push the changes I suggested myself, @TFSThiagoBR98 does it look alright to you?

No problem, thanks

A new commit was added for the patchfile dependencies and the diff between your previous version and the first commit of the new version is this:

Looks good to me

Is there an easy, ideally automated way to test JSO changes beyond mere compilation, btw?

Probably some parts can be tested

@TheOneric
Copy link
Member

Is there an easy, ideally automated way to test JSO changes beyond mere compilation, btw?
Probably some parts can be tested

I now managed to get the samples from the gh-pages branch working locally. They almost all seem to work and #58 is now ls working for me (I only tested with this pr applied).
The "almost" here being is that the JWPlayer-sample is not loading, as also noted in #102. From JWPlayer's website it seems like it's a commercial framework with only a time-limited free trial (and I assume the trial-period ended for the link used in gh-pages)? Does it make sense to keep a JWPlayer-demo?

TFSThiagoBR98 and others added 2 commits August 1, 2021 16:57
The existing brotli submodule already provides an almost drop-in
replacement, though in the future we might want to replace it such that
only the C implementation is used, instead of needing both a C and JS
implementation of the brotli-decompressor.
Previously changing patchfiles did not trigger a rebuild of the
associated projects.
@TheOneric TheOneric merged commit b4d403d into master Aug 3, 2021
@TheOneric TheOneric mentioned this pull request Aug 3, 2021
@TFSThiagoBR98
Copy link
Collaborator Author

TFSThiagoBR98 commented Aug 3, 2021

Does it make sense to keep a JWPlayer-demo?

I don't see any reasons to keep it now, so we can remove it.

@TheOneric TheOneric deleted the brotli branch August 3, 2021 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants