Add compatibility with latest (3.1.54) emsdk version#25084
Add compatibility with latest (3.1.54) emsdk version#25084asmorkalov merged 3 commits intoopencv:4.xfrom
Conversation
opencv-alalek
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
platforms/js/build_js.py
Outdated
| parser.add_argument('--webnn', action="store_true", help="Enable WebNN Backend") | ||
|
|
||
| args = parser.parse_args() | ||
| transformed_args = [f'--cmake_option={arg}' if arg[:2] == "-D" else arg for arg in sys.argv[1:]] |
There was a problem hiding this comment.
Please use .format or % for string formatting.
To satisfy legacy CI builders (which are probably still using Python<3.6). See "default" checks)
modules/js/src/helpers.js
Outdated
| case Module.CV_8UC1: | ||
| case Module.CV_8UC2: | ||
| case Module.CV_8UC3: | ||
| case Module.CV_8UC4: { |
There was a problem hiding this comment.
Could we add alias cv = Module somewhere upper instead?
(perhaps under some condition)
962e2e9 to
6d75e93
Compare
|
Thank you for the comments @opencv-alalek , I've updated the PR to address them, and now the builds pass as well. |
opencv-alalek
left a comment
There was a problem hiding this comment.
LGTM 👍 Thank you for contribution!
|
Thanks all you guys for gathering the fixes together! |
Add compatibility with latest (3.1.54) emsdk version opencv#25084 ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [ ] The PR is proposed to the proper branch - [ ] There is a reference to the original bug report and related work - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake ### Details I was following [this tutorial](https://docs.opencv.org/4.9.0/d4/da1/tutorial_js_setup.html) for building opencv with wasm target. The tutorial mentions that the last verified version of emscripten that is tested with opencv is 2.0.10, but I was curious if I could get it to work with more recent versions. I've run into a few issues with the latest version, for which fixes are included in this PR. I've found a few issues that have the same problems I encountered: - opencv#24620 - opencv#20313 - https://stackoverflow.com/questions/77469603/custom-opencv-js-wasm-using-cv-matfromarray-results-in-cv-mat-is-not-a-co - emscripten-core/emscripten#14803 - opencv#24572 - opencv#19493 (comment) I used the docker image for building and comparing results with different emsdk versions. I tested by building with `--build_wasm` and `--build-test` flags and ran the tests in the browser. I addressed the following issues with newer versions of emscripten: - In newer versions `EMSCRIPTEN` environemnt variable was stopped being set. I added support for deriving location based on the `EMSDK` environment variable, as suggested [here](emscripten-core/emscripten#14803) - In newer versions emcmake started passing `-DCMAKE...` arguments, however the opencv python script didn't know how to handle them. I added processing to the args that will forward all arguments to `cmake` that start with `-D`. I opted for this in hopes of being more futureproof, but another approach could be just ignoreing them, or explicitly forwarding them instead of matching anything starting with `-D`. These approches were suggested [here](opencv#19493 (comment)) - With [version 3.1.31](https://github.com/emscripten-core/emscripten/blob/main/ChangeLog.md#3131---012623) some previously exported functions stopped being automatically exported. Because of this, `_free` and `_malloc` were no longer available and had to be explicitly exported because of breaking tests. - With [version 3.1.42](emscripten-core/emscripten@3.1.41...3.1.42#diff-e505aa80b2764c0197acfc9afd8179b3600f0ab5dd00ff77db01879a84515cdbL3875) the `post-js` code doesn't receive the module named as `EXPORT_NAME` anymore, but only as `moduleArg`/`Module`. This broke existing code in `helpers.js`, which was referencing exported functions through `cv.Mat`, etc. I changed all of these references to use `Module.Mat`, etc. If it is preferred, alternatively the `cv` variable could be reintroduced in `helper.js` as suggested [here](opencv#24620) With the above changes in place, I can successfully build and run tests with the latest emscripten/emsdk docker image (also with 2.0.10 and most of the other older tags, except for a few that contain transient issues like [this](emscripten-core/emscripten#17700)). This is my first time contributing to opencv, so I hope I got everything correct in this PR, but please let me know if I should change anything!
| parser.add_argument('--webnn', action="store_true", help="Enable WebNN Backend") | ||
|
|
||
| args = parser.parse_args() | ||
| transformed_args = ["--cmake_option=%s".format(arg) if arg[:2] == "-D" else arg for arg in sys.argv[1:]] |
There was a problem hiding this comment.
.format can't handle %s because it uses {}
There was a problem hiding this comment.
Sorry I didn't catch this back then, I usually use the f'' format and didn't realise that .format needed different placeholders. During my retest it probably just ended up using a default value, which was correct for my setup.
Would you like me to do a quick followup PR to modify the format string?
Fix incorrect string format in js build script #26374 I accidentally met this small problem mentioned in #25084 (comment) when play with wasm build. It seems https://github.com/EDVTAZ didn't fix it yet, so I create this tiny pr. Additionally, I remove a redundant argument in `add_argument` call. `'store_true'` already set the default, see https://docs.python.org/3/library/argparse.html#action. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
Fix incorrect string format in js build script opencv#26374 I accidentally met this small problem mentioned in opencv#25084 (comment) when play with wasm build. It seems https://github.com/EDVTAZ didn't fix it yet, so I create this tiny pr. Additionally, I remove a redundant argument in `add_argument` call. `'store_true'` already set the default, see https://docs.python.org/3/library/argparse.html#action. ### Pull Request Readiness Checklist See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request - [x] I agree to contribute to the project under Apache 2 License. - [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV - [x] The PR is proposed to the proper branch - [x] There is a reference to the original bug report and related work - [ ] There is accuracy test, performance test and test data in opencv_extra repository, if applicable Patch to opencv_extra has the same branch name. - [ ] The feature is well documented and sample code can be built with the project CMake
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Details
I was following this tutorial for building opencv with wasm target. The tutorial mentions that the last verified version of emscripten that is tested with opencv is 2.0.10, but I was curious if I could get it to work with more recent versions. I've run into a few issues with the latest version, for which fixes are included in this PR. I've found a few issues that have the same problems I encountered:
I used the docker image for building and comparing results with different emsdk versions. I tested by building with
--build_wasmand--build-testflags and ran the tests in the browser. I addressed the following issues with newer versions of emscripten:EMSCRIPTENenvironemnt variable was stopped being set. I added support for deriving location based on theEMSDKenvironment variable, as suggested here-DCMAKE...arguments, however the opencv python script didn't know how to handle them. I added processing to the args that will forward all arguments tocmakethat start with-D. I opted for this in hopes of being more futureproof, but another approach could be just ignoreing them, or explicitly forwarding them instead of matching anything starting with-D. These approches were suggested here_freeand_mallocwere no longer available and had to be explicitly exported because of breaking tests.post-jscode doesn't receive the module named asEXPORT_NAMEanymore, but only asmoduleArg/Module. This broke existing code inhelpers.js, which was referencing exported functions throughcv.Mat, etc. I changed all of these references to useModule.Mat, etc. If it is preferred, alternatively thecvvariable could be reintroduced inhelper.jsas suggested hereWith the above changes in place, I can successfully build and run tests with the latest emscripten/emsdk docker image (also with 2.0.10 and most of the other older tags, except for a few that contain transient issues like this).
This is my first time contributing to opencv, so I hope I got everything correct in this PR, but please let me know if I should change anything!