Skip to content

Add compatibility with latest (3.1.54) emsdk version#25084

Merged
asmorkalov merged 3 commits intoopencv:4.xfrom
EDVTAZ:emscripten-3.1.54-compat
Feb 26, 2024
Merged

Add compatibility with latest (3.1.54) emsdk version#25084
asmorkalov merged 3 commits intoopencv:4.xfrom
EDVTAZ:emscripten-3.1.54-compat

Conversation

@EDVTAZ
Copy link
Copy Markdown
Contributor

@EDVTAZ EDVTAZ commented Feb 24, 2024

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • 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 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_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
  • 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
  • With version 3.1.31 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 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

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

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!

force_builders=Custom
build_image:Docs=docs-js:18.04
build_image:Custom=javascript
buildworker:Custom=linux-1,linux-4,linux-f1

Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

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:]]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use .format or % for string formatting.

To satisfy legacy CI builders (which are probably still using Python<3.6). See "default" checks)

case Module.CV_8UC1:
case Module.CV_8UC2:
case Module.CV_8UC3:
case Module.CV_8UC4: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add alias cv = Module somewhere upper instead?
(perhaps under some condition)

@EDVTAZ EDVTAZ force-pushed the emscripten-3.1.54-compat branch from 962e2e9 to 6d75e93 Compare February 24, 2024 12:18
@EDVTAZ
Copy link
Copy Markdown
Contributor Author

EDVTAZ commented Feb 24, 2024

Thank you for the comments @opencv-alalek , I've updated the PR to address them, and now the builds pass as well.
Should I squash the commits? Is there anything else I should do?

Copy link
Copy Markdown
Contributor

@opencv-alalek opencv-alalek left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you for contribution!

@asmorkalov asmorkalov merged commit 6f48cb7 into opencv:4.x Feb 26, 2024
@asmorkalov asmorkalov mentioned this pull request Feb 26, 2024
@simonbuehler
Copy link
Copy Markdown

Thanks all you guys for gathering the fixes together!

klatism pushed a commit to klatism/opencv that referenced this pull request May 17, 2024
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:]]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.format can't handle %s because it uses {}

Copy link
Copy Markdown
Contributor Author

@EDVTAZ EDVTAZ Aug 12, 2024

Choose a reason for hiding this comment

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

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?

asmorkalov pushed a commit that referenced this pull request Oct 28, 2024
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
thewoz pushed a commit to CobbsLab/OPENCV that referenced this pull request Feb 13, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants