Re: incorrect mask handling in histogram calculation

2001-02-05 Thread Jay Cox 

Roel Schroeven wrote:
 
 Austin Donnelly wrote:
 
  So, fixing this bug means that the Levels tool, the Threshold tool,
  and the Equalise tool will all also change their behaviour: currently
  they use a histogram of the entire layer, but restrict their changes
  to the current selection.  Fixing the bug means that the histogram
  will only be calculated for within the selection too.  Is this the
  desired behaviour?
 
 If the other tools pass a null pointer for the mask, the histogram is still
 calculated for the entire layer, so I think this shouldn't be a problem.

I have commited this fix (and also your median calculation fix) to both the
stable and devel branches of gimp.  The original behaviour resulted in a totaly
corrupt the histogram if you tried to use it with a mask.

I'm not sure if the tools should be modified to stop computing the histogram
through the current mask or not.  Clearly the original intent of the code was
to use the mask in the calculation.

Thanks for digging these bugs up,
Jay Cox
[EMAIL PROTECTED]

Re: incorrect mask handling in histogram calculation

2001-02-03 Thread Austin Donnelly 

On Saturday, 3 Feb 2001, Roel Schroeven wrote:

 In gimphistogram.c there is a function to calculate the histogram for a 
 subregion, declared as follows:
 
 gimp_histogram_calculate_sub_region (GimpHistogram *histogram,
 PixelRegion   *region,
 PixelRegion   *mask)
 
 In that function we have this code snippet:
 
   if (mask)
{
  gdouble masked;
 
  src = region-data;
  msrc = region-data;
 
 I would think that msrc ought to be a pointer into the mask data instead 
 of the region data, like this:
 
msrc = mask-data;

Yes, this is indeed a bug.

However, its not quite that simple.

The histogram code is used by a number of other tools: as part of the
histogram tool (Image/Image/Histogram...) but also as part of the
Levels tool (Image/Image/Colors/Levels...), the Threshold tool
(Image/Image/Colors/Threshold...) , and the Equalize tool
(Image/Image/Colors/Auto/Equalize).

All except the histogram tool care about the current mask (ie
selection), since they restrict themselves to only operating on that
part of the image.

The histogram tool always gives the full histogram for the current
layer, ie it ignores the current selection.

So, fixing this bug means that the Levels tool, the Threshold tool,
and the Equalise tool will all also change their behaviour: currently
they use a histogram of the entire layer, but restrict their changes
to the current selection.  Fixing the bug means that the histogram
will only be calculated for within the selection too.  Is this the
desired behaviour?

Austin

Re: incorrect mask handling in histogram calculation

2001-02-03 Thread Roel Schroeven 

Austin Donnelly wrote:

 So, fixing this bug means that the Levels tool, the Threshold tool,
 and the Equalise tool will all also change their behaviour: currently
 they use a histogram of the entire layer, but restrict their changes
 to the current selection.  Fixing the bug means that the histogram
 will only be calculated for within the selection too.  Is this the
 desired behaviour?

If the other tools pass a null pointer for the mask, the histogram is still
calculated for the entire layer, so I think this shouldn't be a problem.

incorrect mask handling in histogram calculation

2001-02-02 Thread Roel Schroeven 

I just downloaded (and installed and build) the 1.2.1. sources, and I 
was looking at the histogram code (because I wonder why the histogram 
always looks totally different than histograms from Photoshop or Paint 
Shop Pro) and I saw something (unrelated to what I was doing) that looks 
like a bug.

In gimphistogram.c there is a function to calculate the histogram for a 
subregion, declared as follows:

gimp_histogram_calculate_sub_region (GimpHistogram *histogram,
PixelRegion   *region,
PixelRegion   *mask)

In that function we have this code snippet:

  if (mask)
   {
 gdouble masked;

 src = region-data;
 msrc = region-data;

I would think that msrc ought to be a pointer into the mask data instead 
of the region data, like this:

   msrc = mask-data;


Regards,
Roel Schroeven