GetPixel in UnmanagedImage class

[GoogleCodeExporter]

It seems UnmanagedImage has a SetPixel method but no equivalent GetPixel. 
Obviously using the raw image data is much faster in all applications, but 
sometimes we are just trying to check something and not caring much about 
performance.

I would suggest adding a GetPixel method to the UnmanagedImage class. I am 
attaching a proposal for such method. It still doesn't deals with the 
16bpp/48bpp case though.

Original issue reported on code.google.com by cesarso...@gmail.com on 17 Jan 2012 at 4:13

Attachments:

• GetPixel.cs

[GoogleCodeExporter]

>> It seems UnmanagedImage has a SetPixel method but no equivalent GetPixel.
Yes, this is something which always was confusing me as well.

>> but sometimes we are just trying to check something and not caring much 
about performance.
True, it happens quite often.

>> It still doesn't deals with the 16bpp/48bpp case though.
This is something I was always concerned as well.

>>  case PixelFormat.Format8bppIndexed:
>>    color.R = *ptr / 0.2125;
>>    color.G = *ptr / 0.7154; 
>>    color.B = *ptr / 0.0721;
This is something I would a bit disagree. Although I understand where it comes 
from (kind of doing inverse of RGB-to-grayscale conversion), but when analysing 
8bpp grayscale images, I am mostly interested in getting value in the [0-255] 
range. So I would better set all R/G/B components to the same value, which is 
*ptr. This also seems to be quite logical in terms of the color we get – RGB 
encodes grey values with all components set to same value.

Original comment by andrew.k...@gmail.com on 17 Jan 2012 at 4:31

[GoogleCodeExporter]

Seems fair, I would also do the same. But then I would disagree a bit on 
SetPixel doing the automatic conversion as well, because then the 
GetPixel/SetPixel pair wouldn't be symmetric. Setting a 8bpp pixel value and 
reading it afterwards wouldn't result in the same value.

However, I suppose you would agree that if documentation could explicitly tell 
what is going to happen then I don't think there would be a problem in getting 
the same value in all RGB components. Perhaps a using a boolean flag indicating 
whether to perform automatic conversion or not could be of any help in this 
case?

Original comment by cesarso...@gmail.com on 17 Jan 2012 at 4:56

[GoogleCodeExporter]

>> But then I would disagree a bit on SetPixel doing the automatic conversion
>> as well, because then the GetPixel/SetPixel pair wouldn't be symmetric

Yes, SetPixel() does this:
*ptr = (byte) ( 0.2125 * color.R + 0.7154 * color.G + 0.0721 * color.B );

But, what would you propose else? We cannot do anything else when going from 
RGB to grayscale.

>> GetPixel/SetPixel pair wouldn't be symmetric
It is hard to expect them to be symmetric anyway. There are many RGB values 
which can result into the same intensity value of grayscale image. Once 
conversion is done, there is no way back.

Original comment by andrew.k...@gmail.com on 17 Jan 2012 at 5:13

[GoogleCodeExporter]

But there are other ways to perform this conversion, aren't there? Why chose 
this particular one (even if it is the most adequate in most cases)?

Perhaps the mapping strategy could be given by the specification of one of the 
Filters.Grayscale algorithms as an optional parameter. The default can remain 
being the current approach. If the method could accept an optional Grayscale 
object, we could use the [Red/Green/Blue]Coefficient property to indicate which 
values should be used in both Get and Set methods.

I would also suggest adding other static properties in the CommonAlgorithms 
class for the average method.

What do you think?

Original comment by cesarso...@gmail.com on 17 Jan 2012 at 5:43

[GoogleCodeExporter]

>> I would also suggest adding other static properties in the CommonAlgorithms
>> class for the average method.
This sounds fine to me.

>> But there are other ways to perform this conversion, aren't there?
>> Why chose this particular one (even if it is the most adequate in most 
cases)?
Few things ...
1) As you mentioned in the beginning, the Set/GetPixel() are not aimed for some 
real image processing. These are aimed just for quick drawing or checking pixel 
value. If so, then I don't think there is a big issues about the fact that 
SetPixel() uses some predefined grayscaling algorithm.
2) If user wants to do RGB-to-grayscale conversion according to his own 
algorithm, then he still can do it manually by setting pixel value using one of 
the gray colors (where R=G=B). The the gray value will be same R or G or B as 
long as SetPixel() uses coefficient which sum is 1, which is the normal case.

Obviously we can do this extra flexibility, but the question is - do we need 
it? If these methods are just for something quick-n-dirty, then I am not sure 
people will care much about the extra optional parameter.

And, as I mentioned before, this flexibility will not solve the issue you 
mentioned previously - conversion will never be symmetric.

Original comment by andrew.k...@gmail.com on 17 Jan 2012 at 8:57

[GoogleCodeExporter]

Two more comments:
1) I think an exception should be thrown if user tries to get access to pixels 
which are out of the image. What do you think?
2) R/G/B properties of Color structure are read only. Was the code tested at 
all?

Original comment by andrew.k...@gmail.com on 18 Jan 2012 at 8:33

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Oops, sorry about that. It was meant to be only a proposal, so I didn't really 
test it. I am attaching a proper SVN patch this time. And about your last 
comment: seems fair and makes sense, there is no need for extra flexibility 
since the goal is just to perform quick checks.

Original comment by cesarso...@gmail.com on 18 Jan 2012 at 9:19

Attachments:

• UnmanagedImage.cs.patch

[GoogleCodeExporter]

1) Added UnmanagedImage.GetPixel() method, which allows to get pixel out of 
8/24/32 bpp image.
2) Added UnmanagedImage.SetPixel(int, int, byte) method, which allows setting 
pixel with all color component set to same value (for grayscale image this 
method is more intuitive since there is no RGB-to-grayscale conversion).
3) Added unit tests for set/get pixel.

Committed in revision 1658. Will be released in version 2.2.4.

Original comment by andrew.k...@gmail.com on 18 Jan 2012 at 9:35

• Changed state: Fixed
• Added labels: Milestone-Coming, Project-Imaging

[GoogleCodeExporter]

Original comment by andrew.k...@gmail.com on 23 Feb 2012 at 9:15

• Changed state: Released
• Removed labels: Milestone-Coming