Skip to content
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

steps argument in Mat::new_nd_with_data() has a wrong type #201

Closed
jerry73204 opened this issue Jan 24, 2021 · 10 comments
Closed

steps argument in Mat::new_nd_with_data() has a wrong type #201

jerry73204 opened this issue Jan 24, 2021 · 10 comments

Comments

@jerry73204
Copy link
Contributor

The Mat::new_nd_with_data (doc) has a steps argument with type &size_t. I passed &0, the suggested default value, and it failed with the message.

Error: OpenCV(4.5.1) /build/opencv/src/opencv-4.5.1/modules/core/src/matrix.cpp:259: error: (-13:Image step is wrong) Step 7954887940465321323 for dimension 1 must be a multiple of esz1 4 in function 'setSize'
 (code: -13)

Look like it treats the reference as an integer. I guess the argument should have the type size_t,

@jerry73204
Copy link
Contributor Author

Also new_nd_vec_with_data has this issue.

@twistedfall
Copy link
Owner

twistedfall commented Jan 24, 2021

I think it expects a null pointer, not a pointer to null. Can you please try passing something like prt::null_ptr().as_ref() for steps? I’ll try to improve generation in this are in any case

@jerry73204
Copy link
Contributor Author

It won't work. pointer.as_ref() returns None when it is null.

@twistedfall
Copy link
Owner

You’re right! Well, some kind of mem::transmute should do the trick, but yeah, it needs some fixing.

@jerry73204
Copy link
Contributor Author

It is my workaround to pass null pointer as reference. Unfortunately, it passes the tests in in debug build but is killed by SIGILL in release build.

use platform_types::size_t;
let step: &size_t = mem::transmute(ptr::null() as *const size_t);
core::Mat::new_nd_with_data(&size, typ, tensor.data_ptr(), step)?

This is my implementation FYI.
https://github.com/jerry73204/rust-cv-convert/blob/46136573897146f1ea183772b4daeee5ffecab97/src/with_opencv_tch.rs#L347-L357

@twistedfall
Copy link
Owner

In my tests it literally generates the asm UD2 instruction starting with opt-level = 1. I suppose it has to do with the fact that references are guaranteed to be non-null in rust, so such transmute is instant UB. So it looks like there is no workaround. I'll try to make the fix asap.

@jerry73204
Copy link
Contributor Author

Interesting, TIL there is a x86 instruction causes undefined behavior.

@twistedfall
Copy link
Owner

Can you please check the rel branch and confirm it fixes the problem for you? The steps argument is now Option<&[size_t]> so passing None should work.

@jerry73204
Copy link
Contributor Author

Great. It passes every test even in release build.

@twistedfall
Copy link
Owner

It's now released in v0.49.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants