[GSOC 2019] Global sampling matting #2134

Nerdyvedi · 2019-06-03T16:39:52Z

Merge with extra: opencv/opencv_extra#669
relates PR: #2198

This pullrequest is the C++ implementation of the Global sampling method for matting.

Global sampling method for matting by Matting Kaiming He1, Christoph Rhemann2, Carsten Rother3, Xiaoou Tang1,Jian Sun.

This blog post gives a summary of the paper:

Input to the model is the image and a trimap, and the output is the alpha matte of the image.

Steps

Use the following command to run the program

g++ pkg-config --cflags opencv globalmatting.cpp pkg-config --libs opencv -o matting

This produces an executable file called matting

To run the executable file, use the following command

./matting <Path to input image> <Path to trimap> <niter>(number of iterations for expansion of known regions[optional])

Results

After evaluating this implementation on alphamatting.com, the results are almost as good as the original implementation.

Following were the results:

Error type	Original implementation	This implementation
Sum of absolute differences	31	31.3
Mean square error	28.3	29.5
Gradient error	25	26.3
Connectivity error	28	36.3

Some of the outputs with of this implementation are as follows :

Image	Trimap	Alpha matte(this implementation)

force_builders=linux,Docs,Win64,Mac,Win32

alalek · 2019-06-03T17:04:42Z

All modules names are lower-case. Almost all files should be lower case too.
Avoid 'delete something' commits in patch. The same note for 'merge' commits. Your patch should be clean as much as possible.

StevenPuttemans · 2019-06-04T07:18:47Z

@alalek thanks for the basic pointers

@Nerdyvedi I will get through the PR at the end of the week and give you weekly feedback on the status so far. Make sure the commit history is clean and understandable. You can easily squash commits locally using the rebase tool in git, and then make a clean history by force pushing your changes.

Overall, as discussed, lets first focus on getting the complete AlphaGAN and Global Sampling method ready, since the other matting student is focussing on the information flow paper as main contribution. We rather have your other 2 papers as robust implementations, thouroughly tested and even possibly with Python bindings.

This build bot overview page gives you a clear overview of the issues that might arise with your current code. Once the tests have been completed, any errors will pop up there.

In general, feel free to update you starting message with any tests / graphs / progress bullets / ... so we can monitor easily what has been done so far.

StevenPuttemans

First review

Since you have these blog posts, which you referred to in chat, add them as extra explanation in the topic description of this pull request. It helps people understand what you are doing.

I will comment in between the code for specifics, but will ask you to pay attention to these global things.

As commented previous by @alalek there is still a lot of capitals, spaces and dashed lines in the filenames. Remove them completely. For example the path modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py should be changed to modules/matting/alphagan/eval.py for example. You need clear and logical names and structures. And try to avoid to many submodules.
In general I am missing comments. Each function, even in python scripts should be well documented and steps taken explained, so that future people who will work on this understand what you did.
Comments made in one file should be imposed on all the others. I will not keep repeating myself in subsequent files and expect you to do this.
I understand you replaced torch by t for ease of use, but it reduces clarity. I would replace all t. instances by torch. instances so that it is very clear which parts come from torch package.
I do get that sometimes class names with a capital can create clarity, but is it always necessary. I try to avoid that as much as possible.
In general, remove all debugging and commented out pieces of code, unless they really have to be there. In that case, add a DEBUG condition switch at the top.
It seems that you have a lot of binary files. Data should go to opencv_extra as a separate pull request https://github.com/opencv/opencv_extra and be used as test data. @alalek is there any decent guideline on how people have to do this?
It seems that the code for the guided filter is already in OpenCV and multiple other parts is already in OpenCV and should thus not be in this PR, but properly addressed from the correct modules and thus the ximgproc module should be included there.

StevenPuttemans · 2019-06-12T11:47:22Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

+import torchvision.transforms as transforms
+import numpy as np
+import torch as t
+from model.AlphaGAN import NetG


Make sure that your import and from statements are clearly grouped. Avoid switching them in between so that people have a one view oversight.

StevenPuttemans · 2019-06-12T11:56:18Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

+from visualize import Visualizer
+import tqdm
+
+os.environ["CUDA_VISIBLE_DEVICES"] = '2'


Hard coding a GPU capable device cannot be done in a library. You will have to provide the user the possibility to assign which GPU he wants to use.

StevenPuttemans · 2019-06-12T11:57:21Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

+    transforms.ToPILImage()
+])
+
+MODEL_DIR = '/data1/zzl/model/alphaGAN/new_trainset/netG/netG_40.pth'


Same with hard coded paths, I prefer a python file that accepts input arguments and then on top of this file in comments you describe what the file does and what input it expects. This allows more versatile use of your code.

StevenPuttemans · 2019-06-12T12:00:05Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

+        padding_result = np.pad(img, ((0, h_padding - h), (0, w_padding - w), (0, 0)), 'mean')
+
+        return Image.fromarray(padding_result), int(w_padding/320), int(h_padding/320)
+    elif len(img_size) == 2:


If you look at the two conditions, there is common code in retrieving w and h padding. You could make a coditional check on the shape first and then move this code outside the condition, only needing this check once.

StevenPuttemans · 2019-06-12T12:02:08Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

+
+def combination(img_list, h_clip, w_clip):
+
+    column =[]


extra space for clarity

StevenPuttemans · 2019-06-12T12:58:25Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/visualize.py

+
+
+class Visualizer():
+    """


Remove all the chinese signs and overburden.
Also how are we sure that this code is not licensed?

StevenPuttemans · 2019-06-12T13:01:22Z

modules/Matting/Global-Sampling-Method-for-Alpha-Matting/globalmatting.h

+
+#include <opencv2/opencv.hpp>
+
+void expansionOfKnownRegions(cv::InputArray img, cv::InputOutputArray trimap, int niter = 9);


The number of iterations here, is this always fixed? Do we want to change this value?

This function was used in the paper Shared sampling for real time matting, and it is mentioned that the number of iterations must be around 10.

Then I do get we want to set a default value, for example 9, but we would really like to have a setter and getter to change that value when desired.

modules/Matting/Global-Sampling-Method-for-Alpha-Matting/guidedfilter.cpp

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

czgdp1807 · 2019-06-12T13:38:10Z

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py

+        padding_result = np.pad(img, ((0, h_padding - h), (0, w_padding - w)), 'constant', constant_values=0)
+
+        return Image.fromarray(padding_result), int(w_padding/320), int(h_padding/320)
+    else:


This else isn't needed. And by the comment of @StevenPuttemans may be the whole branching can be eliminated? Am I right?

StevenPuttemans · 2019-06-12T13:49:50Z

@czgdp1807 since this PR is the main place for discussing the advancement of the GSoC 2019 project of @Nerdyvedi I suggest that we keep discussions on what is a good way of contributing to the forum (https://answers.opencv.org/questions/) or to the online chat system on SLACK (https://open-cv.slack.com/) or freenode webchat (https://webchat.freenode.net/ - #opencv).

This will allow me to keep working on contents here, and share thoughts on good ways to contribute somewhere else :)

czgdp1807 · 2019-06-12T14:01:13Z

Please accept my apologies. I was just sort of interested in the code so went through it. I will take care from now onwards. Thanks for providing the direct links to the social platforms of OpenCV.

StevenPuttemans · 2019-07-22T08:32:38Z

@Nerdyvedi give me a heads up here when you are finished with the above commits, so that I am sure to evaluate the final version :)

Nerdyvedi · 2019-07-22T13:26:58Z

@Nerdyvedi give me a heads up here when you are finished with the above commits, so that I am sure to evaluate the final version :)

I am first trying to merge the implementation of Global sampling method for matting, as it's implementation is complete, and I don't think a lot of changes would be needed.

StevenPuttemans · 2019-07-23T07:40:11Z

Make sure you squash all commits in the end. Commit history should be clear.

Since you are splitting Global Sampling Method and AlphaGAN, in what seems to be seperate PRS, create both and change their name, so that they resemble the same. Now it may look that you did less than before.

Also by removing most of the AlphaGAN files, the comments are gone, so I hope you took time to note them down.

StevenPuttemans · 2019-07-23T07:42:28Z

@alalek 2 questions on technicalities

The current warning of patch size > 5MB, is that something we can solve at our end?
Where will we need to place all the data for the AlphaGAN? Like datasets that we created? Put it on opencv_extra also?

StevenPuttemans · 2019-07-23T07:43:19Z

and maybe a 3rd question. The stereo tests seem to fail, but they are unrelated to this code. Any idea why this happens? We need to wait for other PR to get merged?

StevenPuttemans · 2019-07-29T08:42:06Z

@mshabunin or @vpisarev is there maybe one of you that could answer the above questions addressed to @alalek? He might be on holidays?

mshabunin · 2019-07-29T11:22:42Z

@StevenPuttemans , commits need to be squashed, then patch size will become normal. Stereo test failure seem to be sporadic, don't worry about it.

Shouldn't it be transformed into sample application in ximgproc module? Or tutorial? Or algorithm + test + sample application?
Currently it does not get covered by CI at all.

StevenPuttemans · 2019-07-29T11:26:09Z

@mshabunin thanks for the feedback :) About your last comment. It is not ready yet, but you have a good point. The reason of building a matting module, has been that several students will work on this. On the other hand, we can put it also under the ximgproc module indeed.

Final idea is indeed algorithm + test + sample for usage + tutorial.

StevenPuttemans · 2019-08-19T13:07:17Z

@Nerdyvedi can you please provide an update on your progress? This PR must contain your final code before end of GSoC for the global sampling part of the project.

alalek · 2019-08-29T14:52:52Z

modules/ximgproc/include/opencv2/ximgproc/globalmatting.hpp

+        public:
+                GlobalMatting();
+
+		void globalMatting(cv::InputArray _image, cv::InputArray _trimap, cv::OutputArray _foreground, cv::OutputArray _alpha, cv::OutputArray _conf);


Please avoid using of 'tabs'. Indentation is 4 spaces (no need to indent after opening of "whole-file" namespaces)

alalek · 2019-08-29T14:54:31Z

modules/ximgproc/include/opencv2/ximgproc/globalmatting.hpp

+#ifndef GLOBAL_MATTING_H
+#define GLOBAL_MATTING_H
+
+#include <opencv2/highgui.hpp>


Please remove highgui from public header and from module dependency.

alalek · 2019-08-29T14:54:37Z

modules/ximgproc/include/opencv2/ximgproc/globalmatting.hpp

+
+#include <opencv2/highgui.hpp>
+#include <opencv2/imgproc.hpp>
+#include <opencv2/ximgproc/edge_filter.hpp>


Include headers which are required to describe this public API only. Remove other.

modules/ximgproc/include/opencv2/ximgproc/globalmatting.hpp

alalek · 2019-08-29T15:00:42Z

modules/ximgproc/test/test_globalmatting.cpp

+  if(img.empty() || trimap.empty())
+  {
+    ts->printf(cvtest::TS::LOG,"Test images not found!\n");
+    ts->set_failed_test_info(cvtest::TS::FAIL_INVALID_TEST_DATA);


ASSERT_FALSE(img.empty() || trimap.empty()) << "Test images not found!";

alalek · 2019-08-29T15:03:45Z

modules/ximgproc/test/test_globalmatting.cpp

+
+
+class CV_GlobalMattingTest : public cvtest::BaseTest
+{


This class is not needed (BaseTest is a legacy stuff).
Put all test code into TEST() body.

Should I use TEST instead of BaseTest, sorry sir, I didn't quite get it.

You should not use cvtest::BaseTest (it is legacy).

TEST() and other GoogleTest stuff is enough.

alalek · 2019-08-29T15:03:55Z

modules/ximgproc/test/test_globalmatting.cpp

+void CV_GlobalMattingTest::runModel()
+{
+  std::string img_path     = std::string(ts->get_data_path()) + INPUT_DIR + "/" + IMAGE_FILENAME;
+  std::string trimap_path  = std::string(ts->get_data_path()) + INPUT_DIR + "/" + TRIMAP_FILENAME;


ts->get_data_path()

Use cvtest::findDataFile(INPUT_DIR + "/" + IMAGE_FILENAME)

@alalek , Could you please take a look , the buildbots tests are not queued.

modules/ximgproc/src/globalmatting.cpp

modules/ximgproc/include/opencv2/ximgproc/globalmatting.hpp

alalek · 2019-08-30T12:58:51Z

~~@Nerdyvedi Please add missing files here from #2198 (keep changes in a single PR)~~

StevenPuttemans · 2019-09-25T06:53:18Z

@alalek @mshabunin is there anyone that knows why this pullrequest is not qeued for buildbots? @Nerdyvedi would like to make sure it gets all green flags so that it can get integrated into the library.

alalek · 2019-09-25T09:41:36Z

Because of "master" source branch: from Nerdyvedi:master.

Build process of OpenCV uses multiple repositories, so it can't be handled automatically by CI.
We prefer using of properly named source branches: https://github.com/opencv/opencv/wiki/How_to_contribute#the-instruction-in-brief (item 6)
All forks have "master" branch (usually stalled), but only useful branches has proper names.

StevenPuttemans · 2019-09-25T11:06:23Z

Ow okay! We will fix that!

alalek · 2019-09-25T11:45:13Z

Unfortunately this can't be fixed without PR re-creation due GitHub limitations.
It is better to continue here (to avoid lost of review progress).

Nerdyvedi force-pushed the master branch from 6f288ec to 9346e06 Compare June 6, 2019 15:11

StevenPuttemans reviewed Jun 12, 2019

View reviewed changes

czgdp1807 reviewed Jun 12, 2019

View reviewed changes

modules/Matting/AlphaGAN-Matting/AlphaGAN/alphaGAN_eval.py Outdated Show resolved Hide resolved

czgdp1807 reviewed Jun 12, 2019

View reviewed changes

Nerdyvedi changed the title ~~[GSOC 2019] Alpha Matting~~ [GSOC 2019] Global sampling matting Jul 23, 2019

Nerdyvedi mentioned this pull request Jul 24, 2019

[GSOC 2019]AlphaGAN Matting #2198

Open

Nerdyvedi force-pushed the master branch 2 times, most recently from 908b1bc to 5aa25e9 Compare July 31, 2019 21:58

Nerdyvedi added 9 commits August 25, 2019 23:09

Add files via upload

2b35f51

Add files via upload

65d4268

Add files via upload

ab0015d

Update README.md

75bb304

Update and rename mattingClass.cpp to globalmatting.cpp

977d524

Rename mattingClass.h to globalmatting.h

0cf5b80

Update and rename globalmatting.h to globalmatting.hpp

cba44e8

Update globalmatting.cpp

61bfe98

Add files via upload

c226204

Nerdyvedi added 8 commits August 27, 2019 18:52

Update globalmatting.cpp

f2773a1

Update test_globalmatting.cpp

0a7cd13

Removing whitespaces

cdd78a8

Removed whitespaces

3bd6b02

Update test_globalmatting.cpp

18e313d

Update test_globalmatting.cpp

02df381

Update test_globalmatting.cpp

6eacc41

Update globalmatting.cpp

2850fe6

alalek reviewed Aug 29, 2019

View reviewed changes

Nerdyvedi added 15 commits September 1, 2019 01:08

Removing global structure from header file

405aa9b

Added the global structure into cpp file

f24a6b6

Added license and removed extra header files

65818c6

Added License information

60fef49

Removed whitespaces

ac5f5ba

Update globalmatting.cpp

0af5e5f

Create globalmatting.md

cda943d

Update globalmatting.md

0782161

Update globalmatting.md

e5e6be1

Removed square function from the public header

afce16a

Added the square function to the source file

7e739df

Added license information and removed BaseTest

f238144

Using cvtest::findDatafile

48abe60

Bringing y to outer loop and x in inner loop

93f54a5

Added license information to sample

0a7ca9e

Nerdyvedi closed this Sep 30, 2019


		#include <opencv2/opencv.hpp>

		void expansionOfKnownRegions(cv::InputArray img, cv::InputOutputArray trimap, int niter = 9);

[GSOC 2019] Global sampling matting #2134

[GSOC 2019] Global sampling matting #2134

Conversation

Nerdyvedi commented Jun 3, 2019 • edited by alalek Loading

Steps

Results

alalek commented Jun 3, 2019 • edited Loading

StevenPuttemans commented Jun 4, 2019

StevenPuttemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenPuttemans commented Jun 12, 2019

czgdp1807 commented Jun 12, 2019

StevenPuttemans commented Jul 22, 2019

Nerdyvedi commented Jul 22, 2019

StevenPuttemans commented Jul 23, 2019

StevenPuttemans commented Jul 23, 2019

StevenPuttemans commented Jul 23, 2019

StevenPuttemans commented Jul 29, 2019

mshabunin commented Jul 29, 2019

StevenPuttemans commented Jul 29, 2019

StevenPuttemans commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalek commented Aug 30, 2019 • edited Loading

StevenPuttemans commented Sep 25, 2019

alalek commented Sep 25, 2019

StevenPuttemans commented Sep 25, 2019

alalek commented Sep 25, 2019

Nerdyvedi commented Jun 3, 2019 •

edited by alalek

Loading

alalek commented Jun 3, 2019 •

edited

Loading

alalek commented Aug 30, 2019 •

edited

Loading