MantisBT - Open CASCADE
View Issue Details
0028876Open CASCADE[OCCT] OCCT:Testspublic2017-06-28 09:482017-09-29 16:28
kgv 
bugmaster 
normalminor 
closedfixed 
[OCCT] 7.2.0 
[OCCT] 7.2.0[OCCT] 7.2.0 
Not required
0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
The image difference is unavailable for test case bugs vis bug28205_1.
The reason is that this test produces Grayscale images, which are not currently supported by Image_Diff tool.
testdiff for bugs/vis/bug28205_1
No tags attached.
child of 0028205closed apn Visualization - add functionality for dumping results of detection algorithms into image 
child of 0023272closed kgv Image comparison algorithm 
Issue History
2017-06-28 09:48kgvNew Issue
2017-06-28 09:48kgvAssigned To => kgv
2017-06-28 09:48kgvRelationship addedchild of 0028205
2017-06-28 10:56bugmasterAssigned Tokgv => apn
2017-07-03 12:28gitNote Added: 0067936
2017-07-03 15:56kgvNote Added: 0067948
2017-07-03 15:56kgvAssigned Toapn => abv
2017-07-03 15:56kgvStatusnew => resolved
2017-07-03 15:56kgvRelationship addedchild of 0023272
2017-07-03 15:57kgvNote Edited: 0067948bug_revision_view_page.php?bugnote_id=67948#r17019
2017-07-03 15:58kgvNote Edited: 0067948bug_revision_view_page.php?bugnote_id=67948#r17020
2017-07-03 19:14abvNote Added: 0067954
2017-07-03 19:14abvAssigned Toabv => kgv
2017-07-03 19:14abvStatusresolved => feedback
2017-07-04 13:50gitNote Added: 0067970
2017-07-04 13:51kgvNote Added: 0067971
2017-07-04 13:51kgvAssigned Tokgv => abv
2017-07-04 13:51kgvStatusfeedback => resolved
2017-07-04 14:21gitNote Added: 0067973
2017-07-04 22:20abvNote Edited: 0067954bug_revision_view_page.php?bugnote_id=67954#r17030
2017-07-05 03:06gitNote Added: 0067981
2017-07-05 03:13abvNote Added: 0067982
2017-07-05 03:13abvAssigned Toabv => kgv
2017-07-05 07:51gitNote Added: 0067984
2017-07-05 07:52gitNote Added: 0067985
2017-07-05 07:53kgvNote Added: 0067986
2017-07-05 07:53kgvAssigned Tokgv => bugmaster
2017-07-05 07:53kgvStatusresolved => reviewed
2017-07-05 11:14gitNote Added: 0067996
2017-07-05 11:17apvAssigned Tobugmaster => apv
2017-07-05 14:34apvTest case number => Not required
2017-07-06 12:40apvNote Added: 0068032
2017-07-06 12:40apvAssigned Toapv => bugmaster
2017-07-06 12:40apvStatusreviewed => tested
2017-07-06 12:41apvNote Added: 0068033
2017-07-07 17:25bugmasterChangeset attached => occt master e958a649
2017-07-07 17:25bugmasterStatustested => verified
2017-07-07 17:25bugmasterResolutionopen => fixed
2017-07-10 08:58gitNote Added: 0068126
2017-07-10 08:58gitNote Added: 0068127
2017-07-10 08:58gitNote Added: 0068128
2017-09-29 16:18aivFixed in Version => 7.2.0
2017-09-29 16:28aivStatusverified => closed

Notes
(0067936)
git   
2017-07-03 12:28   
Branch CR28876 has been created by kgv.

SHA-1: 49f380640e8a14043b09edb031172154fe5c5669


Detailed log of new commits:

Author: kgv
Date: Mon Jul 3 12:27:08 2017 +0300

    0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
    
    Image_Diff has been improved to support Image_Format_Gray.
    Image_Diff::SaveDiffImage() now saves image difference
    in Image_Format_Gray format to reduce size of image file.
(0067948)
kgv   
2017-07-03 15:56   
(edited on: 2017-07-03 15:58)
Patch is ready for review.

CR28876-master-KGV-Products-Windows-64-VC10-opt-tests-compare-windows - Build # 1 - Successful:
Check console output at http://jenkins-test-10.nnov.opencascade.com:8080/job/CR28876-master-KGV-Products-Windows-64-VC10-opt-tests-compare-windows/1/ [^] to view the results.

CR28876-master-KGV-Products-Debian70-64-opt-tests-compare-linux - Build # 1 - Successful:
Check console output at http://jenkins-test-10.nnov.opencascade.com:8080/job/CR28876-master-KGV-Products-Debian70-64-opt-tests-compare-linux/1/ [^] to view the results.

(0067954)
abv   
2017-07-03 19:14   
(edited on: 2017-07-04 22:20)
I propose to revise the patch and some affected code to make it less confusing:

1. It is better to make method PixelColor() returning result by value instead of putting it in non-const reference argument; Quantity_ColorRGBA seems to be better choice than NCollection_Vec4f<float> since the latter does not have semantic of representing a color (). Related to this:

- Quantity_ColorRGBA deserves having constructor accepting 4 numbers
- Both Quantity_ColorRGBA and NCollection_Vec4f deserve having a setter function accepting 4 numbers
- Names of methods __testSize3() __testSize4() in Quantity_ColorRGBA violate C++ rules -- names with double underscore are reserved for compiler and standard library, see e.g. http://en.cppreference.com/w/cpp/language/identifiers [^]

2. In Image_Diff::Compare(), add check for same size of the images before calling memcmp() to avoid possibility of accessing uninitialized memory if images are of different size

3. In Image_Diff.cxx, usage of Standard_Size for index of pixel seems to have no reason and leads to strange casts, better to be avoided (Standard_Integer seems to be quite sufficient).

4. In Image_Diff.cxx, method isBlack() of unnamed struct in "default" case of switch could call inline function isBlack(const NCollection_Vec4<float>&) defined just above instead of doing the same. The same for Gray and Alpha cases. Related to this:

- it is worth declaring inline functions defined in CXX file as static, to have compiler warning in case if they are not used

- it is better to give names even to locally used types (such as struct used in definition of NEIGHBOR_PIXELS -- this will make code more understandable (and also will allow compiler to generate more adequate warnings)

- for the same reason, I would convert methods of that structure to static functions
 
4. Putting textual messages to DefaultMessenger() (i.e. equivalent of cout) is not a good practice for a general-purpose tool... yet I see it possibly useful for our context of running it from DRAW... (this is hidden advertising of Alerts)

(0067970)
git   
2017-07-04 13:50   
Branch CR28876_1 has been created by kgv.

SHA-1: 30526377cac419d3453f446345c1178857cb7670


Detailed log of new commits:

Author: kgv
Date: Mon Jul 3 12:27:08 2017 +0300

    0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
    
    Quantity_ColorRGBA - added method SetValues().
    Image_PixMap::PixelColor() now returns Quantity_ColorRGBA instead of Quantity_Color.
    Image_PixMap::SetPixelColor() now takes Quantity_ColorRGBA instead of NCollection_Vec4<float>.
    
    Image_Diff has been improved to support Image_Format_Gray.
    Image_Diff::SaveDiffImage() now saves image difference
    in Image_Format_Gray format to reduce size of image file.
    
    Image_Diff now uses TColStd_HPackedMapOfInteger instead of
    TColStd_MapOfInteger with manual memory allocation.
    New struct DiffPixelVec2 has been defined for packing
    two 16-bit numbers into 32-bit integer.
(0067971)
kgv   
2017-07-04 13:51   
Patch has been updated following remarks.
(0067973)
git   
2017-07-04 14:21   
Branch CR28876_1 has been updated forcibly by kgv.

SHA-1: ea0e194fc3d532f770a5c9dc9a441507731030e8
(0067981)
git   
2017-07-05 03:06   
Branch CR28876_1 has been updated by abv.

SHA-1: e6729670fe866a05f830250027bb7f9e1cf37f07


Detailed log of new commits:

Author: abv
Date: Wed Jul 5 02:39:59 2017 +0300

    # review: corrections to simplify the code

(0067982)
abv   
2017-07-05 03:13   
I have made two corrections:

- in Image_Diff, avoid usage of NCollection_Vec2 and specific class DiffPixelVec2, using plain base types and simple functions instead

- in Quantity_ColorRGBA, avoid "explicit" in constructors accepting two arguments - here we seems to have no danger of possible unintended type conversion

Please look
(0067984)
git   
2017-07-05 07:51   
Branch CR28876_1 has been updated by kgv.

SHA-1: 4e3c0be106a4eb6538ff279255649e64ec967acd


Detailed log of new commits:

Author: kgv
Date: Wed Jul 5 07:51:31 2017 +0300

    # Image_Diff::SaveDiffImage() - fix parameters order passed to Image_PixMap::SetPixelColor()

(0067985)
git   
2017-07-05 07:52   
Branch CR28876_2 has been created by kgv.

SHA-1: 291fed9fdf750cccadeb077c9beb5522cada4faf


Detailed log of new commits:

Author: kgv
Date: Mon Jul 3 12:27:08 2017 +0300

    0028876: Tests, Image_Diff - the image difference is unavailable for test case bugs vis bug28205_1
    
    Quantity_ColorRGBA - added method SetValues().
    Image_PixMap::PixelColor() now returns Quantity_ColorRGBA instead of Quantity_Color.
    Image_PixMap::SetPixelColor() now takes Quantity_ColorRGBA instead of NCollection_Vec4<float>.
    
    Image_Diff has been improved to support Image_Format_Gray.
    Image_Diff::SaveDiffImage() now saves image difference
    in Image_Format_Gray format to reduce size of image file.
    
    Image_Diff now uses TColStd_HPackedMapOfInteger instead of
    TColStd_MapOfInteger with manual memory allocation.
(0067986)
kgv   
2017-07-05 07:53   
Please test the patch.
(0067996)
git   
2017-07-05 11:14   
Branch CR28876_2 has been updated forcibly by kgv.

SHA-1: 2358a79c1bbddf9334b30bfdfddb8779bff3da6e
(0068032)
apv   
2017-07-06 12:40   
Dear BugMaster,

Branch CR28876_2 from occt git-repository (and master from products git-repository) was compiled on Linux, MacOS and Windows platforms and tested.
SHA-1: 291fed9fdf750cccadeb077c9beb5522cada4faf

Number of compiler warnings:
occt component:
   Linux: 0 (0 on master)
   Windows: 0 (0 on master)
   MasOS: 0 (0 on master)
products component:
   Linux: 64
   Windows: 0
   MacOS: 1215

Regressions/Differences:
Not detected

Testing cases:
Not required

Testing on Linux:
Total MEMORY difference: 92768512 / 92186190 [+0.63%]
Total CPU difference: 18775.510000000166 / 18926.030000000348 [-0.80%]

Testing on Windows:
Total MEMORY difference: 58849153 / 58850236 [-0.00%]
Total CPU difference: 17507.898629398598 / 17597.770805498705 [-0.51%]
(0068033)
apv   
2017-07-06 12:41   
Dear BugMaster,

Please integrate branch CR28876_2 into the occt git-repository master.
(0068126)
git   
2017-07-10 08:58   
Branch CR28876 has been deleted by kgv.

SHA-1: 49f380640e8a14043b09edb031172154fe5c5669
(0068127)
git   
2017-07-10 08:58   
Branch CR28876_1 has been deleted by kgv.

SHA-1: 4e3c0be106a4eb6538ff279255649e64ec967acd
(0068128)
git   
2017-07-10 08:58   
Branch CR28876_2 has been deleted by kgv.

SHA-1: 2358a79c1bbddf9334b30bfdfddb8779bff3da6e