webarkit / opencv-em

A simple utility script to build OpenCV static libs with Emscripten
GNU General Public License v3.0
19 stars 3 forks source link

Add functionality & documentation to mimic OpenCVs correct include st… #7

Closed Algomorph closed 11 months ago

Algomorph commented 11 months ago
kalwalt commented 11 months ago

Hi @Algomorph thank you for this PR! Anyway i made some changes in the code, i think your code need to be updated, anyway i will try to test it.

Algomorph commented 11 months ago

@kalwalt sounds good. I updated my fork based on your changes, but I also had to make some changes. It looks like we are replicating each-other's efforts, i.e. adding WASM simd, etc.

My fork uses different shell script syntax, e.g.: ./build.sh emscripten --simd instead of ./build.sh emscripten-simd

There are a bunch of other options. If you want the canonical include structure, you can use --canonical_includes on my fork now. I made it so that by default it generates the same package structure as yours.

I saw your work on SIMD. I was gettting build errors on WASM build with SIMD with some missing wasm+simd instructions. In case you are seeing that too, there's a simple fix. In modules/core/include/opencv2/core/hal/intrin_wasm.hpp, add:

#if defined(__EMSCRIPTEN__)
#include <emscripten.h>
#endif

at the top of the file, underneath the include guards.

I also have the --pthreads option that uses the pthread WASM optimization. I don't change the build folder for that currently from opencv_js like it is done for SIMD, maybe that will need to change.

I'm not using this for WebARKit, but rather for a MediaPipe-based WASM application, hence the alternative flags that make build output more general-purpose. If you like this, maybe it's still a good idea to merge, but I understand if you don't want the extra script complexity for WebARKit specifically.

kalwalt commented 11 months ago
My fork uses different shell script syntax, e.g.:
./build.sh emscripten --simd
instead of
./build.sh emscripten-simd

I'm super fine with this approach, and with other otions too.

I saw your work on SIMD. I was gettting build errors on WASM build with SIMD with some missing wasm+simd instructions. In case you are seeing that too, there's a simple fix. In [modules/core/include/opencv2/core/hal/intrin_wasm.hpp](https://github.com/webarkit/opencv/blob/308951c59e87672816f58da3e65c0a3968eaadf7/modules/core/include/opencv2/core/hal/intrin_wasm.hpp), add:

#if defined(__EMSCRIPTEN__)
#include <emscripten.h>
#endif
at the top of the file, underneath the include guards.

I didn't receive them, but i modified opencv on our fork in this branch https://github.com/webarkit/opencv/tree/webarkit-4.7.0-simd for that, maybe are you not using it?

In regards of pthreads, yes maybe it's better to assign a separate folder for the compilation.

I tested your script but i have this issue: I run the command ./build.sh linux but i got this error message ./build.sh: row 213: pcregrep: command not found. I'm testing this on my Ubuntu 22.04 machine. it seems that i have not installed pcregrep. All goes fine when i install the pcregrep package, so maybe you are using another linux distro. Maybe it would be better to force the installation of the package before running the linux option.

I'm not using this for WebARKit, but rather for a MediaPipe-based WASM application, hence the alternative flags that make build output more general-purpose. If you like this, maybe it's still a good idea to merge, but I understand if you don't want the extra script complexity for WebARKit specifically.

Good to know! good luck for your project! Any improvements is welcome, of course if not breaks compatibilty with our projects.

kalwalt commented 11 months ago

If i have understood, you have used argbash to update the build script?

Algomorph commented 11 months ago

If i have understood, you have used argbash to update the build script?

Yes, this is correct

Algomorph commented 11 months ago

I tested your script but i have this issue: I run the command ./build.sh linux but i got this error message ./build.sh: row 213: pcregrep: command not found.

sudo apt install pcregrep should fix the issue. It may be possible to add this to the build script, but I don't know what your policy is on that. The issue is regular grep isn't able to match groups. I put that in there for OpenCV version parsing, because I was experimenting with alternative OpenCV versions. But maybe you don't need that.

kalwalt commented 11 months ago

I tested your script but i have this issue: I run the command ./build.sh linux but i got this error message ./build.sh: row 213: pcregrep: command not found.

sudo apt install pcregrep should fix the issue. It may be possible to add this to the build script, but I don't know what you policy is on that. The issue is regular grep isn't able to match groups. I put that in there for OpenCV version parsing, because I was experimenting with alternative OpenCV versions. But maybe you don't need that.

Yes installing the pcregrep package solved the issue, i haven't taken a decision on the policy to use yet, i will think about this. Thank you!

kalwalt commented 11 months ago

We might add a check routine inside the code to verify if a package is installed or not, without installing anything, just to notify. Example code:

#!/bin/bash

# Function to allow check for required packages.
function check_package {
    # Variant for distros that use debian packaging.
    if (type dpkg-query >/dev/null 2>&1) ; then
        if ! $(dpkg-query -W -f='${Status}' $1 | grep -q '^install ok installed$') ; then
            echo "Warning: required package '$1' does not appear to be installed. To install it use 'sudo apt-get install $1'."
        fi
    # Variant for distros that use rpm packaging.
    elif (type rpm >/dev/null 2>&1) ; then
        if ! $(rpm -qa | grep -q $1) ; then
            echo "Warning: required package '$1' does not appear to be installed. To install it use 'sudo dnf install $1'."
        fi
    fi
}

check_package pcregrep

echo 'Done'

If you know a better approach you are welcome.

kalwalt commented 11 months ago

I will merge this PR in the evening, and i will add the last feature by myself.

Algomorph commented 11 months ago

@kalwalt I apologize for the prolonged silence, I'm really swamped at work solving a tough problem. Yes, please feel free to merge as you see fit. I would recommend merging to a different branch first, before you normalize the output 100% to what WebARKit needs currently. E.g. the output archive/folder naming is a little different in my current branch, and you mentioned you wanted a separate directory structure for pthreads/no-pthreads variants. That would actually yield up to four different variants:

The number of variants has the potential to grow polynomialy. Hence, my second recommendation would be to populate the currently-empty em-flags.txt in the output instead of folder name alterations, so that WebARKit (and others) can actually use that information instead of the folder name if the need arises.

But, of course, only if you have time.

kalwalt commented 11 months ago

Thank you @Algomorph, for now i will merge in the dev branch, i will make other improves in a separate PR.