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!

[berak]

ord() only exists in python (and imho, no further conversion needed here)

[StevenPuttemans]

Hi berak. Discovered that C indeed does not have the ord function. As to the conversion. I read somewhere that 'char' gives problems when placed in an into when values larger then 127 occur. Not sure why though!

[StevenPuttemans]

All fixing done!

[StevenPuttemans]

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

[terfendail]

👍

[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?

[alalek]

Original PR fixes warning (compare int and char), which is added back by changes like these.

[StevenPuttemans]

But why would this generate a warning? Buildbot builds samples and there is not a single one out there. If c is an int, then comparing it to 27 or to any other numerical value should work just fine in all C++ standards. Could you elaborate where this could go wrong?

[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.

[apavlenko]

@StevenPuttemans , @alalek don't you think that after making c of type int this comparison can be true or false depending on the signed or unsigned char type default? (I mean the case when the symbol code is more than 127? for 'd' it will work anyway!)
I would prefer leaving char type for c...

[StevenPuttemans]

Hi @apavlenko, I guess I am missing what you are trying to say here. Could you explain where you would like in INT cast to ensure that we have no loss of values above 127? Or am I completely missing your point?

[apavlenko]

I mean that the int key = waitKey(0) & 0xFF; is always non-negative, while char constant ('*') can be negative when the char type is signed and the symbol ASCII code is more than 127 - do you agree?
But in reality people will unlikely use regional-specific symbols is such case, so the issue is minor.

[StevenPuttemans]

Hmm I get your point and agree, but isn't it best to serve a solution that tends to mass?
Maybe we should add a notice somewhere covering this special case?

[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 💃

[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: #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!

[StevenPuttemans]

@terfendail in case like this, do we also prefer use of waitChar?

[terfendail]

It looks for me as a "press any key" wait so may be we even doesn't have to check the last significant byte only.

[StevenPuttemans]

Well yeah it does, but ofcourse, then they could simply have used waitKey() right? maybe I should dig into the actual sample to get an understanding of that DELAY parameter

[terfendail]

Processing looks like some animation with DELAY time between frame changes which could be immediately halted by pressing of any key.

[StevenPuttemans]

Hmmm in fact the 0xFF here is not needed, simply because it has to be any key. Will remove the 0xFF in these cases to not make the code more complex then needed. Basically waitChar should be used if 0xFF is needed, else the 0xFF can go!

[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 CV_KEY_ 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 CV_KEY_ 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 💃

[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: 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 (#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?

[vpisarev]

@StevenPuttemans, we've discussed this problem and the proposed solution with the team. The proposed solution is to modify cvWaitKey()/cv::waitKey() behaviour so that it always applies & 255 before returning the value and also add another function, like cvWaitKeyEx()/cv::waitKeyEx() or such that would return complete available return code. Right now for cvWaitKeyEx() we can simply use the existing cvWaitKey() implementation, and cvWaitKey() can be implemented as:

int cvWaitKey(int delay) { return cvWaitKeyEx(delay) & 255; }

Later on cvWaitKeyEx() can further be modified (without changing API) to return some universal, cross-platform codes for arrows and other special keys regardless of backend.

Rationale and benefits of the proposed solution:

1. it's binary compatible with existing OpenCV
2. 99% of the samples will continue to work properly.

the downside is that 1% of samples/apps that react on special keys will need to be modified, but that's should be easy to detect and fix.

What do you think? If you agree, shall we close this PR and submit a smaller one with the proposed fix?

[StevenPuttemans]

@vpisarev I guess I can agree to that if the whole team is behind that decision! However, I am about to leave for a CV conference until next monday, so if anyone has the time to provide the supposed fix in the meanwhile, do go ahead!

[vpisarev]

thanks! have a nice trip to conference!

[StevenPuttemans]

@vpisarev thanks! The conference was quite nice. Kind of a busy week now, but the week after that should contain time to provide the fix!