add ignore edge function in pdiff.

add ignore edge function in pdiff.

[vangelis, 2009-06-30 02:35:05]

Comments below!

http://codereview.appspot.com/89059/diff/1/6
File README (right):

http://codereview.appspot.com/89059/diff/1/6#newcode1
Line 1: PerceptualDiff is an image comparison utility that makes use of a
This file should be called README.google and follow these basic guidelines:

http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#third_party_packag...

http://codereview.appspot.com/89059/diff/1/3
File files/source/CompareArgs.cpp (right):

http://codereview.appspot.com/89059/diff/1/3#newcode56
Line 56: EdgeDetect = false;
Please match the indentation of the rest of the items.

http://codereview.appspot.com/89059/diff/1/3#newcode89
Line 89: //*********************************************
No need to have the comment above.  Better to stay consistent with the rest of
the code.

http://codereview.appspot.com/89059/diff/1/3#newcode159
Line 159: else if (EdgeDetect)
this doesn't allow us to use both an output file name and edge detect at the
same time.  Do we really want that?

http://codereview.appspot.com/89059/diff/1/4
File files/source/CompareArgs.h (right):

http://codereview.appspot.com/89059/diff/1/4#newcode38
Line 38: bool			EdgeDetect;         // ***run edge detection method on results
to eliminate single pixel edges and scattered points.
remove the *** to match the style of the rest of the comments.

http://codereview.appspot.com/89059/diff/1/2
File files/source/Metric.cpp (right):

http://codereview.appspot.com/89059/diff/1/2#newcode127
Line 127: void detect_edge(RGBAImage* diff_image, int* edge_image, unsigned int
w, unsigned int h) {
best to stick with the 80 column limit and break long lines.

http://codereview.appspot.com/89059/diff/1/2#newcode129
Line 129: int kernel_length = 3;
Seems that the current convention for pdiff is to use camelCase for variable
names.  Best to match that.  
kernel_length -> kernelLength

and there's a few spots below as well.

http://codereview.appspot.com/89059/diff/1/2#newcode137
Line 137: edge_image[index] = 0;
watch indentation.

http://codereview.appspot.com/89059/diff/1/2#newcode138
Line 138: for (cx = -1; cx <=1; cx++)
it's good practice to always use curly braces { } for the body of for loops.  It
makes code easier to read and modify and it's less error prone.

http://codereview.appspot.com/89059/diff/1/2#newcode143
Line 143: if (nx < 0) nx = -nx;
it seems that instead of wrapping we should clamp to the image borders.

http://codereview.appspot.com/89059/diff/1/2#newcode147
Line 147: edge_image[index] += diff_image->Get_Red(ny * w + nx) * kernel[(cy +
1) * kernel_length + cx + 1];
80 col limit

http://codereview.appspot.com/89059/diff/1/2#newcode150
Line 150: if (edge_image[index] > 50)
since 50 is an arbitrary value, it should be exposed as a command line argument 
associated with the edge flag.

http://codereview.appspot.com/89059/diff/1/2#newcode326
Line 326: //detect edges here, and protect them to be checked as inconsistece.*
Not sure I understand the comment here.

http://codereview.appspot.com/89059/diff/1/2#newcode339
Line 339: edge_image[index] == 255)
watch indentation

http://codereview.appspot.com/89059/diff/1/2#newcode349
Line 349: /*std::string ref_name = args.ImgA->Get_Name();
If this code is not used, please remove it.

http://codereview.appspot.com/89059/diff/1/2#newcode368
Line 368: //*******************************************************************
Do we need this comment here?

http://codereview.appspot.com/89059/diff/1/2#newcode379
Line 379: args.ErrorStr+= args.ImgDiff->Get_Name();
Looks like there's tabs in there.  Please remove..  Here and below.

[yux, 2009-06-30 20:16:50]

Fixed. And expose the threshold parameter used in edge detection.
Also modified related selenium test scripts, which can be reviewed here:
 http://codereview.chromium.org/151060
Thanks.
---
Xiao
On 2009/06/30 02:35:05, vangelis wrote:
> Comments below!
> 
> http://codereview.appspot.com/89059/diff/1/6
> File README (right):
> 
> http://codereview.appspot.com/89059/diff/1/6#newcode1
> Line 1: PerceptualDiff is an image comparison utility that makes use of a
> This file should be called README.google and follow these basic guidelines:
> 
>
http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#third_party_packag...
> 
> http://codereview.appspot.com/89059/diff/1/3
> File files/source/CompareArgs.cpp (right):
> 
> http://codereview.appspot.com/89059/diff/1/3#newcode56
> Line 56: EdgeDetect = false;
> Please match the indentation of the rest of the items.
> 
> http://codereview.appspot.com/89059/diff/1/3#newcode89
> Line 89: //*********************************************
> No need to have the comment above.  Better to stay consistent with the rest of
> the code.
> 
> http://codereview.appspot.com/89059/diff/1/3#newcode159
> Line 159: else if (EdgeDetect)
> this doesn't allow us to use both an output file name and edge detect at the
> same time.  Do we really want that?
> 
> http://codereview.appspot.com/89059/diff/1/4
> File files/source/CompareArgs.h (right):
> 
> http://codereview.appspot.com/89059/diff/1/4#newcode38
> Line 38: bool			EdgeDetect;         // ***run edge detection method on results
> to eliminate single pixel edges and scattered points.
> remove the *** to match the style of the rest of the comments.
> 
> http://codereview.appspot.com/89059/diff/1/2
> File files/source/Metric.cpp (right):
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode127
> Line 127: void detect_edge(RGBAImage* diff_image, int* edge_image, unsigned
int
> w, unsigned int h) {
> best to stick with the 80 column limit and break long lines.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode129
> Line 129: int kernel_length = 3;
> Seems that the current convention for pdiff is to use camelCase for variable
> names.  Best to match that.  
> kernel_length -> kernelLength
> 
> and there's a few spots below as well.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode137
> Line 137: edge_image[index] = 0;
> watch indentation.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode138
> Line 138: for (cx = -1; cx <=1; cx++)
> it's good practice to always use curly braces { } for the body of for loops. 
It
> makes code easier to read and modify and it's less error prone.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode143
> Line 143: if (nx < 0) nx = -nx;
> it seems that instead of wrapping we should clamp to the image borders.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode147
> Line 147: edge_image[index] += diff_image->Get_Red(ny * w + nx) * kernel[(cy +
> 1) * kernel_length + cx + 1];
> 80 col limit
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode150
> Line 150: if (edge_image[index] > 50)
> since 50 is an arbitrary value, it should be exposed as a command line
argument 
> associated with the edge flag.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode326
> Line 326: //detect edges here, and protect them to be checked as
inconsistece.*
> Not sure I understand the comment here.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode339
> Line 339: edge_image[index] == 255)
> watch indentation
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode349
> Line 349: /*std::string ref_name = args.ImgA->Get_Name();
> If this code is not used, please remove it.
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode368
> Line 368:
//*******************************************************************
> Do we need this comment here?
> 
> http://codereview.appspot.com/89059/diff/1/2#newcode379
> Line 379: args.ErrorStr+= args.ImgDiff->Get_Name();
> Looks like there's tabs in there.  Please remove..  Here and below.

[vangelis, 2009-07-01 18:35:31]

http://codereview.appspot.com/89059/diff/1010/15
File files/source/CompareArgs.cpp (right):

http://codereview.appspot.com/89059/diff/1010/15#newcode35
Line 35: \t-noedge e      : Degree of edge ignore (0.0 to 1.0), 1 is complete
ignore\n\
'complete ignore' is a bit unclear.  Can you add a comment (similar to what you
have in the samples_list.txt) that describes what 0 and 1 roughly mean?

http://codereview.appspot.com/89059/diff/1010/16
File files/source/CompareArgs.h (right):

http://codereview.appspot.com/89059/diff/1010/16#newcode38
Line 38: float			EdgeDetectDegree;   // degree to eliminate edges
I think if you added a separate bool for "ignoreEdgePixels" it would make the
code a lot cleaner than using edgeDetectDegree = 0 as a sign that you don't want
to ignore edges.  The problem with 0 is there a discontinuity between 0 and 0 +
epsilon.  At 0 you eliminate no pixels and at 0 + epsilon you eliminate a lot of
pixels and then as you go towards 1 you eliminate fewer and fewer.

http://codereview.appspot.com/89059/diff/1010/14
File files/source/Metric.cpp (right):

http://codereview.appspot.com/89059/diff/1010/14#newcode126
Line 126: // simple convolution based edge detection.
It would be helpful to expand a bit on the comment here.  For example say that
you do convolution with a 3x3 kernel and resulting image values are either 0 or
255 depending of whether the pixel value falls above or below the threshold.

http://codereview.appspot.com/89059/diff/1010/14#newcode128
Line 128: unsigned int h, float edgeDetectDegree) {
move second line back by 1 space to align under the first.

http://codereview.appspot.com/89059/diff/1010/14#newcode135
Line 135: for (x = 0; x < w; x++) {
best to use the same number of spaces for indenting lines that the rest of the
code uses.

http://codereview.appspot.com/89059/diff/1010/14#newcode142
Line 142: if (nx < 0) nx = -nx;
I mentioned this in the last review as well but I think the proper thing to do
here is clamp (to zero or to the full size) rather than mirror.

http://codereview.appspot.com/89059/diff/1010/14#newcode147
Line 147: kernel[(cy + 1) * kernelLength + cx + 1];
indent the line in to make it clear that it's continuing from above.

http://codereview.appspot.com/89059/diff/1010/14#newcode152
Line 152: // all edges would be ignored.
That's not quite true, right?  If threshold is 0 zero you mark as edge pixels
any pixel that has 7 neighbors or less, which is almost (except for 1 pixel) a
solid 3x3 block. I think that true edges are when you have about 2 to 4
neighbors so the values are close to (8-2)*255 or (8-4)*255

http://codereview.appspot.com/89059/diff/1010/14#newcode156
Line 156: int threshold = static_cast<int>((1.0f - edgeDetectDegree) *
the threshold calculation should move outside the loop.

http://codereview.appspot.com/89059/diff/1010/14#newcode159
Line 159: edgeImg[index] = 255;
Use 4 space indent to stay consistent.

http://codereview.appspot.com/89059/diff/1010/14#newcode336
Line 336: int* edgeImg = new int[w * h];
code block needs to be indented

http://codereview.appspot.com/89059/diff/1010/14#newcode361
Line 361: if (args.ImgDiff)
watch indentation

[vangelis, 2009-07-01 18:38:29]

http://codereview.appspot.com/89059/diff/1010/15
File files/source/CompareArgs.cpp (right):

http://codereview.appspot.com/89059/diff/1010/15#newcode35
Line 35: \t-noedge e      : Degree of edge ignore (0.0 to 1.0), 1 is complete
ignore\n\
Maybe a better name for this argument would be:
-ignoreEdges e  where e is the threshold which determines how many neighbors the
pixel must have be consider an edge pixel.

[yux, 2009-07-02 23:55:09]

Fixed. Now the threshold of ignoreEdges option could be set in (0 to 7) which
represents the max number of neighbors one pixel can have to be considered as
edge.
Thanks.
---
Xiao

On 2009/07/01 18:38:29, vangelis wrote:
> http://codereview.appspot.com/89059/diff/1010/15
> File files/source/CompareArgs.cpp (right):
> 
> http://codereview.appspot.com/89059/diff/1010/15#newcode35
> Line 35: \t-noedge e      : Degree of edge ignore (0.0 to 1.0), 1 is complete
> ignore\n\
> Maybe a better name for this argument would be:
> -ignoreEdges e  where e is the threshold which determines how many neighbors
the
> pixel must have be consider an edge pixel.

[vangelis, 2009-07-09 02:43:29]

A few comments but otherwise LGTM  !

http://codereview.appspot.com/89059/diff/2001/2006
File README.google (right):

http://codereview.appspot.com/89059/diff/2001/2006#newcode11
Line 11: A "-noedge e (0.0 to 1.0)" option is added to the original version,
flag is now called 'ignoreEdges'

http://codereview.appspot.com/89059/diff/2001/2006#newcode14
Line 14: Parameter e is the threshold in edge detection, if e is 1.0, all
please update the description of the values for 'e'

http://codereview.appspot.com/89059/diff/2001/2002
File files/source/Metric.cpp (right):

http://codereview.appspot.com/89059/diff/2001/2002#newcode362
Line 362: {
seems to be some extra whitespace after the { . Please remove.