bugGNU Octave - Bugs: bug #45456, hsv2rgb is very strict about its...

 
 

bug #45456: hsv2rgb is very strict about its input values

Submitter:  Hartmut <hardy>
Submitted:  Wed 01 Jul 2015 06:05:19 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 29 Jul 2015 07:51:25 PM UTC, comment #15: 

Forget (and do not use) my last patches, concerning this bug report, please. It seems to me that noone reads "closed" bug reports any more.

I'll file a (slightly improved) patch to the patch tracker soon, regarding all the mentioned topics.

Hartmut <hardy>
Mon 27 Jul 2015 07:58:35 PM UTC, comment #14: 

Now that the image repo on Sourceforge is back online, could someone please review my last patch here and either give me feedback or push it into the repo?

I'll attach an improved version of this PATCH to this post, which only includes one more functional test. All the other included changes are as describe in the last post #13.

(file #34540)

Hartmut <hardy>
Mon 20 Jul 2015 09:15:09 PM UTC, comment #13: 

While double checking my new tests for those functions, I've found some more glitches in those color conversion functions:

  • rgb2ntsc always returns double values in Matlab (see Matlab documentation)
  • ntsc2rgb only accepts double values in Matlab (see Matlab documentation)
  • ntsc2rgb returns rgb values strictly between 0 and 1 in Matlab (see Matlab documentation)
  • the new private function colorspace_conversion_input_check.m had a fatal typo in it.


I attach a PATCH that does the following:

  • output data of rgb2ntsc is converted (once again) to class double. (This means converting back and forth, i.e. not run time optimal, but nicer code...)
  • ntsc2rgb still accepts all input classes, and conserves them in the output values (i.e. additional functionality compared to Matlab).
  • ntsc2rgb clips values <0 and scales values >1 for inputs of type double (for Matlab compatibility).
  • fix the new private function colorspace_conversion_input_check.m , i.e. use intmax instead of intmin.


(We should have left our fingers away from those ..ntsc functions...)

(file #34471)

Hartmut <hardy>
Sun 19 Jul 2015 04:45:14 PM UTC, comment #12: 

I went with a different approach. I moved all the input check into a private function and all the 4 functions, rgb2hsv, hsv2rg, ntsc2rgb, and rgb2ntsc use it.  This should extend support for all of the functions and any future fix will apply to all at the same time.

See http://hg.savannah.gnu.org/hgweb/octave/rev/131ce8cfaa80

Carnë Draug <carandraug>
Group Member
Sat 18 Jul 2015 07:32:55 PM UTC, comment #11: 

Thank you for this nice patch to hsv2rgb, Carné.

I have prepared TWO PATCHES in a similar way:

  • The patch to rgb2hsv.m relaxes the input checking for floats, it enables for Nd images and sparse inputs, and it adds some tests (all very, very similar to your last patch to hsv2rgb).


  • The patch to rgb2ntsc.m and ntsc2rgb.m only relaxes the input checking for floats and adds a test for it. It does not care for Nd images nor sparse data. (Sorry, I am not so fluent with this data type and dimension acrobatics...)


Both new patches have the word "relax" in their file name to distinguish them from the one before.

You might consider closing bug #45131 as well then.

(file #34462, file #34463)

Hartmut <hardy>
Thu 16 Jul 2015 05:13:02 PM UTC, comment #10: 

I'm not very fond of adding a specific tolerance.

I have pushed two changesets, one to relax the tolerance in iscolormap (and fixed functions that actually needed that intolerance), and another that rewrites the input checking for hsv2rgb.

As I mentioned below, values of hue outside the range are handled in a special manner to account for the fact that it's an angle in a cylindrical coordinate system.  For saturation and brightness, will start to return  garbage.

I also made a few adjustments for performance when converting between classes, added support for ND images (MxNx3xK), made it return the same class as the input, and allow sparse (also Matlab compatible).

I understand from your previous patch that other rgb2X and X2rgb may suffer of similar issue. Would be nice if you could submit a patch.

Carnë Draug <carandraug>
Group Member
Thu 16 Jul 2015 01:37:21 PM UTC, comment #9: 

On further investigation, apparently their input is limited to the value of hue:


carandraug:   one last check. Could you tell me what "hsv2rgb ([.5 -5 .5])" and "hsv2rgb ([-.5 .5 .5])" return too? I'm curious what they do now for values below zero
xyzwhatever:  wtf, the first gives 0.5000    0.0833    0.0833   and the second NaN


which I kind of disagree.

Carnë Draug <carandraug>
Group Member
Thu 16 Jul 2015 01:08:59 PM UTC, comment #8: 

Apparently, Matlab hsv2rgb in 2010b was quite buggy and the behaviour has changed on recent versions. On Matlab 2013, Matlab returns NaN for invalid RGB colors:


carandraug:   would you mind also tell me what "hsv2rgb ([2.9 1 .5])" returns?
carandraug:   and "hsv2rgb ([2.9 1.5 .5])"
xyzwhatever:  NaN   NaN   NaN
carandraug:   for the two examples?
xyzwhatever:  yeah both


However, I kinda disagree with part of input check. The hue in hsv (first column) is an angle on a cylinder.  Therefore we can simply do:


  ## The value of hue is an angle therefore 0 = 1 = 0° = 360°.
  ## Fix for values outside the [0 1] range which mess up the maths.
  hsv(:,1) = mod (hsv(:,1), 1);


which means that "hsv2rgb ([2.9 1 .5])", which Matlab returns NaNs, could be computed like "hsv2rgb ([0.9 1 .5])" and would still be correct.

Carnë Draug <carandraug>
Group Member
Wed 15 Jul 2015 08:58:03 PM UTC, comment #7: 

Here is an even easier (and more Matlab compatible) suggestion:

  • Use tol=2e-10 as discussed earlier.
  • For input values in the interval [-tol, 1+tol] : just do the calculation anyways. No error, no warning, no data clipping. --> This will for all reasonable image data just take care of rounding errors.
  • For input values outside of [-tol, 1+tol] : issue a warning and do the calculation anyways. --> This will produce "nonsense in, nonsense out", but since Matlab does it, we can do it as well. But I'd like to issue a warning here.


A small PATCH that does the above changes to hsv2rgb.m, rgb2hsv.m and rgb2ntsc.m is attached.

(file #34450)

Hartmut <hardy>
Sun 05 Jul 2015 06:16:40 PM UTC, comment #6: 

Regrading Comment #5:

I think it's a very good suggestion: values in the range [1,1+tol] or [-tol,0] are probably due to roundoff errors.

Values outside this range, and outside [0:1] should not
be clipped. See for example:


I1 = imread ('peppers.bmp');
% famous peppers image (3 bands image)
I2 = double (I1);
I3 = rgb2hsv (I2);
I4 = hsv2rgb (I3);
imshow (uint8 (i4));


In Matlab, the last command shows an image which is the same as the original image. If we truncate I2 in rgb2hsv, we will stop
being compatible, and I3 V channel will have only 0 and 1 values.
Octave should issue a warning, but without truncating all floating point inputs to the range [0,1]. 

Avinoam Kalma <avinoam>
Group Member
Sat 04 Jul 2015 09:01:13 PM UTC, comment #5: 

I would like to suggest the following change to those 3 functions:

  • Use a tolerance of tol=2e-10 . This tolerance is slightly smaller than 2^(-32) and so I would expect no visible changes to 32 bit images or to images with less precision. (I don't know of images with even higher bit depth.) I would assume that +-eps is too tight a tolerance here. And using the suggested value for tol would still make sure that only negligible changes to the data are done.


  • Silently clip the double input data to 0 if it is in the interval [-tol , 0]. And silently clip the input data to 1 if it is in [1, 1+tol].


  • Issue a warning if the input values are outside [-tol , 1+tol]. But do proceed with the function anyways (for Matlab compatibility).
Hartmut <hardy>
Sat 04 Jul 2015 12:47:50 PM UTC, comment #4: 

Seems I was right that Matlab just doesn't bother to do input validation of any kind.  I don't think that is the right way to go.  In the first example it leads to RGB  components well in excess (not just a few eps) of 1.

Do we want to relax the standard?  If so, how close to 1 should it be not to generate an error?  1eps?

The code could also be changed from issuing an error, to issuing a warning, and truncating all floating point inputs to the range [0,1].

Whatever gets done, should be done uniformly for these three scripts


hsv2rgb.m:51:  elseif (isfloat (hsv_map) && (any (hsv_map(:) < 0) || any (hsv_map(:) > 1)))
rgb2hsv.m:46:  elseif (isfloat (rgb) && (any (rgb(:) < 0) || any (rgb(:) > 1)))
rgb2ntsc.m:57:  elseif (isfloat (rgb) && (any (rgb(:) < 0) || any (rgb(:) > 1)))



Rik <rik5>
Group administrator
Thu 02 Jul 2015 05:28:04 AM UTC, comment #3: 


Checking in Matlab 2010b



>> hsv2rgb ([1 1 1.1])

ans =

    1.1000         0    1.1000

>> hsv2rgb ([1 0.5 1e2])

ans =

   100    50   100

>> rgb2hsv(hsv2rgb ([1 1 1.1]))

ans =

    0.8333    1.0000    1.1000

>> rgb2hsv(hsv2rgb ([1 0.5 1e2]))

ans =

    0.8333    0.5000  100.0000


Avinoam Kalma <avinoam>
Group Member
Wed 01 Jul 2015 09:06:25 PM UTC, comment #2: 

Does Matlab do any input validation?  Philosophically they tend towards a lot less input validation than Octave and are willing to accept Garbage-In/Garbage-Out.

What do the following return?


hsv2rgb ([1 1 1.1])
hsv2rgb ([1 0.5 1e2])



Rik <rik5>
Group administrator
Wed 01 Jul 2015 08:00:22 PM UTC, comment #1: 
Avinoam Kalma <avinoam>
Group Member
Wed 01 Jul 2015 06:05:19 PM UTC, original submission:  

Here is a little script to show the behavior (of "hsv2rgb.m") that I mean. It also shows the context (using "imfilter" from the image package) where this behavior disturbs the calculations:


pkg load image

H = 0.3.*ones(5);
S = 0.7.*ones(5);
V = 1.*ones(5);
HSV = cat(3, H, S, V);

f = fspecial('average',3);
V2 = imfilter(V, f);
overshoot = max(max(V2-1))
value_eps = eps
HSV2 = cat(3, H, S, V2);

RGB = hsv2rgb(HSV);
RGB2 = hsv2rgb(HSV2); % -> error


In the very last line, hsv2rgb throws an error, because its input values are not in the expected (and formally  necessary) interval between 0 and 1. The V value is slightly bigger than 1 (it is 1+eps in this example). This error is formally correct behavior. Nevertheless I would like to consider this a BUG because

  • It disturbs the calculations (as shown in the more or less realistic scenario in the script above.)
  • Matlab does NOT throw an error here. I would guess, that Matlab just cuts the V values down to 1.0 .


Notes:

  • One might also argue that the bug is on the side of imfilter in this case. Imfilter does return a slightly "wrong" result here (1+eps), it does not conserve the "class" of its input values as claimed in its help text. (Understanding "double value between 0 and 1" as a seperate "class" here.) Along this line of argumentation the solution to this problem would be that imfilter should strictly conserve its output values in the range 0..1. BUT: Matlab decided to NOT solve it this way. The Matlab version of imfilter does also return 1+eps for this example. So the Octave version of imfilter seems to be fully Matlab compatible in this respect. Nevertheless the Matlab version of hsv2rgb DOES cope with thoses slightly too big input values (and probably also with slightly too small ones). Matlab also copes with 1+5*eps as input values, for example.


  • I used the imfilter function of image-2.4.0, but this is probably not relevant here.



Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34540:  clip_ntsc2rgb_v2.patch added by hardy (3KiB - text/x-diff)
file #34471:  clip_ntsc2rgb.diff added by hardy (3KiB - text/x-diff)
file #34462:  relax_rgb2hsv.diff added by hardy (5KiB - text/x-diff - two patches as explained ("relax"))
file #34463:  relax_rgb2ntsc.diff added by hardy (2KiB - text/x-diff - two patches as explained ("relax"))
file #34450:  tolerant_rgb2hsv.diff added by hardy (3KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by hardy (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-07-27 hardy Attached File- Added clip_ntsc2rgb_v2.patch, #34540
    2015-07-20 hardy Attached File- Added clip_ntsc2rgb.diff, #34471
    2015-07-18 hardy Attached File- Added relax_rgb2hsv.diff, #34462
        Attached File- Added relax_rgb2ntsc.diff, #34463
    2015-07-16 carandraug StatusConfirmed Fixed
        Open/ClosedOpen Closed
        Release4.0.0 dev
    2015-07-15 hardy Attached File- Added tolerant_rgb2hsv.diff, #34450
    2015-07-04 rik5 Item GroupIncorrect Result Matlab Compatibility
        StatusNone Confirmed
        Summaryhsv2rgb is too strict about its input values hsv2rgb is very strict about its input values

    Back to the top

    Powered by Savane 3.13-1277.
    Corresponding source code