Pev fix lsh bad any cast by pemmanuelviel · Pull Request #17640 · opencv/opencv

pemmanuelviel · 2020-06-23T14:11:31Z

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 OpenCV (BSD) 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

allow_multiple_commits=1

alalek

Thank you for the contribution!

Could you please add simple test case for this? (which triggers this exception)

TEST(Flann_LshTable, regression_bad_any_cast)
{
    LshIndexParams params(...);
    ...
    EXPECT_NO_THROW(... check expression ...);
}

Test is useful to avoid similar regressions in the future.

alalek · 2020-06-25T06:48:43Z

modules/flann/include/opencv2/flann/lsh_index.h

 struct LshIndexParams : public IndexParams
 {
-    LshIndexParams(unsigned int table_number = 12, unsigned int key_size = 20, unsigned int multi_probe_level = 2)
+    LshIndexParams(int table_number = 12, int key_size = 20, int multi_probe_level = 2)


Lets do not change constructor parameters (preserve source and ABI compatibility).

I believe we can fix that in this way (without changing of struct fields / constructors):

-(*this)["table_number"] = table_number; +(*this)["table_number"] = (int)table_number;

change type for "key_size", "multi_probe_level".

Good catch, thanks.
I've made the change to see if this passes the tests now.
Adding the test will be my next task.

alalek · 2020-06-27T20:13:47Z

modules/flann/include/opencv2/flann/lsh_index.h

+    int key_size_;
    /** How far should we look for neighbors in multi-probe LSH */
-    unsigned int multi_probe_level_;
+    int multi_probe_level_;


Please revert these changes too

I've squashed all these commits in a new branch "pev--fix-bad-any-cast-in-lsh" based on 3.4. It includes a test.
Finally we don't change any unsigned int for int type for table_number_, key_size_ and multi_probe_level_.
However, as

in all the others types of IndexParams the types are int and not unsigned int

and the version of these three parameters in LshIndexParams of miniflann is int and not unsigned int,
I would suggest for consistency to switch to int for master, what is actually done by the "pev-fix-lsh-bad-any-cast" (one dash and not two after pev to easily identify between master based branched and 3.4 based ones)

…ndexParams()

pemmanuelviel · 2020-06-29T11:20:20Z

@alalek your forced push let the members declared as int instead of their original unsigned int type (the revert mentioned earlier concerning the few lines after 378).
I've pushed another commit (b0315a7) to change that on the branch, but it doesn't seem to appear in the PR.

alalek · 2020-06-29T11:36:22Z

@pemmanuelviel According to statement from #17684:

in all the others types of IndexParams the types are int an not unsigned int

lets change (keep from original patch) "int" type for fields.

I believe we are ready to merge this PR in the current state.

pemmanuelviel · 2020-06-29T11:54:20Z

@alalek I was not sure you wanted to keep the change.
So yes, we can merge this PR without my latest commit b0315a7.

Talking about consistency between uint and int, what do you think about my suggestion to merge PR 17628 on master to change also the LshIndexParams arguments type so they become int

like for others xxxIndexParams,
and are identical to the ones of the LshIndexParams in miniflann.hpp (l.121) and miniflann.cpp (l.276)?

alalek · 2020-06-29T11:58:45Z

PR on master (#17628) will be closed after merging this PR and after regular merge of fixes from 3.4 to master branch.

Which type is currently used in upstream flann project for smilar fields in xxxIndexParams?

pemmanuelviel · 2020-06-29T12:48:28Z

@alalek
All of them, except LshIndexParams, are using signed integers in their respective header files:

CompositeIndexParams(int trees = 4, int branching = 32, int iterations = 11,
HierarchicalClusteringIndexParams(int branching = 32,
KDTreeIndexParams(int trees = 4)
KDTreeSingleIndexParams(int leaf_max_size = 10, bool reorder = true, int dim = -1)
KMeansIndexParams(int branching = 32, int iterations = 11,

And for their mapping in miniflann, even LshIndexParams use signed integers.

alalek · 2020-06-29T19:59:13Z

@pemmanuelviel Thank you for information! Actually my question was about int table_number_ fields from LshIndex.

alalek

Thank you for contribution!

pemmanuelviel · 2020-06-29T21:13:06Z

@alalek Each type of tree has its own set of parameters, with some equivalences for some of them.
Thus "table_number" is the equivalent for LSH of "trees" for KDTree and "branching" for KMeans and HierachicalClustering.

alalek · 2020-06-29T23:19:22Z

Third commit is omitted due to GitHub todays incident.
PR is merged with commit cdac7c7

https://github.com/opencv/opencv/pull/17640/commits/b0315a74a6a91b1446ee56a47b67fe6e675b95ec

This commit does not belong to any branch on this repository.

Still broken.

PR is not marked as merged automatically due GitHub malfunction. Closing...

alalek reviewed Jun 25, 2020

View reviewed changes

alalek reviewed Jun 27, 2020

View reviewed changes

pemmanuelviel added 2 commits June 28, 2020 19:51

Fix the 'cvflann::anyimpl::bad_any_cast' error using Lsh

fe09c79

Add test checking we don't throw when creating GenericIndex with LshI…

cdac7c7

…ndexParams()

alalek mentioned this pull request Jun 28, 2020

Pev fix bad any cast in lsh #17684

Closed

6 tasks

Set the members type as unsigned, as they were before the fix

b0315a7

alalek approved these changes Jun 29, 2020

View reviewed changes

opencv-pushbot pushed a commit that referenced this pull request Jun 29, 2020

Merge pull request #17640 from pemmanuelviel:pev--fix-lsh-bad-any-cast

a84afb6

alalek mentioned this pull request Jun 29, 2020

Merge 3.4 #17697

Merged

pemmanuelviel deleted the pev--fix-lsh-bad-any-cast branch July 2, 2020 18:46

This was referenced Aug 12, 2020

ios use stitching crash libc++abi.dylib: terminating with uncaught exception of type cvflann::anyimpl::bad_any_cast #9573

Closed

stitch image error:libc++abi.dylib: terminating with uncaught exception of type cvflann::anyimpl::bad_any_cast #8018

Closed

Uh oh!

Conversation

pemmanuelviel commented Jun 23, 2020 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

alalek Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

pemmanuelviel Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

alalek Jun 27, 2020

Choose a reason for hiding this comment

Uh oh!

pemmanuelviel Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pemmanuelviel commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Jun 29, 2020

Uh oh!

pemmanuelviel commented Jun 29, 2020

Uh oh!

alalek commented Jun 29, 2020

Uh oh!

pemmanuelviel commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Jun 29, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

pemmanuelviel commented Jun 29, 2020

Uh oh!

alalek commented Jun 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pemmanuelviel commented Jun 23, 2020 •

edited by alalek

Loading

pemmanuelviel Jun 28, 2020 •

edited

Loading

pemmanuelviel commented Jun 29, 2020 •

edited

Loading

pemmanuelviel commented Jun 29, 2020 •

edited

Loading