From d88730685e9f7b093743870d9354047bb54181fe Mon Sep 17 00:00:00 2001 From: Vadim Levin Date: Wed, 19 Jan 2022 00:07:50 +0300 Subject: [PATCH] fix: submodules creation and registration - Add special case handling when submodule has the same name as parent - `PyDict_SetItemString` doesn't steal reference, so reference count should be explicitly decremented to transfer object life-time ownership - Add sanity checks for module registration input - Add Python 2 and Python 3 reference counting handling --- .../include/opencv2/core/bindings_utils.hpp | 6 + modules/python/src2/cv2.cpp | 203 ++++++++++++++---- modules/python/test/test_misc.py | 27 +++ 3 files changed, 200 insertions(+), 36 deletions(-) diff --git a/modules/core/include/opencv2/core/bindings_utils.hpp b/modules/core/include/opencv2/core/bindings_utils.hpp index 22a86ff9be..7a50390aed 100644 --- a/modules/core/include/opencv2/core/bindings_utils.hpp +++ b/modules/core/include/opencv2/core/bindings_utils.hpp @@ -219,6 +219,12 @@ AsyncArray testAsyncException() return p.getArrayResult(); } +namespace nested { +CV_WRAP static inline bool testEchoBooleanFunction(bool flag) { + return flag; +} +} // namespace nested + namespace fs { CV_EXPORTS_W cv::String getCacheDirectoryForDownloads(); } // namespace fs diff --git a/modules/python/src2/cv2.cpp b/modules/python/src2/cv2.cpp index b39db34fcb..294905c783 100644 --- a/modules/python/src2/cv2.cpp +++ b/modules/python/src2/cv2.cpp @@ -130,45 +130,155 @@ struct ConstDef long long val; }; -static void init_submodule(PyObject * root, const char * name, PyMethodDef * methods, ConstDef * consts) -{ - // traverse and create nested submodules - std::string s = name; - size_t i = s.find('.'); - while (i < s.length() && i != std::string::npos) - { - size_t j = s.find('.', i); - if (j == std::string::npos) - j = s.length(); - std::string short_name = s.substr(i, j-i); - std::string full_name = s.substr(0, j); - i = j+1; +static inline bool strStartsWith(const std::string& str, const std::string& prefix) { + return prefix.empty() || \ + (str.size() >= prefix.size() && std::memcmp(str.data(), prefix.data(), prefix.size()) == 0); +} - PyObject * d = PyModule_GetDict(root); - PyObject * submod = PyDict_GetItemString(d, short_name.c_str()); - if (submod == NULL) +static inline bool strEndsWith(const std::string& str, char symbol) { + return !str.empty() && str[str.size() - 1] == symbol; +} + +/** + * \brief Creates a submodule of the `root`. Missing parents submodules + * are created as needed. If name equals to parent module name than + * borrowed reference to parent module is returned (no reference counting + * are done). + * Submodule lifetime is managed by the parent module. + * If nested submodules are created than the lifetime is managed by the + * predecessor submodule in a list. + * + * \param parent_module Parent module object. + * \param name Submodule name. + * \return borrowed reference to the created submodule. + * If any of submodules can't be created than NULL is returned. + */ +static PyObject* createSubmodule(PyObject* parent_module, const std::string& name) +{ + if (!parent_module) { - submod = PyImport_AddModule(full_name.c_str()); - PyDict_SetItemString(d, short_name.c_str(), submod); + return PyErr_Format(PyExc_ImportError, + "Bindings generation error. " + "Parent module is NULL during the submodule '%s' creation", + name.c_str() + ); + } + if (strEndsWith(name, '.')) + { + return PyErr_Format(PyExc_ImportError, + "Bindings generation error. " + "Submodule can't end with a dot. Got: %s", name.c_str() + ); } - if (short_name != "") - root = submod; - } + const std::string parent_name = PyModule_GetName(parent_module); - // populate module's dict - PyObject * d = PyModule_GetDict(root); - for (PyMethodDef * m = methods; m->ml_name != NULL; ++m) - { - PyObject * method_obj = PyCFunction_NewEx(m, NULL, NULL); - PyDict_SetItemString(d, m->ml_name, method_obj); - Py_DECREF(method_obj); - } - for (ConstDef * c = consts; c->name != NULL; ++c) - { - PyDict_SetItemString(d, c->name, PyLong_FromLongLong(c->val)); - } + /// Special case handling when caller tries to register a submodule of the parent module with + /// the same name + if (name == parent_name) { + return parent_module; + } + if (!strStartsWith(name, parent_name)) + { + return PyErr_Format(PyExc_ImportError, + "Bindings generation error. " + "Submodule name should always start with a parent module name. " + "Parent name: %s. Submodule name: %s", parent_name.c_str(), + name.c_str() + ); + } + + size_t submodule_name_end = name.find('.', parent_name.size() + 1); + /// There is no intermediate submodules in the provided name + if (submodule_name_end == std::string::npos) + { + submodule_name_end = name.size(); + } + + PyObject* submodule = parent_module; + + for (size_t submodule_name_start = parent_name.size() + 1; + submodule_name_start < name.size(); ) + { + const std::string submodule_name = name.substr(submodule_name_start, + submodule_name_end - submodule_name_start); + + const std::string full_submodule_name = name.substr(0, submodule_name_end); + + + PyObject* parent_module_dict = PyModule_GetDict(submodule); + /// If submodule already exists it can be found in the parent module dictionary, + /// otherwise it should be added to it. + submodule = PyDict_GetItemString(parent_module_dict, + submodule_name.c_str()); + if (!submodule) + { + /// Populates global modules dictionary and returns borrowed reference to it + submodule = PyImport_AddModule(full_submodule_name.c_str()); + if (!submodule) + { + /// Return `PyImport_AddModule` NULL with an exception set on failure. + return NULL; + } + /// Populates parent module dictionary. Submodule lifetime should be managed + /// by the global modules dictionary and parent module dictionary, so Py_DECREF after + /// successfull call to the `PyDict_SetItemString` is redundant. + if (PyDict_SetItemString(parent_module_dict, submodule_name.c_str(), submodule) < 0) { + return PyErr_Format(PyExc_ImportError, + "Can't register a submodule '%s' (full name: '%s')", + submodule_name.c_str(), full_submodule_name.c_str() + ); + } + } + + submodule_name_start = submodule_name_end + 1; + + submodule_name_end = name.find('.', submodule_name_start); + if (submodule_name_end == std::string::npos) { + submodule_name_end = name.size(); + } + } + return submodule; +} + +static bool init_submodule(PyObject * root, const char * name, PyMethodDef * methods, ConstDef * consts) +{ + // traverse and create nested submodules + PyObject* submodule = createSubmodule(root, name); + if (!submodule) + { + return false; + } + // populate module's dict + PyObject * d = PyModule_GetDict(submodule); + for (PyMethodDef * m = methods; m->ml_name != NULL; ++m) + { + PyObject * method_obj = PyCFunction_NewEx(m, NULL, NULL); + if (PyDict_SetItemString(d, m->ml_name, method_obj) < 0) + { + PyErr_Format(PyExc_ImportError, + "Can't register function %s in module: %s", m->ml_name, name + ); + Py_CLEAR(method_obj); + return false; + } + Py_DECREF(method_obj); + } + for (ConstDef * c = consts; c->name != NULL; ++c) + { + PyObject* const_obj = PyLong_FromLongLong(c->val); + if (PyDict_SetItemString(d, c->name, const_obj) < 0) + { + PyErr_Format(PyExc_ImportError, + "Can't register constant %s in module %s", c->name, name + ); + Py_CLEAR(const_obj); + return false; + } + Py_DECREF(const_obj); + } + return true; } #include "pyopencv_generated_modules_content.h" @@ -176,7 +286,10 @@ static void init_submodule(PyObject * root, const char * name, PyMethodDef * met static bool init_body(PyObject * m) { #define CVPY_MODULE(NAMESTR, NAME) \ - init_submodule(m, MODULESTR NAMESTR, methods_##NAME, consts_##NAME) + if (!init_submodule(m, MODULESTR NAMESTR, methods_##NAME, consts_##NAME)) \ + { \ + return false; \ + } #include "pyopencv_generated_modules.h" #undef CVPY_MODULE @@ -193,7 +306,13 @@ static bool init_body(PyObject * m) PyObject* d = PyModule_GetDict(m); - PyDict_SetItemString(d, "__version__", PyString_FromString(CV_VERSION)); + PyObject* version_obj = PyString_FromString(CV_VERSION); + if (PyDict_SetItemString(d, "__version__", version_obj) < 0) { + PyErr_SetString(PyExc_ImportError, "Can't update module version"); + Py_CLEAR(version_obj); + return false; + } + Py_DECREF(version_obj); PyObject *opencv_error_dict = PyDict_New(); PyDict_SetItemString(opencv_error_dict, "file", Py_None); @@ -207,7 +326,18 @@ static bool init_body(PyObject * m) PyDict_SetItemString(d, "error", opencv_error); -#define PUBLISH(I) PyDict_SetItemString(d, #I, PyInt_FromLong(I)) +#define PUBLISH_(I, var_name, type_obj) \ + PyObject* type_obj = PyInt_FromLong(I); \ + if (PyDict_SetItemString(d, var_name, type_obj) < 0) \ + { \ + PyErr_SetString(PyExc_ImportError, "Can't register " var_name " constant"); \ + Py_CLEAR(type_obj); \ + return false; \ + } \ + Py_DECREF(type_obj); + +#define PUBLISH(I) PUBLISH_(I, #I, I ## _obj) + PUBLISH(CV_8U); PUBLISH(CV_8UC1); PUBLISH(CV_8UC2); @@ -243,6 +373,7 @@ static bool init_body(PyObject * m) PUBLISH(CV_64FC2); PUBLISH(CV_64FC3); PUBLISH(CV_64FC4); +#undef PUBLISH_ #undef PUBLISH return true; diff --git a/modules/python/test/test_misc.py b/modules/python/test/test_misc.py index 051ac33ac9..48657d595c 100644 --- a/modules/python/test/test_misc.py +++ b/modules/python/test/test_misc.py @@ -1,6 +1,7 @@ #!/usr/bin/env python from __future__ import print_function +import sys import ctypes from functools import partial from collections import namedtuple @@ -607,6 +608,32 @@ class Arguments(NewOpenCVTests): self.assertTrue(isinstance(rr, tuple), msg=type(rrv)) self.assertEqual(len(rr), 3) + def test_nested_function_availability(self): + self.assertTrue(hasattr(cv.utils, "nested"), + msg="Module is not generated for nested namespace") + self.assertTrue(hasattr(cv.utils.nested, "testEchoBooleanFunction"), + msg="Function in nested module is not available") + + if sys.version_info[0] < 3: + # Nested submodule is managed only by the global submodules dictionary + # and parent native module + expected_ref_count = 2 + else: + # Nested submodule is managed by the global submodules dictionary, + # parent native module and Python part of the submodule + expected_ref_count = 3 + + # `getrefcount` temporary increases reference counter by 1 + actual_ref_count = sys.getrefcount(cv.utils.nested) - 1 + + self.assertEqual(actual_ref_count, expected_ref_count, + msg="Nested submodule reference counter has wrong value\n" + "Expected: {}. Actual: {}".format(expected_ref_count, actual_ref_count)) + for flag in (True, False): + self.assertEqual(flag, cv.utils.nested.testEchoBooleanFunction(flag), + msg="Function in nested module returns wrong result") + + class CanUsePurePythonModuleFunction(NewOpenCVTests): def test_can_get_ocv_version(self): import sys