Changed hog class with a new optimized version which exploits SSE2 operations (3x speedup in descriptor computation). #156

mmunaro · 2013-06-24T16:30:07Z

No description provided.

jspricke · 2013-06-24T16:39:34Z

For the sse.hpp, would it make sense to put it into pcl_common? Also, can you put guards around your SSE code and add a non SSE version as well? Have a look at Brisk for how I did it.

Cheers Jochen

mmunaro · 2013-06-25T09:14:41Z

Hi Jochen,

Sorry for the whitespaces, they were not added on purpose.
Do you want the copyright header to be like that?

Point Cloud Library (PCL) - www.pointclouds.org
Copyright (c) 2013-, Open Perception, Inc.
Copyright (c) 2012, Piotr Dollar & Ron Appel. [pdollar-at-caltech.edu]

I can change the copyright, I only have to retain the line from the previous author. The license is slightly different, since it is the Simplified BSD. Maybe it can be changed to BSD, but I am not sure.
I committed the new header.

For the sse.hpp, I moved it to pcl_common. The guards already exist aroung that code, but I converted the guard names to PCL style:

#ifndef PCL_COMMON_SSE_HPP_
#define PCL_COMMON_SSE_HPP_

What did you want me to look at in the Brisk code?

Do you think it is necessary to add a non SSE version as well?
I am not very familiar with SSE. If you think it is necessary, I do not think to have time to do that right now, but maybe I can try to convert it in the next two weeks.

Cheers,
Matteo

jspricke · 2013-06-25T18:08:54Z

Hi Matteo,

Matteo Munaro [email protected] [2013-06-25 02:14]:
1. Do you want the copyright header to be like that?
* Point Cloud Library (PCL) - www.pointclouds.org
* Copyright (c) 2013-, Open Perception, Inc.
* Copyright (c) 2012, Piotr Dollar & Ron Appel. [pdollar-at-caltech.edu]

Sounds good.

I can change the copyright, I only have to retain the line from the previous author. The license is slightly different, since it is the Simplified BSD. Maybe it can be changed to BSD, but I am not sure.

Ah, I see, should be fine.

For the sse.hpp, I moved it to pcl_common. The guards already exist aroung that code, but I converted the guard names to PCL style:

#ifndef PCL_COMMON_SSE_HPP_
#define PCL_COMMON_SSE_HPP_

No, you should guard the SSE part, have a look at the Brisk code ;).

Do you think it is necessary to add a non SSE version as well?
I am not very familiar with SSE. If you think it is necessary, I do not think to have time to do that right now, but maybe I can try to convert it in the next two weeks.

We can't add your code to a stable release if it's SSE only, cause it
would be broken for most people.

Cheers Jochen

mmunaro · 2013-07-02T15:51:34Z

Hi Jochen,
I added the guards around the SSE code. Please tell me if I misunderstood the way you wanted it to be done.
Is the check for 64 bit ( !defined(i386) ) necessary in your opinion?
The non-SSE version is still missing.

Thanks for help!

Cheers,
Matteo

jspricke · 2013-07-02T16:08:43Z

Hi Matteo,

Matteo Munaro [email protected] [2013-07-02 08:51]:

I added the guards around the SSE code. Please tell me if I misunderstood the way you wanted it to be done.

Great! Please don't indent stuff within the guards, that's non standard.

Is the check for 64 bit ( !defined(i386) ) necessary in your opinion?

No, it's only because of problems in the Brisk code.

mmunaro · 2013-07-02T16:40:22Z

I corrected the indentation and removed the check for 64 bit.

Cheers,
Matteo

jspricke · 2013-07-02T16:48:36Z

Matteo Munaro [email protected] [2013-07-02 09:40]:

I corrected the indentation and removed the check for 64 bit.

Great, looking good now :). As this took a number of tries, could you
squash everything into one commit (or into logical separate once)? Have
a look into git rebase -i how to do that.

Also, what's your opinion on merging this into master and the next
stable version? The point is, that the current version would go into
stable, as it doesn't need SSE2 but the new one wont, unless it's added.

mmunaro · 2013-07-02T22:26:28Z

If I am still on time, I can try to provide the non SSE version by today (Wednesday).
If you want to finalize the Release Candidate before then, please do not include the people module.
In case the people module is not inserted in the RC, would it be possible to add it in the final version of this release?

Cheers,
Matteo

…erations (3x speedup in descriptor computation).

mmunaro · 2013-07-03T17:05:12Z

Hi Jochen,

I committed a non-SSE version of the hog code.
The ifdefs switch between the two implementations.
I tested it with Linux 64bits and the results are the same than those of the SSE-based.

If you want, you can include this version to the release candidate.
Please tell me if I have to change something.

Cheers,
Matteo

jspricke · 2013-07-03T21:20:03Z

test/test_people_groundBasedPeopleDetectionApp.cpp

  }
-  EXPECT_EQ (k, 5);		// verify number of people found (should be five)
+  EXPECT_EQ (k, 5);    // verify number of people found (should be five)
+#else


I guess this can be removed now?

mmunaro · 2013-07-04T10:06:29Z

Hi Jochen,

I updated the code with your last suggestions and I removed some compilation warnings.
From my side, the people module is ready for the release candidate.
It is important to know if it compiles with all the architectures PCL is targeted to, thus I hope that you can test it with travis and report me if something should be changed or if you have other suggestions.

The issues I mentioned in pcl_developers are due to some changes to PCL's visualizer which were merged to trunk after the 28th of May (before that date, everything was working).
I am not an expert on this, but it seems that point clouds are sometimes rotated of 180 degrees and I hope that someone can fix this issue, because visualization is an important part of PCL.

If you need some other changes to be done before the RC, I can work on them until 7pm today (GMT+1 time).

Thanks!

Cheers,
Matteo

jspricke · 2013-07-04T14:27:13Z

Merged, Thanks a lot Matteo!

jspricke · 2013-07-04T14:31:05Z

Matteo Munaro [email protected] [2013-07-04 03:06]:

It is important to know if it compiles with all the architectures PCL is targeted to, thus I hope that you can test it with travis and report me if something should be changed or if you have other suggestions.

Travis is testing 64bit Ubuntu only, but hopefully there will be enough
people testing the RC ;).

The issues I mentioned in pcl_developers are due to some changes to PCL's visualizer which were merged to trunk after the 28th of May (before that date, everything was working).
I am not an expert on this, but it seems that point clouds are sometimes rotated of 180 degrees and I hope that someone can fix this issue, because visualization is an important part of PCL.

Can you do a git bisect?

mmunaro · 2013-07-04T14:39:58Z

Ok, I am trying right now.

mmunaro · 2013-07-04T16:55:47Z

git bisect did not finish because I found some commits which broke compilation.
However, I am pretty convinced that the problem was introduced with this commit:

Merge pull request #92 from nfioraio/master
Better support to point cloud pose setting in PCLVisualizer
789fb08

The last good and bad commits were:
last good: b16dff1
last bad: a49df4c

Tomorrow I will retry to isolate the problem and I will write on the open issues about that.

Thanks for your work for this RC!

Cheers,
Matteo

nfioraio · 2013-07-07T18:11:52Z

Dear all, I didn't read all the posts, but my pull was just about applying the sensor pose stored in the pcl::PointCloud objects using the standard VTK transformations. May the 180° rotation be related to some approx error due to the quaternion --> 3x3 matrix conversion? I haven't experienced anything like this on my pc but...

jspricke · 2013-07-08T07:14:07Z

Hi Nicola,

can we move the discussion into #116, as this is about the pull request and already closed. Thanks!

mmunaro added 3 commits July 3, 2013 10:40

Changed hog class with a new optimized version which exploits SSE2 op…

bd8b66c

…erations (3x speedup in descriptor computation).

Fixed bug multiple definitions of hog (moved hog.hpp to hog.cpp).

0847dff

Bug fixed in hog.g: clip_ variable changed from bool to float.

74a0b6c

jspricke reviewed Jul 3, 2013
View reviewed changes

mmunaro added 2 commits July 4, 2013 10:29

Added non-SSE version of the hog code.

e570d3c

Removed some compilation warnings from people module.

c2d5383

jspricke merged commit c2d5383 into PointCloudLibrary:master Jul 4, 2013

jspricke mentioned this pull request Jul 8, 2013

Groundbased_rgbd_people_detector #116

Closed

Uh oh!

Changed hog class with a new optimized version which exploits SSE2 operations (3x speedup in descriptor computation). #156

Changed hog class with a new optimized version which exploits SSE2 operations (3x speedup in descriptor computation). #156

Uh oh!

Conversation

mmunaro commented Jun 24, 2013

Uh oh!

jspricke commented Jun 24, 2013

Uh oh!

mmunaro commented Jun 25, 2013

Uh oh!

jspricke commented Jun 25, 2013

Uh oh!

mmunaro commented Jul 2, 2013

Uh oh!

jspricke commented Jul 2, 2013

Uh oh!

mmunaro commented Jul 2, 2013

Uh oh!

jspricke commented Jul 2, 2013

Uh oh!

mmunaro commented Jul 2, 2013

Uh oh!

mmunaro commented Jul 3, 2013

Uh oh!

jspricke Jul 3, 2013

Choose a reason for hiding this comment

Uh oh!

mmunaro commented Jul 4, 2013

Uh oh!

jspricke commented Jul 4, 2013

Uh oh!

jspricke commented Jul 4, 2013

Uh oh!

mmunaro commented Jul 4, 2013

Uh oh!

mmunaro commented Jul 4, 2013

Uh oh!

nfioraio commented Jul 7, 2013

Uh oh!

jspricke commented Jul 8, 2013

Uh oh!

Uh oh!