Pull Request for Computational Photography

[Siddharthk]

All 3 modules added

[kirill-korniakov]

@Siddharthk, the pull request looks impressive! First of all please check the build errors: http://pullrequest.opencv.org. Secondly, am I right that you're not participating in the OpenCV's GSoC project?

In any case, for such giant contributions we propose to create a separate extra module in the opencv_contrib repository. This way we'll have some additional time for beta-testing. And if the module is mature enough, we'll add it to the main OpenCV repo, hopefully before the 3.0.

So, do you think you're ready to create a separate extra module? It could be named photo_extra, or something. Otherwise, it may take a lot of time before we are ready to get this code, since it should be tested and maintained in the future. Of course if you have a mentoring person from OpenCV team, it will simplify the process. But even the core development team will now put some code to the opencv_contrib repository, in order to make the maintenance process simpler...

[Siddharthk]

I am going through the build errors. Actually Gary hired me as an OpenCV summer intern. My mentor is Kris who has closely monitored my work. I will first try to remove the build errors.

[vrabaud]

Hi, I can confirm that @Siddharthk was working for us but not as an official GSOC (Gary managed to get that going). The pull request is indeed big but high quality for the results (we need to check the code too). It should probably be put in opencv_extra for now as you mentioned but considered very seriously for a quick merge: it really strengthen the abilities of the photo module, examples here: http://siddharthkherada.wordpress.com/

BTW, could we have the Microsoft Windows errors in English please ? That could help debugging ;) Thx !

[kirill-korniakov]

Well, OK, you can continue to update this pull request. I agree that this stuff is really important for the photo module, since right now we have only a couple of functions in it.

But I'm afraid that we can't help with Windows errors, since the Visual Studio is in Russian, and we simply don't have the English version. I agree this is stupid, but I believe that Vadim knows about this issue, and you can discuss it with him...

[StevenPuttemans]

Nice and impressive pull request! Will be glad to see this included in the openCV repository one day :+1:

[Siddharthk]

@kirill-kornyakov, I am facing one problem.

http://build.opencv.org/pullrequest?page=1

This page is showing only the topmost 10 pull request(1 to 5 toggle page is also not working). I am not able to see my merge status details(when I click on the details button). So now when I commit, I am not able to see the status of my PR( i.e which platform builds are successful). Am I doing something wrong?

[StevenPuttemans]

@Siddharthk I have the same problem using firefox, but surfing to http://pullrequest.opencv.org gives me full functionality. This might help?

[Siddharthk]

@StevenPuttemans, This works. Thanks :)

[StevenPuttemans]

:+1:

[kirill-korniakov]

@AnnaKogan8, please review and help with the build issues if you can. Thanks!

[AnnaPetrovicheva]

Hello @Siddharthk! Wow, what a great work have you done! OpenCV would certainly benefit a lot from this functionality! :) Firstly, to make tests run correctly, you need to push opencv_extra not as a part of this pull request. You should rather create in your fork of opencv_extra (https://github.com/Itseez/opencv_extra) a branch called "master" (like the branch for which you've done a pull request) and move all testdata there. No pull request in opencv_extra is needed! When testing, OpenCV BuildBot will automatically look into your extra repo and treat the branch with same name there as a part of this pull request, putting it in a right place for tests to see their testdata. When this pull request is merged, everything will be done automatically too. I will try to investigate the reasons for other failures and report if I found something.

[Siddharthk]

@AnnaKogan8, Thanks :)

I was not able to commit the changes on github due to some LAN issues in my hostel. Hopefully it will be back by tomorrow. I will do the changes as directed by you.

[Siddharthk]

@AnnaKogan8, I have forked the opencv_extra repository and moved all the testdata there. Still I am getting build errors. Can you please take a look at them?

[AnnaPetrovicheva]

@Siddharthk, situation is as follows:

• MacOS builder produces failure on test Photo_SeamlessClone_colorChange.regression. There are no hints why this happens, so I suggest inserting logs there. Right after you add new commits to your branch, builders rerun, so you can try remote debugging using logs. BTW, the way you check accuracy of results in tests isn't like it's done in other OpenCV tests, so please, rewrite them a little. OpenCV has a magic function SANITY_CHECK(Mat data), which automatically checks whether data is as it is supposed to be. So you should delete imwrite in the end of the tests and replace it with SANITY_CHECK for result matrix. Having that done, you should run performance tests from all the photo module with --perf_write_sanity (only one first time, then no extra flags are needed). This will write your matrix data (well, not all matrix - only some of its values to compare with) into opencv_extra/testdata/perf/photo.xml. You should commit this file into your opencv_extra fork. Tests that have SANITY_CHECK allow to check both regression and performance.
• Android builder has veeery strange issue - internal compiler error. I tried to find out information on how to fix it, but no results yet. Would you please give me rights to write in your repo in order for me to try to fix it?

[kirill-korniakov]

Would you please give me rights to write in your repo in order for me to try to fix it?

No need for that, you can clone the branch, and make your own pull request. This way you don't need write permissions...

[AnnaPetrovicheva]

@Siddharthk, please, revert the last commit - it produces merge conflict. As @jet47 pointed out, the issue on Windows builder is caused by extra header, so it seems there's no need in new dependency on video if highgui is no longer included.

[jperezrua]

Have you find any clue with the Android error?

[Siddharthk]

@juanmanpr Nope...

[AnnaPetrovicheva]

@Siddharthk, will try to investigate the reason. Good news: it seems I found the reason for Android builder failure - it is about STL linking. Will try to find a solution ASAP.

[Siddharthk]

@AnnaKogan8, Windows build is successful (replaced "while" with "for" loop). Now only Android error remains to be removed.

[AnnaPetrovicheva]

@Siddharthk, the Android builder failure is caused by the flag -DANDROID_STL=stlport_static. There is a possibility that we can use gnustl instead of stlport in the Android builder of BuildBot - then the problem is solved, but this decision hasn't been made yet. The code should work even with stlport, anyway, but it seems it's not your fault it doesn't. There may really be a bug in gcc, and we can submit a bug report there, as is suggested in failure message. But in that case we would have to wait until the next gcc compiler release to merge a pull request.

[Siddharthk]

@AnnaKogan8, Let me know whatever you decide. @juanmanpr is also getting the same error for Android platform. Also after the latest commit, I am getting warnings for Lin platform. Can you please check it.

[asmorkalov]

Fails on Android platform were cased by bug in gcc 4.7 in Android NDK. I've updated NDK up to r9.

[jperezrua]

@asmorkalov, Does that means it should work now? I am also having the same error. Do I have to push another commit to test the compilation again?

[AnnaPetrovicheva]

@juanmanpr, this error is only possible with gcc 4.7 version. On Android builder now NDK is updated, but still the old version is used (as cmake says), but it's a simple infrastructure problem. As soon as NDK r9 is used in Android builder cmake, the problem will dissapear:)

[Siddharthk]

@AnnaKogan8, What should we do now? Do we have to change anything or will the error disappear after updated NDK is used?

[AnnaPetrovicheva]

@Siddharthk, we just have to wait until Android builder is reconfigured: I hope, it will be done in a couple of days:)

[jperezrua]

Hi, maybe this #1527 can be interesting for you, I don't exactly understand how the last commit solved the problem, but now this is already compiling on Android.

[AnnaPetrovicheva]

Yes, like in #1527, replacing STL calls with simple non-STL function will help. @juanmanpr, as we discussed, the Android builder failure is caused by gcc bug which pops out because of use of stlport library.

[Siddharthk]

Ok. I will replace the STL sqrt() and pow() functions.

[jperezrua]

Thanks @AnnaKogan8, I didn't realize you had had identified the root of the problem.

[AnnaPetrovicheva]

@Siddharthk, no need in replacements - #1536 is targeted to solve our issue:) As soon as it's merged, Android builder will finally become green. Meanwhile, I've started reviewing your code. Overall, the work you've done is amazing, and I like the new functionality and the way you implemented it a lot, but I have quite a few remarks.

[AnnaPetrovicheva]

@Siddharthk, I can admit that your code, though implements extremely valuable functionality, isn't yet understandable enough to be put in photo module of OpenCV. The main reason is that it breaks some major OpenCV code conventions. That will make it extremely hard to maintain the functionality in future for anyone (except you:), and it could be finally lost for that reason. The main weakness of the contribution is that in most cases instead of using OpenCV methods it contains self-written analogs (like for copyTo, split, boundingBox methods), that are considerably slower and harder to read in the code. Just replacing them with corresponding methods from OpenCV will surely make the code significantly better (and readable) :) Second remark is about variables naming. It just should be more meaningful than now:) Further, less important thing is to shorten the functions. It is widely acceptable that one function should take one screen height to list, it's OK if it takes two or three, but no more. That also makes the code readable for others.

There are two options for us now:

• Improve the contribution. I myself will definitely help you to work on code and point out the very places that don't meet OpenCV code conventions and suggest solutions.
• Push it to opencv_contrib module just as is. Fewer people actually use it, and such way functionality will probably never be broadened, but it will be in OpenCV releases.

So, it's up to you to decide:)

[Siddharthk]

@AnnaKogan8, I have started making changes to the code as suggested by you. We should definitely go with the first option and improve the contribution. First of all, I will go through the codes once again and try to replace them with the OpenCV methods. Give me a few days to do this and then we can discuss what changes need to be done further.

[AnnaPetrovicheva]

@Siddharthk, that's really great! I will provide any needed help and review:)

[Siddharthk]

@AnnaKogan8, I have made changes to the decolor module. You can look at the files contrast_preserve.cpp/hpp. I am thinking of completing one module at a time. So please go though the code once and let me know if it needs more changes.

[AnnaPetrovicheva]

@Siddharthk, decolor module seems fine for me now! Really impressive progress:)

[kirill-korniakov]

But please note that all builders are now red because of the merge conflict...

[Siddharthk]

@AnnaKogan8, I have made changes to the cloning module. You can look at the files seamless_cloning.cpp/hpp. I tried using sobel to find gradient in x and y direction but it is not giving the desired output. So we should not change that function. Apart from this, I have made a lot of changes to the code. Please go through the code once and let me know if more changes are required.

[AnnaPetrovicheva]

@Siddharthk, sorry for the delay - will review cloning module ASAP.

[AnnaPetrovicheva]

@Siddharthk, everything is very good except for some minor remarks i listed above.

[Siddharthk]

@AnnaKogan8 , I have made changes to the npr module. You can look at the files npr.cpp/hpp. Let me know if more changes are required. I have also made changes to the cloning modules as suggested by you.

[mdim]

@AnnaKogan8 next step? still merge problem..

[Siddharthk]

I think the merge problem is due to the HDR module incorporated into OpenCV(under photo module). Should I update my forked repo? But I think in updating also there might be merge problems.

[AnnaPetrovicheva]

@Siddharthk, thank you for the update! Concerning the merge failure: it's because modules/photo/doc/photo.rst documentation file in the PR disagrees with corresponding file in repo. @Siddharthk, please check whether this file in your branch contains everything that is in master OpenCV branch now - it may have changed last month.

[Siddharthk]

Yes, this is the problem. But if I directly update my forked repo, then it will also give merge conflict(as my local repo is changed). So first I have to remove the photo module from my local repo and then update it. But I will try to find a better way to do this using git commands.

[Siddharthk]

@AnnaKogan8, I have removed the merge conflict. Android build is also successful. But there are build issues in Mac. I will try to find the solution.

[AnnaPetrovicheva]

@Siddharthk, the issue there is caused by pictures in opencv_extra repo. I guess rebasing to the last repo state should help.

[Siddharthk]

@AnnaKogan8, When I am running samples/cpp codes, I am getting the following error: g++: error: rt: No such file or directory g++: error: pthread: No such file or directory g++: error: m: No such file or directory g++: error: dl: No such file or directory

Before update it was working fine. I got this fixed. In opencv.pc, I changed "rt pthread m dl" with "-lrt -lpthread -lm -ldl".

But now I am getting the following error: ./a.out: error while loading shared libraries: libopencv_bioinspired.so.3.0: cannot open shared object file: No such file or directory

[Siddharthk]

There are some problems with cmake. Its not able to find gtk+ 2.x. I have "libgtk2.0-dev 2.20.1-0ubuntu2.1" installed. Therefore not able to use GUI also. Are there any fix? All these problems are coming after the update.