CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
Sole tbb executor #17851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sole tbb executor #17851
Conversation
e348b3d
to
b612d42
Compare
@anton-potapov please invite other reviewers from the team first |
@anton-potapov your turn |
#include <type_traits> | ||
#include <memory> | ||
|
||
#ifdef OPENCV_WITH_ITT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This macro is not propagated to other modules except opencv_core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm.... So what is the right way to check for ITT here then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalek ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right way is reusing OpenCV's trace.hpp functionality for profiling purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a FIXME
|
||
#define ITT_AUTO_TRACE_GUARD_IMPL_(LINE, h) ITT_NAMED_TRACE_GUARD(itt_trace_guard_##LINE , h) | ||
#define ITT_AUTO_TRACE_GUARD_IMPL(LINE, h) ITT_AUTO_TRACE_GUARD_IMPL_(LINE, h) | ||
#define ITT_AUTO_TRACE_GUARD(h) ITT_AUTO_TRACE_GUARD_IMPL(__LINE__ , h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful with definition macros with ITT_
prefixes.
Consider using of GAPI_ITT_
prefix or similar instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed prefix to GAPI_ITT
return (use_tbb_scheduler_bypass::yes == reuse_current_task) ? (recycle_as_continuation(), this) : nullptr; | ||
} | ||
~functor_task(){ | ||
assert_graph_is_running(parent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful with exceptions in destructors.
Details: https://en.cppreference.com/w/cpp/error/uncaught_exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the warning, but this intentionally is not a exception, but and assert
either CV_
one or a hand-crafted one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to be enabled in debug only
if ((active_async_tasks == 0) || (wake_master == wake_tbb_master::yes) )//was the last or there is the new TBB tasks to execute | ||
{ | ||
ITT_AUTO_TRACE_GUARD(ittTbbUnlockMasterThread); | ||
//Wile decrement of async_tasks_t::count is atomic it might be done after waiting thread checked it value but _before_ it actually start waiting on the condition variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
broken indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+a too long line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in while
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
modules/gapi/CMakeLists.txt
Outdated
@@ -94,6 +94,7 @@ set(gapi_srcs | |||
|
|||
# Executor | |||
src/executor/gexecutor.cpp | |||
src/executor/gapi_tbb_executor.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything here is gapi
, so I'd recommend renaming it to gtbbexecutor
modules/gapi/CMakeLists.txt
Outdated
if(HAVE_TBB) | ||
if(TARGET opencv_test_gapi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AND
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
// | ||
// Copyright (C) 2020 Intel Corporation | ||
|
||
#ifndef OPENCV_GAPI_GAPI_ITT_HPP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should ITT be part of the /executor
? maybe move it to backends/common
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a FIXME
|
||
#define ITT_AUTO_TRACE_GUARD_IMPL_(LINE, h) ITT_NAMED_TRACE_GUARD(itt_trace_guard_##LINE , h) | ||
#define ITT_AUTO_TRACE_GUARD_IMPL(LINE, h) ITT_AUTO_TRACE_GUARD_IMPL_(LINE, h) | ||
#define ITT_AUTO_TRACE_GUARD(h) ITT_AUTO_TRACE_GUARD_IMPL(__LINE__ , h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
} | ||
} //namespace util | ||
|
||
namespace gimpl { namespace parallel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it has nothing with parallelism, right? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right :)
added a FIXME
|
||
}}} // namespace cv::gimpl::parallel | ||
|
||
void cv::gimpl::parallel::execute(tbb::concurrent_priority_queue<tile_node* , tile_node_indirect_priority_comparator> & q) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we don't have lines long like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
//Specify tbb::task_group_context::concurrent_wait in the traits to ask TBB scheduler not to change ref_count of the task we wait on (root) when wait is complete. | ||
//As the traits is last argument explicitly specify (default) value for first argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And especially like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if ((active_async_tasks == 0) || (wake_master == wake_tbb_master::yes) )//was the last or there is the new TBB tasks to execute | ||
{ | ||
ITT_AUTO_TRACE_GUARD(ittTbbUnlockMasterThread); | ||
//Wile decrement of async_tasks_t::count is atomic it might be done after waiting thread checked it value but _before_ it actually start waiting on the condition variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+a too long line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation is very cool, thanks a lot for this!!
Comments are decorative, part of comments are just questions to understand code idea more deeply.
The main comment about code style - unaligned spaces, could you please align all indents in expressions, functions signatures and other statements? I haven't caught all unaligned spaces in comments because started to write feedback for them quite late..
The second comment about code style which is also was caught by Dmitry Matveev is too long comments and too long signatures/statements. Could you please break long comments, signatures and statements to multiple lines?
The third proposal is just about code documentation - may be we can split code by phases or highlight logical entities with comments? For example, highlight discussed logical entities with "Creating G-API static graph" (it is actually phase in the test, but still), "Creating TBB dynamic graph", "Main execution entry point for the each TBB task", "Execution of real tile node's body", "Spawning tasks for dynamic TBB graph", and so on?
if ((active_async_tasks == 0) || (wake_master == wake_tbb_master::yes) )//was the last or there is the new TBB tasks to execute | ||
{ | ||
ITT_AUTO_TRACE_GUARD(ittTbbUnlockMasterThread); | ||
//Wile decrement of async_tasks_t::count is atomic it might be done after waiting thread checked it value but _before_ it actually start waiting on the condition variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in while
Note there are failing tests: |
a97e530
to
a02c2fc
Compare
a02c2fc
to
61a4933
Compare
- the sole executor - unit tests for it - no usage in the GAPI at the momnet
- introduced new overload of execute to explicitly accept tbb::arena argument - added more basic tests - moved arena creation code into tests -
- fixed compie errors & warnings
- split all-in-one execute() function into logicaly independant parts
- used util::variant in in the tile_node
- moved copy_through_move to separate header - rearranged details staff in proper namespaces - moved all implementation into detail namespace
- fixed build error with TBB 4.4. - fixed build warnings
- aligned strings width - fixed spaces in expressions - fixed english grammar - minor improvements
- added more comments - minor improvements
- changed ITT_ prefix for macroses to GAPI_ITT
- no more "unused" warning for GAPI_DbgAssert - changed local assert macro to man onto GAPI_DbgAssert
- file renamings - changed local assert macro to man onto GAPI_DbgAsse
- test file renamed - add more comments
e96bf3f
to
a217a97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I have some little suggestions, mostly typo-corrections
- minor clenups and cosmetic changes
- minor clenups and cosmetic changes
fcc2a44
to
598371d
Compare
- changed spaces and curly braces alignment
6b9e13b
to
cd73aac
Compare
- minor cleanups
@alalek could you please merge it ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -2,16 +2,23 @@ | |||
// It is subject to the license terms in the LICENSE file found in the top-level directory | |||
// of this distribution and at https://opencv.org/license.html. | |||
// | |||
// Copyright (C) 2018 Intel Corporation | |||
// Copyright (C) 2020 Intel Corporation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2018-2020 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
@@ -33,7 +40,7 @@ namespace detail | |||
|
|||
|
|||
#ifdef NDEBUG | |||
# define GAPI_DbgAssert(expr) | |||
# define GAPI_DbgAssert(expr) cv::util::suppress_unused_warning(expr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, expression is still should be computed by compiler (and then dead code is eliminated - but not all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suppress_unused_warning(expr)
is the function call, it's actual argument i.e. expr
is calculated, then however it's value is discarded as bound to unnamed function argument.
The suppress_unused_warning
defined as:
template<typename T> void suppress_unused_warning( const T& ) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All functions and methods are not "pure" in C++ standard ("pure" is GCC extension).
So some non-inline .type()
calls will be evaluated if they are part of this expr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the classic C assert()
implementation from assert.h
/ cassert
headers (it is "dbg" assert in OpenCV):
https://en.cppreference.com/w/cpp/error/assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the assert implemented like this does not prevent "unused" warnings (and that is why it was not enough to use CV_DbgAssert
as is and I had to re-invent it. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that sizeof()
is the solution here. As it is silence the warning and does not evaluate it's argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that sizeof() is the solution here.
But not for lambdas :) - fixed to use constexpr
and bool operations short-circuit evaluation
|
||
enum class is_tbb_work_present { | ||
no, | ||
yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By coding style constants / enums should be upper case.
NO
is a macro on some platforms (Apple).
Consider using bool
type instead. Enum doesn't look as necessary here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using meaningful names instead of faceless true
/ false
, so bool
is not an option :)
will rename to UPPERCASE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed
Not yet, as it is first part of the puzzle- the executor itself. Actual usage/integration is to come up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with the TBB part, not very certain about macro changes, though
# define GAPI_DbgAssert CV_DbgAssert | ||
#else | ||
# define GAPI_DbgAssert(expr) cv::util::suppress_unused_warning(expr) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what's the reason for all these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason is - compiler warnings on release builds on unused variables in asserts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this unused variable is still going to be evaluated. Why we need that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that actually evaluate the expression is the only reliable way to actually "use" variables it made of (and there fore silence those warnings).
And yes compiling optimization of dead code removal is crucial here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"unused"
problem is in caller code. Not in assertion macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just replace to C-like assert()
and you will receive the same "unused" warning in release builds.
Workaround for such cases:
-auto result = t->decrement_ref_count();
+auto result = t->decrement_ref_count(); (void)result;
ASSERT(result >= 1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, almost every problem can be workarounded :) ... but why if there is a proper solution to it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there is a proper solution
It still evaluates expression.
More complex and common cases looks like ASSERT(node->isValid());
. Compiler's DCE can't remove that completely.
Assertion macro must not evaluate passed expression (should be empty) in case of "noop".
Do not confuse developers with new specific behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last version of the patch does not evaluate the expression (and not cause "unused" warning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it is a tricky way, but looks like it works.
63da882
to
35cf42a
Compare
// First participate in execution of TBB graph till there are no more ready tasks. | ||
ctx.root->wait_for_all(); | ||
|
||
if (!async_work_done()) { // Wait on the conditional variable iff there is active async work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iff
-> if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the efforts!
{ | ||
namespace util | ||
{ | ||
//This is a tool to move initialize captures of a lambda in C++11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check gasync.cpp
for similar stuff. We really don't want to have duplicated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do not mind, I will unify them in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <opencv2/gapi/util/copy_through_move.hpp> | ||
#include "logger.hpp" // GAPI_LOG | ||
|
||
#include <tbb/task.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW,
note: ‘#pragma message: TBB Warning: tbb/task.h is deprecated. For details, please see Deprecated Features appendix in the TBB reference manual.’
TBB (ver 2020.3 interface 11103)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I know this
# define GAPI_DbgAssert CV_DbgAssert | ||
#else | ||
# define GAPI_DbgAssert(expr) cv::util::suppress_unused_warning(expr) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it is a tricky way, but looks like it works.
#endif | ||
|
||
#include <opencv2/gapi/util/compiler_hints.hpp> | ||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iostream
Do we really need that here? (consider using <ostream>
if std::ostream
is required only without cout/cerr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch - will change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
- minor cleanups
35cf42a
to
920d8d3
Compare
Great! |
* TBB executor for GAPI - the sole executor - unit tests for it - no usage in the GAPI at the momnet * TBB executor for GAPI - introduced new overload of execute to explicitly accept tbb::arena argument - added more basic tests - moved arena creation code into tests - * TBB executor for GAPI - fixed compie errors & warnings * TBB executor for GAPI - split all-in-one execute() function into logicaly independant parts * TBB executor for GAPI - used util::variant in in the tile_node * TBB executor for GAPI - moved copy_through_move to separate header - rearranged details staff in proper namespaces - moved all implementation into detail namespace * TBB executor for GAPI - fixed build error with TBB 4.4. - fixed build warnings * TBB executor for GAPI - aligned strings width - fixed spaces in expressions - fixed english grammar - minor improvements * TBB executor for GAPI - added more comments - minor improvements * TBB executor for GAPI - changed ITT_ prefix for macroses to GAPI_ITT * TBB executor for GAPI - no more "unused" warning for GAPI_DbgAssert - changed local assert macro to man onto GAPI_DbgAssert * TBB executor for GAPI - file renamings - changed local assert macro to man onto GAPI_DbgAsse * TBB executor for GAPI - test file renamed - add more comments * TBB executor for GAPI - minor clenups and cosmetic changes * TBB executor for GAPI - minor clenups and cosmetic changes * TBB executor for GAPI - changed spaces and curly braces alignment * TBB executor for GAPI - minor cleanups * TBB executor for GAPI - minor cleanups
This Merge Request adds TBB based executor into GAPI
TBB executor description
TBB executor engine operate with graph of tasks represented via
cv::gimpl::parallel::tile_node
instances.Dependencies
For every task (
tile_node
) there might be a number of input dependencies (i.e. tasks that should be executed before). This number is represented twice in thetile_node
structure. They aredependencies
anddependency_count
. First one describes total number (i.e. input edges in the graph before execution) and does not change during execution, while second one describes current number of unresolved input dependencies. Oncedependency_count
drops to zero, task is ready for execution as all it's dependencies are satisfiedDependents
Every task that directly depends on the current one is listed in the
tile_node::dependees
member. The engine decreesdependency_count
of these tasks right after the completion of the current task.Sync Vs. Async payload
Task's body in enclosed in either
task_body
orasync_task_body
members depending on task's nature (andasync
flag). While in both cases the engine calls passed in function there are two differences:Execution and Priorities
Once all input dependencies for the task are satisfied (i.e. it's
dependency_count
drops to zero) it is treated as ready for execution and is put into the "ready" queue. The queue (tbb::concurrent_priority_queue
) sorts all task according to their priorities.Some TBB tasking Likbez
TBB uses
tbb::task
derivatives to describe a piece of work. They can be linked together into a tree describing tasks dependencies. Every TBB task has arefcount
field describing unsatisfied dependencies. Once it drops to zero task is ready for execution (i.e. can bespawn
ed). Dependency tasks are called child tasks (i.e. parent depends on it's childs). Every child task on creation increases it's parent'srefcount
. After child task is executed TBB task scheduler automatically decrease parent'srefcount
. Root of tasks tree is used only to call it'stbb::task::wait_for_all
method to wait for the CPU work to complete.wait_for_all
waits for rootrefcount
value to become1
as a sign that all work is done.Mapping to TBB
The executor engine creates a
root
task (derived fromtbb::task
) - a root of the task's tree. Once newtile_node
item is pushed to the ready queue, new child task ofroot
's is created (Effectively increasingroot
dependency count). Every child task body does the following :tile_node
instance from the priority queuedependency_count
for every dependenttile_node
instance (viatile_node::dependees
member)tile_node
withdependency_count
dropped to zero,spawn
extra (tbb
) child task.Asynchronicity work and TBB active waiting
While waiting for the root task, TBB does not put to sleep the calling thread. Instead, it keeps searching for
spawn
ed (orenqueue
d) TBB tasks to do, assuming it somehow related with theroot
task.However, in case of async work (like GPU or non TBB CPU work) there may be no such (tbb) tasks to do. In this case calling thread wasting CPU time, constantly searching for tasks or spin waiting. To overcome drawback, more elaborated scheme then plain calling to
tbb::task::wait_for_all
is needed.The scheme
Master thread (i.e. one that enters
cv::gimpl::parallel::execute
) after graph execution ignition does the following loop:root->wait_for_all()
effectively participating in those tasks execution(
wait_for_all()
will return once all (soawn
ed) childes ofroot
complete)In order to track number of active asynchronous tasks
count
filed ofexec_ctx::async_tasks
is used.Graph re-execution
The engine allows correct re-execution of already completed (
tile_node
based) graph instance. To allow itdependency_count
of every executedtile_node
is reset back todependencies
immediately after execution.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.