fixing waitKey commands to be universal

[StevenPuttemans]

As a follow up for the discussion held in PR #7098, and the discussion started in this topic, we decided to implement a waitChar function that always returns the correct value if the returned value is needed for further comparison to ASCII codes of char inputs.

[StevenPuttemans]

Hmm seems it is not working like expected ... Will take a look at it beginning next week and report back!

[StevenPuttemans]

All fixing done!

[StevenPuttemans]

Oops - kind of screwed stuff up it seems :( my ord fixes are gone. Will reapply them.

[terfendail]

:+1:

[StevenPuttemans]

@terfendail I guess you are new to the team? If so welcome! Haven't seen you around yet!

[alalek]

I believe it will be good to reduce magic code. May be we could add some wrapper and use them:

static inline
char cv::waitChar(int delay = 0)
{
    return (char)(cv::waitKey(delay) & 0xff); // works fine with the most part of printable chars on many platforms
}

[StevenPuttemans]

I agree that the magic code is maybe a bit weird, but people are so used to using the waitKey interface, as it is documented everywhere on the wide web, that adding a new function will only raise more questions in my opinion. It will also generate a flood of questions in my experience

I guess adding a function specific for this is a bit overkill?

[StevenPuttemans]

Just destroyed the extra addons by @terfendail but will add em back manually. Gimme a sec!

[StevenPuttemans]

@alalek I just continued some research about this. I have locally built OpenCV + OpenCV examples, with noisy warnings enabled, so that basically everything gets outputted. When checking up on the output, there wasn't a single issue reported on comparing ints and chars in this way.

Neither did building the samples manually inside a IDE trigger any warnings.

Can you tell me how I could reproduce those warnings? Or am I correct to say they do not matter for C++ compilers?

[StevenPuttemans]

Oh btw, the only, somewhat relevant error I was able to dig up the WWW, is when comparing an integer to a char pointer. But since we have no pointer comparisons here, those risks are not the case here.

[alalek]

I assume that author of #7098 capture some noise warning about this, possible from some static analyzer.

[StevenPuttemans]

@yyyhssss Can you please explain how you got those noisy errors and check if my version of the code still generates those? Because I am not finding any way of generating those.

@alalek But if the most common compilers do not cover this issue, shouldnt we then keep everything universal? I am afraid this is only applicable in some very perticular cases.

[StevenPuttemans]

Just to add, the static analyzer cppcheck does not report anything on Ubuntu 14.04 when testing a sample.

[StevenPuttemans]

@terfendail it seems you made a remark but it is not showing on github. Can you readd it?

[terfendail]

My previous comment adds nothing to @apavlenko reply that's why I removed it.

Existing discussion more an more persuade me that separate function as suggested by @alalek would be the best decision. It could be a place to describe possible issues and it could be tuned as necessary to avoid as much issues as possible. May be we could extend it to handle signed/unsigned char option

static inline
char cv::waitChar(int delay = 0) // works fine with the most part of printable chars on many platforms
{
#if CHAR_MIN==SCHAR_MIN
    int intKey = cv::waitKey(delay) & 0xFF;
    return (char)(intKey & 0x80 ? intKey - 0x100 : intKey);
#else
    return (char)(cv::waitKey(delay) & 0xff);
#endif
}

@StevenPuttemans @apavlenko @alalek What's your opinion on this?

[StevenPuttemans]

I do get that we need a universal solution, but already explained above why I am not in favor for new functionality to wrap this. That being said, it kind of makes sense to change all this to make it more uniform, just be prepared for a shitload of hey its not working anymore questions and issues :dancer:

[StevenPuttemans]

Also, I will hapily implement the suggestion, but probably be by the beginning of next week. Got a deadline to catch here ;)

[StevenPuttemans]

@terfendail I guess the reason why this is crashing, is due to some master samples being update in the time between?

[StevenPuttemans]

As I was afraid: https://github.com/opencv/opencv/pull/6945 introduced changes that shifted lines in the docs... I am not sure what would be the fastest way to solve these conflicts without all the edits did by me...

[StevenPuttemans]

I squashed commits and will now see if I can cherry pick that commit onto a new branch

[StevenPuttemans]

Did a cherry pick to a new branch on top of recent master, solved conflicts and changed name locally then pushed it back. Should work now!

[StevenPuttemans]

Okay so applying the suggested static inline function results in In file included from /build/precommit_linux64/opencv_contrib/modules/sfm/src/libmv_light/libmv/correspondence/nRobustViewMatching.cc:21:0: /build/precommit_linux64/opencv/modules/highgui/include/opencv2/highgui.hpp:355:32: error: explicit qualification in declaration of 'char cv::waitChar(int)' char cv::waitChar(int delay = 0).

Will take a look at possible solutions!

[yyyhssss]

hello Steven,

sorry for my late reply because i never expect any response of this issue... i run this on Ubuntu 16.04, here is my code and result. there is no error during compiling or building. but it doesn't exit by pressing q or Q without truncation.

and in your suggestion of "while(key != ord('q') && key != ord('Q')", what's the function of ord?

At 2016-08-16 20:18:32, "Steven Puttemans" notifications@github.com wrote:

@yyyhssss Can you please explain how you got those noisy errors and check if my version of the code still generates those? Because I am not finding any way of generating those.

@alalek But if the most common compilers do not cover this issue, shouldnt we then keep everything universal? I am afraid this is only applicable in some very perticular cases.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[StevenPuttemans]

@yyyhssss as you can see, we moved into a different direction and made a function that avoids this issue, named waitChar(). As to the ord thing, was something from python and doesn't exist in C++ backend.

[StevenPuttemans]

@terfendail is the error generated on the python side

/usr/bin/ld: CMakeFiles/opencv_python2.dir/__/src2/cv2.cpp.o: relocation R_X86_64_PC32 against undefined symbol `_Z13pyopencv_fromIcEP7_objectRKT_'

can not be used when making a shared object; recompile with -fPIC` an issue related to our code adaptation? Or is the buildbot acting up again?

[StevenPuttemans]

@sovrasov It seems you did quite the fixes on the python tutorials, so asking your opinion on the python part. If I get it correctly, porting the waitChar() function using CV_EXPORTS_W ensures that the use of ord() is no longer needed, therefore I removed them.

Could you confirm this or point me to someone who can?

[berak]

undefined symbol `_Z13pyopencv_fromIcEP7objectRKT'

there is no pyopencv_from() and pyopencv_to() for signed char (yet) in modules/python/src2/cv2.cpp (missing template specialization for that type)

[edit]: src2.cpp, not gen2.py

[StevenPuttemans]

there is no pyopencv_from() and pyopencv_to() for signed char (yet) in modules/python/gen/cv2.py (missing template specialization for that type)

Oh damn, hit a wasps nest again. If someone could help me out with this, feel free to do so! My experience with Python is quite limited...

[StevenPuttemans]

Btw @berak I guess you are pointing to modules/python/src2/gen2.py on the master branch?

[berak]

yes, master, but cv2.cpp, not gen2.py

have a look at cv2.cpp:544 and just add:

template<>
PyObject* pyopencv_from(const char& value)
{
    return PyInt_FromLong(value);
}

[StevenPuttemans]

Ok so basically it runs all smooth now. @terfendail how could we expand waitChar() to always return a -1 when the timer runs out? This is the one thing I would prefer instead of letting it be system specific...

[alalek]

BTW, "char" type can be unsigned on some platforms, so there is no chance to return -1 in general way.

[StevenPuttemans]

BTW, "char" type can be unsigned on some platforms, so there is no chance to return -1 in general way.

Okay not a general way, but taking a look at the code, we already check if signed and unsigned on the CHAR definition are equal to define if the system has signed or unsigned char. So we could also provide solutions for each platform ensure that -1 is returned right?

[StevenPuttemans]

So with the following code, and having a CHAR_MIN==SCHAR_MIN system myself, it seems that running this code:

#include <iostream>
#include "opencv2/opencv.hpp"

using namespace std;
using namespace cv;

int main( int argc, const char** argv )
{
    Mat test = imread("/data/eyes.jpg");
    imshow("test", test);

    cerr << 'f' << endl;

    cerr << CHAR_MIN << " " << SCHAR_MIN << endl;

    #if CHAR_MIN==SCHAR_MIN
        int intKey = cv::waitKey(10) & 0xFF;
        cerr << "Finished waiting, returning key ";
        cerr << intKey << endl;
        cerr << (char)(intKey & 0x80 ? intKey - 0x100 : intKey);
    #else
        cerr << "Finished waiting, returning key ";
        cerr << (char)(cv::waitKey(delay) & 0xff);
    #endif

    return 0;
}

My system outputs perfectly fine the f when pressed, but when waiting 10 milliseconds this comes out

f
-128 -128
Finished waiting, returning key 255
�

which basically means that the 255 value gets scrambled into something untranslatable. For my kind of system this can be solved by adding a simple conditional loop.

if(intKey == 255){
   cerr << -1;
}else{
   cerr << (char)(intKey & 0x80 ? intKey - 0x100 : intKey);
}
   cerr << -1;
}else{
   cerr << (char)(intKey & 0x80 ? intKey - 0x100 : intKey);
}

Which outputs -1 if I wait enough. Can anyone check on the other type of system, if anyone is around, that this works for you guys?

[terfendail]

At the moment I see no way to return -1 on unsigned char platforms. Since the system expect that return value is unsigned it will treat every received number as positive regardless of the internal representation so it will never consider it as "-1". We could change function behavior to return either value we want but it have to be positive.

Of course there is decision to explicitly change return value to signed char but it will hurt value comparison for the whole high part of ASCII table(however it will be at least marked with warning by most of modern compilers)

[alalek]

What is about these return values:

• 0 - timeout (no key is pressed)
• 255("unsigned" char)/-1("signed" char) - no mapping to ASCII char (FN keys, arrows/navigation, etc). Also we can add macro define for this value, like CV_KEY_UNKNOWN/CV_CHAR_UNKNOWN which will be -1 or 255.
• 8,9,13,27,32-127 - it is regular ASCII symbols (CV_KEY_BACKSPACE, CV_KEY_TAB, CV_KEY_ENTER, CV_KEY_ESC, CV_KEY_SPACE)
• other values - are platform/backend/locale defined

Also we can swap first two positions (cv::waitKey returns -1 on timeout). Also we can change return type from "char" to "int" again to return to correct -1 value. But there is a question about comparison issue of "int"s and "char"s, which we can't observe/reproduce.

[StevenPuttemans]

Guys I want to wrap some things up here ... because basically this doesn't seem to lead to any final stage and it is starting to grow more and more like some sort of hidden monster inside OpenCV ...

So

1) Can @terfendail, @alalek and @apavlenko first come to a conclusion on the return value? I mean, I had everything forced as an integer, then @apavlenko suggested to go the char way, then @terfendail concluded that should be it, and now @alalek is pushing it back to the INT...

2) I proposed the suggestion about CVKEY enums already before to add more clarity and make it visually more appealing code, but it seems then that the majority of people following the discussion was against adding more enums just for that? Can we also come to a conclusion there first?

3) I want to come back to the issue of comparison char's versus int's which was the whole reason my first suggest PR was broken down... I did several tests, different machines, OS'es, compilers, debuggers and analytic tools, even set compiler to output every single error or warning out there, the small they are, and there wasn't a single time that comparing int to char resulted in an issue... If this is unreproducable, then why are will still taking this in account?

I get that we want to get an interface that suits everyone his needs, but by now I have overhauled the 100 files for about 10 times, and I am not going to do it further until there is a consensus on these points :) As far as I see it, the original PR, keeping the int enforcing and adding 0xFF overall seemed fine. I do get the value of a waitChar to remove those 0xFF because new users tend to get lost in those ... but why not simply do that inside the original waitKey() function instead of providing new functionality.

Let the discussion start!

[terfendail]

I definitely have to agree that this discussion looks like architecture debate rather than fine tuning of the final fix.

So from my point of view: 1) It would be better to retain char as return value. The most of updated cases looks like "get a pressed key character". Usually the code doesn't even bother about timeout return value and looks for specific character only. So for such cases it doesn't matter which return value will represent the timeout. There are a few cases where the code waits for any key press and doesn't check it against specific characters but here we could retain waitKey() function. 2) I don't think we have to define CVKEY enums since there already are escape sequences to represent most of useful control characters. 3) Comparison issue could be reproduced only on systems localized to specific languages if user wants to check return result against non-English alphabet. It is not the most frequent case since users usually tries to avoid it anyway due to probable codepage related issues, but it is possible 4) Masking of waitKey() return value with 0xFF conceal negative return values so there should be special timeout treatment added. I still consider separate function intended for this a better option since we retain for end user a way to access keyboard scan-code. Even if this value isn't a real scan-code and/or system/compiler/environment/etc dependent the user still could try to handle it in a more complex way that better suites the task.

[StevenPuttemans]

There are a few cases where the code waits for any key press and doesn't check it against specific characters but here we could retain waitKey() function.

I agree to this. And combining with your point 4, this seems indeed valid to keep the function.

... answer to 3the question

You completely lost me there :dancer:

[terfendail]

Regarding question 3 I've tried to notice that waitKey() is able to select key codes depending on selected input language. So if user select language which alphabet contains special characters(i.e. vowels with umlauts) or is completely different from Latin then return values for character keys will be greater than 127. As far as I know it is the only case for return values greater than 127 and so the only case to bother about char variable sign.

[StevenPuttemans]

I get the impression that the others kind of left the discussion ... I would still urge @alalek @apavlenko to get back and defend their positions before moving further.

@terfendail regarding to your last post. Indeed the only problem will arise with regional special chars. Therefore I suggest to limit the use of waitChar to the 0-127 range and add this as a not. It is fairly straightforward that programmers to not use regional specific chars, just to avoid their software not being managable outside their region, so I guess limiting it won't do so much harm. In that idea, we could just quit thinking about the whole signed/unsigned char discussion and move back to the int idea ... which seemed far more reasonable to me...

[alalek]

My another suggestion is to create a poll (or set of pools) via issues tracker with votes via "GitHub reaction" button. I'm not sure that all results will be fully accepted, but it is a great feedback anyway.

[StevenPuttemans]

I agree that polling the community for its opinion could help, but lets be straight here, how many people do we actually expect to reply? If I see how low the number of interactive users is on here, and on the forum, then how will we have a valid representation of the communities opinion?

[alalek]

but why not simply do that inside the original waitKey() function instead of providing new functionality.

There is related proposal how we can do this: https://github.com/opencv/opencv/compare/master...alalek:waitKey

It works fine on example with MSVC compiler on debug/release builds (and with/without "define").

[StevenPuttemans]

@alalek I kind of like your proposition. But we should drop the inline code. Inline code cannot be wrapped around for the python wrappers, which can be solved by seperating implementation and declaration into .hpp and .cpp files.

[apavlenko]

Sorry for keeping silence! Guys, my opinion is that we should leave the int waitKey() function returning scan-code as it is now and introduce another function (e.g. int waitChar()) returning -1 for the expired time and 0 - 255 values for the key pressed. Also we should update the samples to use the 2nd function when there is a difference.

[StevenPuttemans]

@apavlenko thanks for the reply!

@alalek, I noticed you are adding a PR yourself (https://github.com/opencv/opencv/pull/7190) with your suggested fix. This makes me wonder why I am keeping this open and even try to put in effort to get to a concensus? For now I will close it down, let you guys decide for what you want. If you want to use the branch to cherry pick changes from, go ahead, I will leave it open on my repo.

@terfendail It seems @alalek decided to test his approach, guess the discussion on how to proceed is between you guys now.

[alalek]

@StevenPuttemans #7190 is added to check Python bindings builds (with "inline"). It works fine (but has some ABI problems on Linux). I'm not going to update mentioned PR actually.

[StevenPuttemans]

@alalek I will get back to you with some pointers on the inline thing at the other PR

[terfendail]

So it looks like we've came to decision to retain waitChar() with integer return values 0 - 255 instead of char.

Decision Pros:

• Simplified timeout check
• Platform independent result.

Decision Cons:

• Simple matching to character literals could be performed only for low half of ASCII, however complicated platform dependent matching to localized high half of ASCII considered a rare case and therefore acceptable.

Have I missed something?