bugGNU Octave - Bugs: bug #59623, [octave forge] (image) bwdist for...

 
 

bug #59623: [octave forge] (image) bwdist for quasi-euclidean case fails on i386

Submitter:  Rafael Laboissière <rlaboiss>
Submitted:  Fri 04 Dec 2020 07:14:07 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Sat 28 Aug 2021 02:16:09 PM UTC, comment #4: 

Closing as fixed, according to comment #3.
Thanks, @Hartmut.

Avinoam Kalma <avinoam>
Group Member
Sat 09 Jan 2021 08:35:23 PM UTC, comment #3: 

I have now pushed a patch with the originally proposed change to the image repo under Rafael's name: http://hg.code.sf.net/p/octave/image/rev/35f1b490c699

This bug report can now be closed as FIXED.

Hartmut <hardy>
Tue 22 Dec 2020 08:48:31 PM UTC, comment #2: 

I have read a bit more in "the manual". Here is what I found, please correct me if I am mistaken at any point. I reason about this piece of code in bwdist.cc:


static float
quasi_euclidean (short x, short y)
{
  static const float sqrt2_1 = sqrt (2) - 1;
  return abs(x)>abs(y) ? (abs(x) + sqrt2_1 * abs(y)) :
                         (sqrt2_1 * abs(x) + abs(y)) ;
}

(The proposed patch would substitute the last four "abs" with "fabs" in this code.)

  • We are using C++11 in the image package.
  • The two input variables (x and y) are of type "short", this is a shorthand for "short signed integer" (having mostly 16 bits).
  • The abs function should work fine with those signed integer input values, I found just one corner case (intmin as input) where it would behave unexpectedly.
  • But the fabs function should also work fine with signed integer input values, it just converts the input to float before working on it. Since in our code we convert the output of abs to float anyways, I think this totally is fine in our case.
  • I have no clue why this test (in line 638 of bwdist.cc) hangs (with the current code) on i386 systems. The input image (bw) in this test contains only (0)s and (-2)s. But the two lines before we do nearly the identical test but with (0)s and (3)s, without any trouble. And the content of the input image bw should not matter at all, it is just "foreground" for nonzero elements and "background" for zero elements.


I have tested the patch from comment #0. The resulting file bwdist.oct behaves well, and all BIST tests still pass on my 64bit linux. The current BISTs also contain several test of output values with the "quasi_euclidean" parameter. So the changed piece of code is really tested by the current BISTs.

I suggest that I will PUSH the proposed patch from Rafel to the image repo during the next couple of days. Please let me know if you have any objections.

Hartmut <hardy>
Sat 05 Dec 2020 05:09:43 PM UTC, comment #1: 

Thanks for letting us know this, Rafael. And also thanks for the patch.

In our current code in bwdist.cc, the function abs() is called with an argument of the integer type "short". (I am unsure if this is a signed or unsigned integer type.)

I am not experienced in C++. Reading the documentation, it sounds to me that it is alright to call abs() on integer types. fabs() seems to be mostly ment for floating point input types. I only find this disclaimer in the docs of abs: "If std::abs is called with an unsigned integral argument that cannot be converted to int by integral promotion, the program is ill-formed.". But I also read in the docs of fabs: "For integral arguments, the integral overloads of std::abs are likely better matches."

Maybe someone else with more experience in C++ can jump in and comment here?

(An additional question that comes to my mind is: If we exchange 4 of the 6 occurances of abs() with fabs() in this function "quasi_euclidean", why not also the two other ones?)

Hartmut <hardy>
Fri 04 Dec 2020 07:14:07 PM UTC, original submission:  

The unit tests in file src/bwdist.cc involving the "quasi-euclidean" distance fail on a i386 system.  This was detected in the i386 autobuilder of Debian, as can be seen in this build log.

The build enters a infinite loop when exercising the unit test:


assert (bwdist (bw, "quasi-euclidean"), out, 0.0001);


The patch attached to this bug report fixes the problem.  It simply replaces abs() by fabs() in function quasi_euclidean of file src/bwdist.cc.  I have no idea why this is necessary for i386, but the proposed patch works on all architectures.

Rafael Laboissière <rlaboiss>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #50404:  fix-bwdist-on-i386.patch added by rlaboiss (917B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by hardy
  • -email is unavailable- added by hardy
  • -email is unavailable- added by rlaboiss (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-08-28 avinoam StatusNone Fixed
        Open/ClosedOpen Closed
    2020-12-05 hardy Carbon-Copy- Added avinoam
        Carbon-Copy- Added carandraug
    2020-12-04 rlaboiss Attached File- Added fix-bwdist-on-i386.patch, #50404

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code