Prefix global javascript functions with sub-namespaces by keroiber · Pull Request #20743 · opencv/opencv

keroiber · 2021-09-24T07:31:55Z

Description

Fixes #15299
Both the global namespace and the fisheye namespace contain the function projectPoints. The bindings generator exports both functions with the original name globally. This solution adds the namespace as prefix to global function names, resulting in distict function names. In this case: projectPoints and fisheye_projectPoints.

Incompatibilities

All functions in namespaces will get a prefix in js. From the modules that are exported by default the dnn module is affected.

Personally I would prefer to make javscript objects for namespaces and add all enums, classes and functions there - but that would be a bigger change.

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

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

alalek

Thank you for contribution!

This change also replaces names of "dnn" module calls which can break existed user code. So it can't be accepted in the current form.

Could you please reimplement namespaces_dict from Java binding?
Prefix is applied to fisheye through config file

keroiber · 2021-09-29T04:30:45Z

@alalek as I mentioned in the PR I am aware of that problem and didn't expect the PR to be accepted. Thanks for the namespaces_dict hint, that was exactly what I was searching for. I will implement it that way. However, I am tempted to do it the other way around: Export with namespace by default and put the dnn as an exception in the dictionary.

Nevertheless I still would prefer to see the namespaces reflected in the js interface as described in the PR. That can avoid future name clashes. Assume a new function in dnn that has a namesake in another module. This cannot be solved with the namespace dictionary approach.
Would there be a chance to get that? Maybe as an alternative or additional js build result?

alalek · 2021-09-29T07:38:48Z

I take a look on original issue one more time.
JS functions are filtered through whitelist (to avoid very large build artifacts).
Example of such configuration is here: https://github.com/opencv/opencv/blob/master/platforms/js/opencv_js.config.py
Perhaps we should improve that to properly handle namespaces too.

P.S. need to check if embindgen could emulate "namespaces" (to avoid "ns_" prefixes).

keroiber · 2021-09-29T08:24:56Z

Example of such configuration is here: https://github.com/opencv/opencv/blob/master/platforms/js/opencv_js.config.py

That's where I intend to add the namespaces_dict - this is the correct place, isn't it?

P.S. need to check if embindgen could emulate "namespaces" (to avoid "ns_" prefixes).

I also didn't check that til now. It seens that embind doesn't support that, see also this issue

alalek · 2021-09-29T09:16:04Z

Yes. this is correct place.
Currently binding generator processes 2 instances from cv and cv::fisheye namespaces.

Probably we can fix filtering process (and exclude cv::fisheye) to resolve the original issue. It would be a quick fix, so we can return to nested namespaces problem later (looks like that patch would be large enough).

- avoid functions override with same name but different namespace

alalek · 2021-10-03T00:11:29Z

Implemented filtering based on namespace_prefix_override.
Rebased to 3.4 branch.

keroiber

Looks good

keroiber · 2021-10-04T05:42:05Z

modules/js/generator/embindgen.py


-            if js_func_name == "_findHomography1":
-                os.error(ns_name, ns_parts, ns_part)
-


I had already a commit prepared for removing the debug output, but you were faster...

keroiber · 2021-10-04T06:26:30Z

platforms/js/opencv_js.config.py


 white_list = makeWhiteList([core, imgproc, objdetect, video, dnn, features2d, calib3d])
+
+# namespace_prefix_override['dnn'] = ''  # compatibility stuff (enabled by default)


Wouldn't it be clearer to define the compatibility behaviour here and leave namespace_prefix_override empty in embindgen.py?

This file is a sample of opencv.js configuration.
Users of custom opencv.js builds already have own configurations, so we need guarantee compatibility for them.

keroiber · 2021-10-04T06:27:22Z

platforms/js/opencv_js.config.py

+        'fisheye_initUndistortRectifyMap',
+        'fisheye_projectPoints',
+    ],
+}


Now this is the only list in the file that's formatted this way

1 item per-line formatting is for easy merging between branches (it is already partially used on the master/4.x branch).

keroiber · 2021-10-08T06:55:34Z

@alalek thank you for finishing the compatibility parts

keroiber mentioned this pull request Sep 24, 2021

Building OpenCV.js with projectPoints + solvePnP #15299

Closed

asmorkalov added the category: javascript (js) label Sep 24, 2021

alalek reviewed Sep 28, 2021

View reviewed changes

keroiber and others added 2 commits October 3, 2021 00:01

Prefix global javascript functions with sub-namespaces

92c33e4

js: handle 'namespace_prefix_override', update filtering

75d67f1

- avoid functions override with same name but different namespace

keroiber commented Oct 4, 2021

View reviewed changes

alalek approved these changes Oct 4, 2021

View reviewed changes

alalek merged commit f11f2bf into opencv:3.4 Oct 4, 2021

alalek mentioned this pull request Oct 4, 2021

(4.x) Merge 3.4 #20805

Merged

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged


		if js_func_name == "_findHomography1":
		os.error(ns_name, ns_parts, ns_part)


		white_list = makeWhiteList([core, imgproc, objdetect, video, dnn, features2d, calib3d])

		# namespace_prefix_override['dnn'] = '' # compatibility stuff (enabled by default)

Uh oh!

Conversation

keroiber commented Sep 24, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Incompatibilities

Pull Request Readiness Checklist

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

keroiber commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Sep 29, 2021

Uh oh!

keroiber commented Sep 29, 2021

Uh oh!

alalek commented Sep 29, 2021

Uh oh!

alalek commented Oct 3, 2021

Uh oh!

keroiber left a comment

Choose a reason for hiding this comment

Uh oh!

keroiber Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

keroiber Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

keroiber Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

keroiber commented Oct 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

keroiber commented Sep 24, 2021 •

edited by alalek

Loading

keroiber commented Sep 29, 2021 •

edited

Loading