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 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.

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/Histogram...) but also as part of the
Levels tool (/Image/Colors/Levels...), the Threshold tool
(/Image/Colors/Threshold...) , and the Equalize tool
(/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