add single scale detection capability

[StevenPuttemans]

What does this PR change?

In the old days, there was support for the detectSingleScale functionality on cascade classifiers. Nowadays those functions are removed and people keep asking how they can add a single scale detector on a predefined scale.

This PR adds the 3 detection methods on a fixed scale, wrapping the detectMultiScale functionality correctly and hopefully solving the stormload of questions on this topic.

[StevenPuttemans]

@alalek some help appreciated here

It seems that for ABI compatibility, we cannot add extra virtual functions, but the whole cascade classifier interface is written like that. How can I avoid changing the 0.5% difference?

[Dikay900]

got the same problem at #5362 and I could not get along the abi compatibility... so the initial fix couldn't get merged...

Does not seem like there is a way around i think. I fiddled around a bit with the PR last time...

[StevenPuttemans]

Well true except for the fact that this is complete new functionality... I am not changing existing stuff... so not sure why this would break ABI

[Dikay900]

I also wanted to add a new virtual function, but dropped it again so it ended up be a small fix instead of fixing the bug entirely. The virtual table which will be created at compile time is also optimizable by the compiler (to maybe remove an entry in the virtual table) replacing it with the correct static call if the compiler can be sure of that. That will result in a faster call since every virtual call must get looked up in the virtual table.

So all in all i don't think we cannot change anything in the virtual part of the code without adding a complete new class which cannot be changed again after the first implementation of virtual functions.

[StevenPuttemans]

So we should just give the class a complete overhaul and remove all the virtual shizzle!

[Dikay900]

Well these classes are programmed in a robust way and using base classes is (in most cases) much easier to maintain once the structure is analysed by the maintainer. With the ABI Compatibility the whole design is just killing itself. Maybe with the inclusion of template-based, header only parts of opencv, if this is getting started, this design should again be applicable but maybe we will run even deeper into the ABI Compatibility nightmare. :imp:

[StevenPuttemans]

I am just wondering why OpenCV needs to guarantee ABI compatiblity?

[Dikay900]

well it sure was a decision by the team and with every decision you have pros and cons. We are definitely feeling the cons here :/ I think they decided that with the still growing userbase providing a stable interface is much more important. With ABI compatibility you can simply exchange your library without modifiying (and recompiling) your own application using opencv. You are immune to changes with a stable interface while getting internal bug fixes of the library and performance improvements. Comfort for the user, complexity for the developer. For everyday users its definitely the better way to have ABI Compatibility.

In comparison you can just look at how the ffmpeg interface is constantly changing and there is always something to adjust to have all the functionality for all the ffmpeg versions. They don't have ABI compatibility but the user wants to use opencv with ffmpeg in the same way in every version so the diversity of ffmpeg versions is made compatible by the wrapper here in opencv.

edit: i wonder how many users even try to update a library without recompiling it.

[paroj]

you can add new virtual functions, but those have to be defined at the end of the class. This is UB-ish, but usually works.

[StevenPuttemans]

Okay will have a look into that!

[StevenPuttemans]

@paroj actuall I just defined the new virtual functions at the end of the class without success. It still breaks down ABI.

[StevenPuttemans]

@Dikay900 I can agree that each decision has its pros and cons, but what you should absolutely avoid in an open source, community based, software library is in my view, that people cannot add functionality where they actually need it.

I understand that it might give benefits to depending libraries, but on the other hand, I am pretty sure that our largest user base consists of researchers, R&D teams, students, ... who actually don't mind the fact that ABI gets broken.

edit: I wonder how many users even try to update a library without recompiling it.

Actually that was my exact question also, I never noticed anyone doing so, so it might be nice to see a core dev come up with a good sample case of why this would actually be needed.

I mean, if you look for example to your PR, it simply fixes WRONG functionality... so I am not quite understanding why it is better to approach something incorrectly, then having to recompile the library ...

[Dikay900]

@paroj actuall I just defined the new virtual functions at the end of the class without success. It still breaks down ABI.

Thats because the compiler does the optimizations i wrote about. Sometimes it will work this way sometimes not...

I understand that it might give benefits to depending libraries, but on the other hand, I am pretty sure that our largest user base consists of researchers, R&D teams, students, ... who actually don't mind the fact that ABI gets broken.

exactly this but we have no insight which companies are in contact with the devs and are maybe using this in a productive way. The GPU part of opencv seems to be used quiet often it seems, since there were many changes in the 2.4 branch first thus there is probably some proprietary software which is still based on the 2.4 branch. But this is all speculation since as I already stated we have no in-depth knowledge as the devs probably have.

might be nice to see a core dev come up

I always appreciate some in-depth look as well. Would be great if a dev could give a statement here. :)

But it seems like it's currently a quiet time (or maybe even a busier time because of GSOC?) since there was no update of the dev report summary in the wiki for a while now.

[paroj]

actuall I just defined the new virtual functions at the end of the class without success. It still breaks down ABI.

they also must not be pure virtual. define them as virtual foo() {}

[paroj]

The virtual table which will be created at compile time is also optimizable by the compiler (to maybe remove an entry in the virtual table)

nope. the compiler might replace a virtual function call (function pointer dereference) with the actual function if it can deduce it at compile time, but the vtable itself can not be changed.

[Dikay900]

Ok if this is the case that would be great, unsure if I tried this in my PR. Maybe I misunderstood the optimizitation.

edit: if i remember correctly the code did not compile if it wasnt pure virtual but unsure about that lets see what the buildbot will say

[paroj]

there will be still a minor ABI break - namely if user code derived from BaseCascadeClassifier and added new virtual functions. But this is probably something that the OpenCV devs can let pass.

[alalek]

@StevenPuttemans ABI workaround is here: https://github.com/alalek/opencv/tree/pr6523

Could you add some tests for this?

[StevenPuttemans]

Guys, I was in a meeting all morning :) Give me the time to provide the necessary fixes and I will get back to you! :) And by the way, thank you all for the interest!

[StevenPuttemans]

@alalek your solution is to add a new virtual class part, which I get, but which I find quite destroying the nice layout of the code. I will copy the changes and let you know if it works! As to tests, never did that before but I will give it a try!

[StevenPuttemans]

to all of you, first tried suggestion of @paroj right now, because that is for me, a nicer solution then the one proposed by @alalek. If it does not work, I will go for his solution!

[alalek]

Test will show that my workaround is not completed. I miss one thing:

-class CascadeClassifierImpl : public BaseCascadeClassifier
+class CascadeClassifierImpl : public BaseCascadeClassifier2

[StevenPuttemans]

@alalek noted! Thank you!

[StevenPuttemans]

@paroj as said, the ABI incompatibility drops from 0.5% to 0.2% but it does not get completely fixed this way. Just running a last test, with a minor change, but I am expecting that it will never yield a 0% incompatibility.

[StevenPuttemans]

Okay last one did not work also! Going for the suggestion of @alalek!

[StevenPuttemans]

@Dikay900 might this workaround be something you could do also?

[Dikay900]

Yes this seems to work in general so i could adapt this but when this will get merged we need BaseCascadeClassifier3 if we add something to the Base class again. If i got some time i will rework my PR.

[StevenPuttemans]

And what if I simply wait and integrate your suggestions with mine? Then a third wrapped class wont be needed

[alalek]

BaseCascadeClassifier3

This should be incremented with official release only (we update ABI files in this moment).

[StevenPuttemans]

Still need to fix 2 documentation warnings. Will do so in an hour!

[Dikay900]

After reading your posts i did ask myself if the ABI will created again after each PR or will the ABI files only get generated after every major release? If so my changes could easily be added in the extra class you created here?

[StevenPuttemans]

Oh jeez it actually passed all checks! GREAT! I will just adapt the last discussion on the parameter set of the detectSingleScale functions before allow it to get merged.

@Dikay900 As far as I understand, you can now simple use the BaseCascadeClassifier2 class to overwrite everything you want without breaking ABI compatibility.

[StevenPuttemans]

@alalek just got full green screen :+1: , merged last commits so this can get merged once it is green again :)

[StevenPuttemans]

@alalek all fixes merged, all green lights, even solved the buildbots failing. Could you merge this one? Also, wondering if we are going to keep backporting functionality. What are your thoughts about it?

[alalek]

Looks good to me! But I believe @vpisarev should take a look on this.

[StevenPuttemans]

Okay fine :D @vpisarev still has some other PR's to review under his name. I will poke him until he gets back to us :D

[StevenPuttemans]

@vpisarev can you confirm that this PR is okay and maybe merge it in? I am using this functionality quite exhaustively for the moment and have 2 companies waiting on a release with this included :)

[vpisarev]

this PR will be recreated because of CI issues