Skip to content

Conversation

MrKepzie
Copy link
Contributor

@MrKepzie MrKepzie commented Jan 4, 2021

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • [X ] I agree to contribute to the project under Apache 2 License.
  • [ X] 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
  • [ X] 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.
  • [X ] The feature is well documented and sample code can be built with the project CMake

@alalek
Copy link
Member

alalek commented Jan 4, 2021

Thank you for contribution!

Please fix the whitespace issue (and amend commit in this PR: git commit -a --am).
Also please double check commit e-mails (they should be consistent with emails in your GitHub account). You may need to reset commit's author: git commit -a --am --reset-author


// Interpolators below expect non empty matches
if (filtered_prevPoints.empty())
return;
Copy link
Member

Choose a reason for hiding this comment

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

/cc @tsenst Could you please take a look on this change? Should we "reset" some fields/parameters to return consistent result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ammended the commit with correct author, i think this should be fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

I can take a look during the next days

Copy link
Contributor

Choose a reason for hiding this comment

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

@MrKepzie thank you for this bugfix 👍 . Here the optical flow could not be computed because of the algorithm behaviour. It would be great if it would be possible to indicate this. Unfortunately the DenseOpticalFlow class has no interface for the error case. Maybe throwing an exception would be the correct way. If not the flow should be at least set to 0 ( e.g. flow.setTo(0) ). In both cases please update the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

CV_Error(Error::StsNoConv, ...) may be used to indicate issue (throwing an exception).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to return a vector field with 0 everywhere is this is not really an error but just 2 images without any correspondences, I'm going to ammend commit and do as suggested

Copy link
Contributor

Choose a reason for hiding this comment

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

And please add a note in the docfile that when no correspondences could be found for interpolation the optical flow field would be 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated the commit

@MrKepzie MrKepzie force-pushed the optflow-rlof-crash branch from 4d23cb7 to f57990d Compare January 4, 2021 14:15
@MrKepzie MrKepzie force-pushed the optflow-rlof-crash branch from f57990d to 55f5303 Compare January 9, 2021 16:46
Copy link
Contributor

@tsenst tsenst left a comment

Choose a reason for hiding this comment

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

Well done.

@opencv-pushbot opencv-pushbot merged commit 697d9e2 into opencv:master Jan 11, 2021
@alalek alalek mentioned this pull request Apr 9, 2021
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.

4 participants