patchGNU Octave - Patches: patch #8709, fixes to color conversion...

 
 

patch #8709: fixes to color conversion functions (mainly rgb2ntsc and ntsc2rgb)

Submitter:  Hartmut <hardy>
Submitted:  Wed 29 Jul 2015 07:54:02 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 03 Nov 2015 06:57:47 PM UTC, comment #13: 

This looks perfectly fine to me now. Thanks for thoroughly revising this patch!

Hartmut <hardy>
Tue 03 Nov 2015 06:30:32 PM UTC, comment #12: 

I went ahead and completely removed integer support from ntsc2rgb:

http://hg.savannah.gnu.org/hgweb/octave/rev/ae83fda9929f

and made a final adjustment to the private functions:

http://hg.savannah.gnu.org/hgweb/octave/rev/1672bb8882dd

I'm closing this as done but do comment here if I missed anything. I will handle the stuff you mentioned for the image package on its own patch report.

Carnë Draug <carandraug>
Group Member
Mon 02 Nov 2015 10:13:13 PM UTC, comment #11: 

Alright, there seems to be some reasonable definition of how to save YIQ colors in integer values. But when I read this for example:

https://en.wikipedia.org/wiki/YUV#Numerical_approximations

then I see that it is for some funny reasons NOT only about scaling the possible value range [Y_min, Y_max] to [0, int_max]. But there is some additionally shifting involved.

The cited Wikipedia article is about the definition and implementation integer datatypes for YUV images (and NOT the YIQ images used by rgb2ntsc and ntsc2rgb in Octave). But I would guess there will be similar shiftings involved as well.

I am in no way against an implementation of integer types for YIQ colors in Octave. But I personally would not bother to code it. And I think they aren't too frequently used any more, either. (In contrast I have heard of integer valued Lab colors before.)

Hartmut <hardy>
Mon 02 Nov 2015 07:22:11 PM UTC, comment #10: 

We convert hsv and rgb images in integers into the [0 1] range because those are the values we expect those images to have if they were floating points.  We can easily compute the expected range of YIQ values as floating points.

Since the matrix for conversion is:


  trans = [ 0.299  0.596  0.211
            0.587 -0.274 -0.523
            0.114 -0.322  0.312 ];


the expected output range (consider RGB values between [0 1]) would be:


Y = [ 0      1.106]
I = [-0.797  0.587]
Q = [-0.322  0.426]


It wouldn't be too complicated to do such conversion. What do you think? It may not be worth the extra complication if we don't think anyone will ever find or use it. If that's so, then I agree with your first solution of not allowing floating point input at all for this function.

Carnë Draug <carandraug>
Group Member
Fri 30 Oct 2015 08:14:51 PM UTC, comment #9: 

Regarding the behavior of ntsc2rgb with integer inputs I see two possibilities:

  • ntsc2rgb could ONLY accept floating point inputs (I would suggest single and double). This would still be fully Matlab compatible, since Matlab only claims to accept doubles. This would only result in Octave being a SMALLER superset of Matlab as currently.


  • We could stick to the current input behavior in integers, i.e. transform the values 0...intmax to 0...1. This would make it impossible to display certain colors as integer values in ntsc images.


I would opt for the first solution (=do not accept ntsc integer images). Reasons:

  • I have no idea of a good definition how to interpret integer ntsc values better than the above crude simplification. (If someone else has a good definition at hand, I would then suggest do gladly use it and describe it in the help text.)


  • The user can NOT create integer valued ntsc images with Octave anyways. The inverse function rgb2ntsc.m now only outputs floating point values.


  • If you take the (new, patiently waiting in patch #8713) additional color conversion functions of the image package into account, then it is nothing extraordinary that in some colorspaces integer values can not (or only hardly) be used. For example does the function xyz2rgb.m currently only accept floating point inputs (The function lab2rgb currently only accepts floting point inputs as well, but this one could be reasonable generalized to also accept integers in the future. Its Matlab version does not accept integers though.)


Note: In patch #8713 I introduced an additional switch "only_floats" in colorspace_conversion_input_check.m for this very same purpose. I don't know how useful it would be to keep those two versions (Octave core version and image package version) of colorspace_conversion_input_check.m the identical.

Hartmut <hardy>
Wed 28 Oct 2015 10:42:16 PM UTC, comment #8: 

Since you also agree, I have then pushed those changes with a bunch more tests.  I have also added support for signed integer images (image package supports int16 but this adds support for int16 and int8).  See

http://hg.savannah.gnu.org/hgweb/octave/rev/a5ed88c9eb14
http://hg.savannah.gnu.org/hgweb/octave/rev/a278de349250
http://hg.savannah.gnu.org/hgweb/octave/rev/67e6343cd29a

One thing that I noticed while writing the tests is that the conversion from integers to double by ntsc2rgb does not really work.  The reason is that we are currently scaling the values into the [0 1] range. However, ntsc values are not meant to be on that range (do not believe Matlab documentation, it's all wrong).  Just consider the values from its sister function rgb2ntsc


octave> rgb2ntsc ([0 1 0])
ans =

   0.58700  -0.27400  -0.52300


How should we be scaling integer values for NTSC?

Carnë Draug <carandraug>
Group Member
Tue 27 Oct 2015 04:57:20 PM UTC, comment #7: 

(The listing in the last post was a bit disorderd, but I agree on your conclusions...)

I agree that the following would be a good Octave behavior regarding data classes for those color conversion functions:

  • input values: always accept int, single and double values
  • return values:
    • int types -> double
    • single -> single
    • double -> double


This would mostly result in the desired behavior that the Octave functionality is a superset of the Matlab functionality.

The only exeption would be that the Matlab version of hsv2rgb and rgb2hsv seem to return double output even for single input. But I would as well consider this a Matlab bug, and not try to reproduce it in Octave.

Note: Regarding the behavior for images/colormaps of size [1 3] I will try to again test the behavior of the current code in the repo. At the time when I created this patch, those 1-pixel images/colormaps were not properly treated (if I remember right). But I'll come back with this topic if I find that it's not settled yet :)


Hartmut <hardy>
Mon 26 Oct 2015 02:17:49 AM UTC, comment #6: 

Regarding the last part of this patch (conversion of the output back to the original class), Matlab 2010b seems to have the following behaviour:

  • ntsc2rgb
    • input of any class
    • output always double


  • hsv2rgb
    • input of floating point class only
    • output matches input (double or single)


  • rgb2hsv
    • input of any class
    • output always double (except when input is single)


  • rgb2hsv
    • input of any class
    • output always double


For Matlab compatibility, the only function where we could return the same class as the input is hsv2rgb (and then, only because Matlab does not accept other classes as input giving us freedom to choose).  So for sake of consistency, I'm thinking of also always returning a floating point for that. This means that instead of adding a "keep_class" option, I'll just remove the code that converted the values back to the original class.

I am also thinking of introducing a small Matlab incompatibility on purpose. The functions ntsc2rgb and rgb2ntsc seem to always return an image of class double, even when input is of class single. This seems a very odd behaviour, I would argue it's a Matlab bug, so I'm planning on always returning double unless the input is single.

Unless someone thinks otherwise, I'll push such change later this week.

Carnë Draug <carandraug>
Group Member
Sun 25 Oct 2015 08:18:31 PM UTC, comment #5: 


> The purpose of the cited part of my patch is to allow for color conversion of "single pixel images", having size [1 3] or [1 3].


None of those is a single pixel image (a single pixel image would be of size [1 1 3]).

[1 3] is a single color colormap so it gets treated like one. It would also not go into that path because of the "if (! iscolormap (in_arg))" above. This already works, on stable and default branches.

Something of size [3 1] seems like wrong input. Checked on Matlab R2010b and it doesn't support it. I don't we think we should either.

I have pushed two more csets related to this patch:

http://hg.savannah.gnu.org/hgweb/octave/rev/175aed5acb85

http://hg.savannah.gnu.org/hgweb/octave/rev/bc51aa0d604a

Carnë Draug <carandraug>
Group Member
Sun 25 Oct 2015 07:16:41 PM UTC, comment #4: 

Sorry for the confusion about the default and stable releases.

The purpose of the cited part of my patch is to allow for color conversion of "single pixel images", having size [1 3] or [1 3]. I.e. this command should work (and did work before the introduction of colorspace_conversion_input_check.m):

rgb2ntsc([0.8 0.5 0.3])


This feature is very useful when you just want to know the color values of one single color in a different color space. Matlab also supports this, and the new color conversion functions (ment to go into the image package with patch #8713) support this usage as well.

For a list of the intended changes by my patch, please also have a look at the closed bug report #45456 .

Hartmut <hardy>
Sun 25 Oct 2015 05:03:33 PM UTC, comment #3: 

What is the purpose of:


diff -r 4bde15a9c8bb -r 6b9ab8ff18fd scripts/image/private/colorspace_conversion_input_check.m
--- a/scripts/image/private/colorspace_conversion_input_check.m Di Jul 28 13:24:49 2015 -0400
+++ b/scripts/image/private/colorspace_conversion_input_check.m Mi Jul 29 21:47:19 2015 +0200
@@ -31,7 +31,7 @@
   if (! iscolormap (in_arg))
     if (! any (strcmp (cls, {"uint8", "uint16", "single", "double"})))
       error ("%s: %s of invalid data type '%s'", func, arg_name, cls);
-    elseif (size (in_arg, 3) != 3)
+    elseif (size (in_arg, 3) != 3 && !(size (in_arg) == [1, 3] || size (in_arg) == [3, 1]))
       error ("%s: %s must be a colormap or %s image", func, arg_name, arg_name);
     elseif (! isreal (in_arg) || ! isnumeric (in_arg))
       error ("%s: %s must be numeric and real", func, arg_name);
@@ -46,7 +46,7 @@

     ## Allow for ND images, i.e., multiple images on the 4th dimension.
     nd = ndims (in_arg);
-    if (nd == 3)
+    if (nd == 2 || nd == 3)
       is_nd = false;
     elseif (nd == 4)
       is_nd = true;


I have pushed http://hg.savannah.gnu.org/hgweb/octave/rev/b4ceb06009e0 as a single commit while I review the rest.

Carnë Draug <carandraug>
Group Member
Sun 25 Oct 2015 04:16:54 PM UTC, comment #2: 

This patch will not go into the octave 4.0.1 release. The regression you are mentioning went into the default branch (not stable). This means that it will go into 4.2, not 4.0.1.

Carnë Draug <carandraug>
Group Member
Sat 24 Oct 2015 06:27:22 PM UTC, comment #1: 

Could someone please review this small Octave Core patch before the Octave 4.0.1 release? Among other bugs it fixes a regression bug, that was newly introduced after Octave 4.0.0 and will otherwise prevent integer valued images to be processed at all (by rgb2ntsc, ntsc2rgb and indirectly maybe even rgb2gray).

Hartmut <hardy>
Wed 29 Jul 2015 07:54:02 PM UTC, original submission:  

Here is a patch that fixes some Matlab compatibility issues with the color conversion functions, mainly in rgb2ntsc.m and ntsc2rgb.m. They became visible to me, when working on the (now closed) bug report #45456. Some further details can also be found there.

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34552:  color_conversion_fixes.patch added by hardy (6KiB - text/x-diff - hg patch file)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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 logged-in users can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-11-03 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2015-07-29 hardy Attached File- Added color_conversion_fixes.patch, #34552

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code