Skip to content

Parallelize pyrDown & calcSharrDeriv#15799

Merged
alalek merged 8 commits intoopencv:3.4from
Cpitis:feature/parallelization
Oct 31, 2019
Merged

Parallelize pyrDown & calcSharrDeriv#15799
alalek merged 8 commits intoopencv:3.4from
Cpitis:feature/parallelization

Conversation

@Cpitis
Copy link
Copy Markdown
Contributor

@Cpitis Cpitis commented Oct 28, 2019

This pullrequest changes

Parallellizes execution of cv::pyrDown and calcSharrDeriv. Both are used extensively in buildOpticalFlowPyramid and changes are aimed to speedup this function.

Copy link
Copy Markdown
Member

@alalek 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 contribution!

Please fix whitespace issues (check build status).
and consider adding hook to avoid whitespace issues in the future: https://github.com/opencv/opencv/wiki/How_to_contribute#q3-i-was-asked-to-remove-whitespace-issues-how-can-i-do-that


typedef short deriv_type;

struct SharrDerivInvoker : ParallelLoopBody
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider using Scharr() instead of separate implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This implementation is not mine - it was here, and was merely parallelized. Do you want me to use Scharr() instead?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like related changes are pretty small, so we can keep the current state.

BTW, Instead of Mat* we can use mutable Mat& in ParallelLoopBody classes (operator() is "const").

@Cpitis
Copy link
Copy Markdown
Contributor Author

Cpitis commented Oct 30, 2019

@alalek
I've cleared out whitespace errors and I've changed granularity, now the granularity is set based on the length divided by amount of threads ( ( end - start ) / cv::getNumThreads() ). Do you think that's reasonable or do you propose another strategy of setting granularity?

Also, the build failed but it seems its not my fault but of the build system.

@alalek
Copy link
Copy Markdown
Member

alalek commented Oct 31, 2019

Thank you for updates! Looks good to me.

May I ask you to rebase this patch onto 3.4 branch before merge. We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

So, please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no needs to re-open PR, apply changes "inplace".

@Cpitis Cpitis force-pushed the feature/parallelization branch from cdbc7af to dde7d7a Compare October 31, 2019 12:15
@Cpitis Cpitis changed the base branch from master to 3.4 October 31, 2019 12:16
@Cpitis
Copy link
Copy Markdown
Contributor Author

Cpitis commented Oct 31, 2019

@alalek Rebased onto 3.4 branch, and changed the granularity to be simply cv::getNumThreads(). Do you think we're ready to go with what we have?

Thanks,
Alex

Copy link
Copy Markdown
Member

@alalek 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 contribution!

@alalek alalek merged commit d2e0277 into opencv:3.4 Oct 31, 2019
@alalek alalek mentioned this pull request Nov 4, 2019
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.

2 participants