Move rgb2ntsc and ntsc2rgb to image package

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Move rgb2ntsc and ntsc2rgb to image package

Rik-4
Carnë,

As you are maintainer of the image package, I'm writing to coordinate on
some of of the lesser used image functions in core Octave that should move
to the image package.  This sort of clean-up has already been done for the
statistics functions.

It seems like rgb2ntsc and ntsc2rgb are pretty specific and should be moved
to the image package.  However, the general functions rgb2gray and gray2rgb
which use the luminance channel of rgb2ntsc should be written.  I don't
mind doing that if you can take care of incorporating the other two into
the image package.

Cheers,
Rik



Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Carnë Draug
On 16 January 2018 at 16:06, Rik <[hidden email]> wrote:

> Carnë,
>
> As you are maintainer of the image package, I'm writing to coordinate on
> some of of the lesser used image functions in core Octave that should move
> to the image package.  This sort of clean-up has already been done for the
> statistics functions.
>
> It seems like rgb2ntsc and ntsc2rgb are pretty specific and should be moved
> to the image package.  However, the general functions rgb2gray and gray2rgb
> which use the luminance channel of rgb2ntsc should be written.  I don't
> mind doing that if you can take care of incorporating the other two into
> the image package.
>
> Cheers,
> Rik

ok. I think there's more functions like ind2gray and gray2ind so let
me know.  I have a patch ready for the functions you mentioned.

Carnë

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Rik-4
On 01/16/2018 10:04 AM, Carnë Draug wrote:

> On 16 January 2018 at 16:06, Rik <[hidden email]> wrote:
>> Carnë,
>>
>> As you are maintainer of the image package, I'm writing to coordinate on
>> some of of the lesser used image functions in core Octave that should move
>> to the image package.  This sort of clean-up has already been done for the
>> statistics functions.
>>
>> It seems like rgb2ntsc and ntsc2rgb are pretty specific and should be moved
>> to the image package.  However, the general functions rgb2gray and gray2rgb
>> which use the luminance channel of rgb2ntsc should be written.  I don't
>> mind doing that if you can take care of incorporating the other two into
>> the image package.
>>
>> Cheers,
>> Rik
> ok. I think there's more functions like ind2gray and gray2ind so let
> me know.  I have a patch ready for the functions you mentioned.
>
> Carnë

I added rgb2gray.m here
(http://hg.savannah.gnu.org/hgweb/octave/rev/3ad53e4793fc).  I removed
rgb2ntsc.m and ntsc2rgb.m here
(http://hg.savannah.gnu.org/hgweb/octave/rev/afbef2f579c9).  It should be
okay to push your patch for the image package now.

Thanks,
Rik


Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Carnë Draug
On 17 January 2018 at 05:25, Rik <[hidden email]> wrote:

> On 01/16/2018 10:04 AM, Carnë Draug wrote:
>> On 16 January 2018 at 16:06, Rik <[hidden email]> wrote:
>>> Carnë,
>>>
>>> As you are maintainer of the image package, I'm writing to coordinate on
>>> some of of the lesser used image functions in core Octave that should move
>>> to the image package.  This sort of clean-up has already been done for the
>>> statistics functions.
>>>
>>> It seems like rgb2ntsc and ntsc2rgb are pretty specific and should be moved
>>> to the image package.  However, the general functions rgb2gray and gray2rgb
>>> which use the luminance channel of rgb2ntsc should be written.  I don't
>>> mind doing that if you can take care of incorporating the other two into
>>> the image package.
>>>
>>> Cheers,
>>> Rik
>> ok. I think there's more functions like ind2gray and gray2ind so let
>> me know.  I have a patch ready for the functions you mentioned.
>>
>> Carnë
>
> I added rgb2gray.m here
> (http://hg.savannah.gnu.org/hgweb/octave/rev/3ad53e4793fc).  I removed
> rgb2ntsc.m and ntsc2rgb.m here
> (http://hg.savannah.gnu.org/hgweb/octave/rev/afbef2f579c9).  It should be
> okay to push your patch for the image package now.

Unlike the version of rgb2ntsc, rgb2gray should return the same data
type as the input image.  On the image package, that happens with the
imcast/im2X functions but they are not in core [1].  Not sure how to
best handle this in core without having those functions duplicated.

Also, despite what Matlab says on the documentation, they don't seem
to use that transformation matrix with only 3 decimal cases:

    >> T = rgb2gray ([1 0 0; 0 1 0; 0 0 1]);
    >> T(:,1)
    ans =
       0.298936021293776
       0.587043074451121
       0.114020904255103

so might be better to had a few more decimal cases.  At least so it's
compatible with the existing function in the image package.

Finally, the reshaping back to the original shape can be done with the
existing private function colorspace_conversion_revert [2].

Carnë

[1] http://hg.code.sf.net/p/octave/image/file/b2a85fb8b517/inst/rgb2gray.m#l50
[2] http://hg.savannah.gnu.org/hgweb/octave/file/afbef2f579c9/scripts/image/private/colorspace_conversion_revert.m

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Rik-4
On 01/17/2018 07:20 AM, Carnë Draug wrote:

> On 17 January 2018 at 05:25, Rik <[hidden email]> wrote:
>> On 01/16/2018 10:04 AM, Carnë Draug wrote:
>>> On 16 January 2018 at 16:06, Rik <[hidden email]> wrote:
>>>> Carnë,
>>>>
>>>> As you are maintainer of the image package, I'm writing to coordinate on
>>>> some of of the lesser used image functions in core Octave that should move
>>>> to the image package.  This sort of clean-up has already been done for the
>>>> statistics functions.
>>>>
>>>> It seems like rgb2ntsc and ntsc2rgb are pretty specific and should be moved
>>>> to the image package.  However, the general functions rgb2gray and gray2rgb
>>>> which use the luminance channel of rgb2ntsc should be written.  I don't
>>>> mind doing that if you can take care of incorporating the other two into
>>>> the image package.
>>>>
>>>> Cheers,
>>>> Rik
>>> ok. I think there's more functions like ind2gray and gray2ind so let
>>> me know.  I have a patch ready for the functions you mentioned.
>>>
>>> Carnë
>> I added rgb2gray.m here
>> (http://hg.savannah.gnu.org/hgweb/octave/rev/3ad53e4793fc).  I removed
>> rgb2ntsc.m and ntsc2rgb.m here
>> (http://hg.savannah.gnu.org/hgweb/octave/rev/afbef2f579c9).  It should be
>> okay to push your patch for the image package now.
> Unlike the version of rgb2ntsc, rgb2gray should return the same data
> type as the input image.  On the image package, that happens with the
> imcast/im2X functions but they are not in core [1].  Not sure how to
> best handle this in core without having those functions duplicated.
If that is the required behavior then I don't think there's any way to
avoid it re-coding some of imcast in to rgb2gray.  I will take a look at
that later.

>
> Also, despite what Matlab says on the documentation, they don't seem
> to use that transformation matrix with only 3 decimal cases:
>
>     >> T = rgb2gray ([1 0 0; 0 1 0; 0 0 1]);
>     >> T(:,1)
>     ans =
>        0.298936021293776
>        0.587043074451121
>        0.114020904255103
>
> so might be better to had a few more decimal cases.  At least so it's
> compatible with the existing function in the image package.
Sure, I'll change it to keep 6 significant figures.  Most LCD displays are
going to have, at best, 1/255 precision in display or .004.  Assuming some
display technology that we regularly use eventually has uint16 precision
that would be 1 part in 2^16 or just 1.5e-5 so keeping the matrix rounded
to 1e-6 should be good enough.

>
> Finally, the reshaping back to the original shape can be done with the
> existing private function colorspace_conversion_revert [2].
I tried using that function at first, but had to give up because I couldn't
get ND-images to work correctly.  The problem is that, unlike rgb2ntsc, the
third dimension is removed.  I can't mimic that by just deleting the
dimension from the sz vector because colorspace_conversion_revert expects 4
dimensions.  The code in question is

    if (is_nd)
      rv = reshape (rv, [sz(1:2) sz(4) sz(3)]);
      rv = permute (rv, [1 2 4 3]);

We could recode this to be more general, or we could just make rgb2gray a
one-off function.  I'm inclined to do the latter, but if you see a strong
reason for modifying colorspace_conversion_revert we could do that.

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: colorspace_conversion_input_check

Rik-4
In reply to this post by Carnë Draug
Carnë,

According to the Matlab documentation, input images for rgb2gray and
rgb2hsv can only be of type double, single, uint8, uint16.  However, the
input validation is also accepting int8 and int16:

    if (! any (strcmp (cls, {"uint8", "int8", "int16", "uint16", ...
                             "single", "double"})))
      error ("%s: %s of invalid data type '%s'", func, arg_name, cls);

Should we remove support for int8 and int16, or is there undocumented
Matlab voodoo and images of those types are accepted?

I've finished up with rgb2gray, except for this issue.

--Rik


Reply | Threaded
Open this post in threaded view
|

Re: colorspace_conversion_input_check

Doug Stewart-4


On Wed, Jan 17, 2018 at 11:47 AM, Rik <[hidden email]> wrote:
Carnë,

According to the Matlab documentation, input images for rgb2gray and
rgb2hsv can only be of type double, single, uint8, uint16.  However, the
input validation is also accepting int8 and int16:

    if (! any (strcmp (cls, {"uint8", "int8", "int16", "uint16", ...
                             "single", "double"})))
      error ("%s: %s of invalid data type '%s'", func, arg_name, cls);

Should we remove support for int8 and int16, or is there undocumented
Matlab voodoo and images of those types are accepted?

I've finished up with rgb2gray, except for this issue.

--Rik



Isn't it ok to do more than matlab?

--
DAS

Reply | Threaded
Open this post in threaded view
|

Re: colorspace_conversion_input_check

Carnë Draug
In reply to this post by Rik-4
On 17 January 2018 at 16:47, Rik <[hidden email]> wrote:

> Carnë,
>
> According to the Matlab documentation, input images for rgb2gray and
> rgb2hsv can only be of type double, single, uint8, uint16.  However, the
> input validation is also accepting int8 and int16:
>
>     if (! any (strcmp (cls, {"uint8", "int8", "int16", "uint16", ...
>                              "single", "double"})))
>       error ("%s: %s of invalid data type '%s'", func, arg_name, cls);
>
> Should we remove support for int8 and int16, or is there undocumented
> Matlab voodoo and images of those types are accepted?
>
> I've finished up with rgb2gray, except for this issue.

In Matlab, rgb2hsv fails with int8 and int16 types, while rgb2gray
accepts both int8 and int16.

The int16 is a valid image data type, and Matlab has a im2int16
function and other im2X functions will accept int16 type.  int8 is not
recognized but often works fine too.  I prefer to support all of those
data types since the code is usually the same, it's more work to
actually filter them out, and makes the image processing functions
more consistent.

Carnë

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Carnë Draug
In reply to this post by Rik-4
On 17 January 2018 at 16:22, Rik <[hidden email]> wrote:

> On 01/17/2018 07:20 AM, Carnë Draug wrote:
>> [...]
>> Finally, the reshaping back to the original shape can be done with the
>> existing private function colorspace_conversion_revert [2].
> I tried using that function at first, but had to give up because I couldn't
> get ND-images to work correctly.  The problem is that, unlike rgb2ntsc, the
> third dimension is removed.  I can't mimic that by just deleting the
> dimension from the sz vector because colorspace_conversion_revert expects 4
> dimensions.  The code in question is
>
>     if (is_nd)
>       rv = reshape (rv, [sz(1:2) sz(4) sz(3)]);
>       rv = permute (rv, [1 2 4 3]);
>
> We could recode this to be more general, or we could just make rgb2gray a
> one-off function.  I'm inclined to do the latter, but if you see a strong
> reason for modifying colorspace_conversion_revert we could do that.
>
The thing is that in a grayscale image, the 3rd dimension must be one,
the 4th dimension can't simply be shifted to the 3rd dimension like it
is happening now on core.  The test unit for ND images is also
incorrect.  I have attached a diff that fixes it.

Carnë

fix-rgb2gray-sz.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: rgb2gray

Rik-4
In reply to this post by Carnë Draug
On 01/17/2018 09:00 AM, Carnë Draug wrote:

> On 17 January 2018 at 16:47, Rik <[hidden email]> wrote:
>> Carnë,
>>
>> According to the Matlab documentation, input images for rgb2gray and
>> rgb2hsv can only be of type double, single, uint8, uint16.  However, the
>> input validation is also accepting int8 and int16:
>>
>>     if (! any (strcmp (cls, {"uint8", "int8", "int16", "uint16", ...
>>                              "single", "double"})))
>>       error ("%s: %s of invalid data type '%s'", func, arg_name, cls);
>>
>> Should we remove support for int8 and int16, or is there undocumented
>> Matlab voodoo and images of those types are accepted?
>>
>> I've finished up with rgb2gray, except for this issue.
> In Matlab, rgb2hsv fails with int8 and int16 types, while rgb2gray
> accepts both int8 and int16.
>
> The int16 is a valid image data type, and Matlab has a im2int16
> function and other im2X functions will accept int16 type.  int8 is not
> recognized but often works fine too.  I prefer to support all of those
> data types since the code is usually the same, it's more work to
> actually filter them out, and makes the image processing functions
> more consistent.

I made rgb2gray consistent with Matlab along the lines you suggested.  See
http://hg.savannah.gnu.org/hgweb/octave/rev/6c8822790411.  As Doug pointed
out, there's no reason we can't be better than Matlab so I included support
for int8 and int16 as well as a BIST test for that.

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Rik-4
In reply to this post by Carnë Draug
On 01/17/2018 09:32 AM, Carnë Draug wrote:

> On 17 January 2018 at 16:22, Rik <[hidden email]> wrote:
>> On 01/17/2018 07:20 AM, Carnë Draug wrote:
>>> [...]
>>> Finally, the reshaping back to the original shape can be done with the
>>> existing private function colorspace_conversion_revert [2].
>> I tried using that function at first, but had to give up because I couldn't
>> get ND-images to work correctly.  The problem is that, unlike rgb2ntsc, the
>> third dimension is removed.  I can't mimic that by just deleting the
>> dimension from the sz vector because colorspace_conversion_revert expects 4
>> dimensions.  The code in question is
>>
>>     if (is_nd)
>>       rv = reshape (rv, [sz(1:2) sz(4) sz(3)]);
>>       rv = permute (rv, [1 2 4 3]);
>>
>> We could recode this to be more general, or we could just make rgb2gray a
>> one-off function.  I'm inclined to do the latter, but if you see a strong
>> reason for modifying colorspace_conversion_revert we could do that.
>>
> The thing is that in a grayscale image, the 3rd dimension must be one,
> the 4th dimension can't simply be shifted to the 3rd dimension like it
> is happening now on core.  The test unit for ND images is also
> incorrect.  I have attached a diff that fixes it.

Go ahead and push the changes.  I thought the singleton dimension would
have been squeezed out, but I don't work with images much so I'm apparently
wrong.

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Carnë Draug
On 17 January 2018 at 17:48, Rik <[hidden email]> wrote:

> On 01/17/2018 09:32 AM, Carnë Draug wrote:
>> On 17 January 2018 at 16:22, Rik <[hidden email]> wrote:
>>> On 01/17/2018 07:20 AM, Carnë Draug wrote:
>>>> [...]
>>>> Finally, the reshaping back to the original shape can be done with the
>>>> existing private function colorspace_conversion_revert [2].
>>> I tried using that function at first, but had to give up because I couldn't
>>> get ND-images to work correctly.  The problem is that, unlike rgb2ntsc, the
>>> third dimension is removed.  I can't mimic that by just deleting the
>>> dimension from the sz vector because colorspace_conversion_revert expects 4
>>> dimensions.  The code in question is
>>>
>>>     if (is_nd)
>>>       rv = reshape (rv, [sz(1:2) sz(4) sz(3)]);
>>>       rv = permute (rv, [1 2 4 3]);
>>>
>>> We could recode this to be more general, or we could just make rgb2gray a
>>> one-off function.  I'm inclined to do the latter, but if you see a strong
>>> reason for modifying colorspace_conversion_revert we could do that.
>>>
>> The thing is that in a grayscale image, the 3rd dimension must be one,
>> the 4th dimension can't simply be shifted to the 3rd dimension like it
>> is happening now on core.  The test unit for ND images is also
>> incorrect.  I have attached a diff that fixes it.
>
> Go ahead and push the changes.  I thought the singleton dimension would
> have been squeezed out, but I don't work with images much so I'm apparently
> wrong.

Done http://hg.savannah.gnu.org/hgweb/octave/rev/93219261164d

I have also patched the image package to check for it during
installation http://hg.code.sf.net/p/octave/image/rev/db129c587407

Thank you for doing this.  Should I also prepare a patch to include
the conversion between indexed images?

Carnë

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Rik-4
On 01/17/2018 10:22 AM, Carnë Draug wrote:

> On 17 January 2018 at 17:48, Rik <[hidden email]> wrote:
>> On 01/17/2018 09:32 AM, Carnë Draug wrote:
>>> On 17 January 2018 at 16:22, Rik <[hidden email]> wrote:
>>>> On 01/17/2018 07:20 AM, Carnë Draug wrote:
>>>>> [...]
>>>>> Finally, the reshaping back to the original shape can be done with the
>>>>> existing private function colorspace_conversion_revert [2].
>>>> I tried using that function at first, but had to give up because I couldn't
>>>> get ND-images to work correctly.  The problem is that, unlike rgb2ntsc, the
>>>> third dimension is removed.  I can't mimic that by just deleting the
>>>> dimension from the sz vector because colorspace_conversion_revert expects 4
>>>> dimensions.  The code in question is
>>>>
>>>>     if (is_nd)
>>>>       rv = reshape (rv, [sz(1:2) sz(4) sz(3)]);
>>>>       rv = permute (rv, [1 2 4 3]);
>>>>
>>>> We could recode this to be more general, or we could just make rgb2gray a
>>>> one-off function.  I'm inclined to do the latter, but if you see a strong
>>>> reason for modifying colorspace_conversion_revert we could do that.
>>>>
>>> The thing is that in a grayscale image, the 3rd dimension must be one,
>>> the 4th dimension can't simply be shifted to the 3rd dimension like it
>>> is happening now on core.  The test unit for ND images is also
>>> incorrect.  I have attached a diff that fixes it.
>> Go ahead and push the changes.  I thought the singleton dimension would
>> have been squeezed out, but I don't work with images much so I'm apparently
>> wrong.
> Done http://hg.savannah.gnu.org/hgweb/octave/rev/93219261164d
>
> I have also patched the image package to check for it during
> installation http://hg.code.sf.net/p/octave/image/rev/db129c587407
>
> Thank you for doing this.  Should I also prepare a patch to include
> the conversion between indexed images?

Don't we have this already?

I see ind2gray and gray2ind as well as ind2rgb and rgb2ind.  Maybe they
should have a review though because ind2gray, for example, is only using a
transformation matrix with 5 significant figures.

Also, the helper function accepts any unsigned integer class so uint32
would be okay, although I don't think that is a valid image class.

--Rik

>
> Carnë
>
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Carnë Draug
On 17 January 2018 at 18:41, Rik <[hidden email]> wrote:
> On 01/17/2018 10:22 AM, Carnë Draug wrote:
>> [...]
>> Thank you for doing this.  Should I also prepare a patch to include
>> the conversion between indexed images?
>
> Don't we have this already?
>
> I see ind2gray and gray2ind as well as ind2rgb and rgb2ind.

The ind2gray and gray2ind functions are part of Matlab image toolbox,
nor part of Matlab core.  Weirdly, ind2rgb and rgb2ind appear in both:

  https://uk.mathworks.com/help/matlab/functionlist.html
  https://uk.mathworks.com/help/images/functionlist.html

Reply | Threaded
Open this post in threaded view
|

Re: Move rgb2ntsc and ntsc2rgb to image package

Rik-4
On 01/17/2018 11:00 AM, Carnë Draug wrote:

> On 17 January 2018 at 18:41, Rik <[hidden email]> wrote:
>> On 01/17/2018 10:22 AM, Carnë Draug wrote:
>>> [...]
>>> Thank you for doing this.  Should I also prepare a patch to include
>>> the conversion between indexed images?
>> Don't we have this already?
>>
>> I see ind2gray and gray2ind as well as ind2rgb and rgb2ind.
> The ind2gray and gray2ind functions are part of Matlab image toolbox,
> nor part of Matlab core.  Weirdly, ind2rgb and rgb2ind appear in both:
>
>   https://uk.mathworks.com/help/matlab/functionlist.html
>   https://uk.mathworks.com/help/images/functionlist.html
>
>
>

I'd vote to keep them in the core.  These seem like nice, still pretty
general, functions to have.  But, if you want to move them that is okay too.

I kept more statistics functions in the core than strictly called for
because they seemed useful.  We don't necessarily need to segregate
functions to maximize revenue by forcing the purchase of another toolbox.

--Rik