After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 530077 - Unsharp Mask: procedure of finding center value is probably incorrect
Unsharp Mask: procedure of finding center value is probably incorrect
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: Plugins
2.4.x
Other All
: Normal minor
: 2.4
Assigned To: GIMP Bugs
GIMP Bugs
Depends on:
Blocks:
 
 
Reported: 2008-04-26 15:08 UTC by whwhwhwh
Modified: 2008-10-30 20:12 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description whwhwhwh 2008-04-26 15:08:53 UTC
Please describe the problem:
static gint gen_convolve_matrix (gdouble   radius, gdouble **cmatrix_p):
......
  /* find center val -- calculate an odd number of quanta to make it symmetric,
   * even if the center point is weighted slightly higher than others. */
  sum = 0;
  for (j = 0; j <= 50; j++)
    sum += exp (- SQR (0.5 + 0.02 * j) / (2 * SQR (std_dev)));
......

Steps to reproduce:
1. check the for-loop above;
2. 
3. 


Actual results:
the center value will be the numeric integration from 0.5 to 1.5

Expected results:
shouldn't it be the integration from -0.5 to +0.5, for the sake of symmetry?

Does this happen every time?
Yes.

Other information:
Comment 1 Sven Neumann 2008-04-28 11:26:12 UTC
What procedure are you referring to? Can you show us an example where this code produces a wrong result?
Comment 2 whwhwhwh 2008-04-29 02:18:04 UTC
sorry, I cannot show you an example where this code produces a wrong result, because the appearance of the unsharped images have been "accepted" by most users over all these years, and appearantly no one has complained about it.

I've confirmed with the original developer, Winston Chang, and he himself agreed that there's probably a bug here.

Theoretically, I believe the function should look like this:

/* generates a 1-D convolution matrix to be used for each pass of
 * a two-pass gaussian blur.  Returns the length of the matrix.
 */
static gint
gen_convolve_matrix (gdouble   radius,
                     gdouble **cmatrix_p)
{
  gdouble *cmatrix;
  gdouble  std_dev;
  gdouble  sum;
  gint     matrix_length;
  gint     i, j;

  /* we want to generate a matrix that goes out a certain radius
   * from the center, so we have to go out ceil(rad-0.5) pixels,
   * inlcuding the center pixel.  Of course, that's only in one direction,
   * so we have to go the same amount in the other direction, but not count
   * the center pixel again.  So we double the previous result and subtract
   * one.
   * The radius parameter that is passed to this function is used as
   * the standard deviation, and the radius of effect is the
   * standard deviation * 2.  It's a little confusing.
   */
  radius = fabs (radius) + 1.0;

  std_dev = radius;
  radius = std_dev * 2;

  /* go out 'radius' in each direction */
  matrix_length = 2 * ceil (radius - 0.5) + 1;
  if (matrix_length <= 0)
    matrix_length = 1;

  *cmatrix_p = g_new (gdouble, matrix_length);
  cmatrix = *cmatrix_p;

  /*  Now we fill the matrix by doing a numeric integration approximation
   * from -2*std_dev to 2*std_dev, sampling 50 points per pixel.
   * We do the bottom half, mirror it to the top half, then compute the
   * center point.  Otherwise asymmetric quantization errors will occur.
   *  The formula to integrate is e^-(x^2/2s^2).
   */

  /* first we do the top (right) half of matrix */
  for (i = matrix_length / 2 + 1; i < matrix_length; i++)
    {
      gdouble base_x = i - (matrix_length / 2) - 0.5;

      sum = 0;
      for (j = 1; j <= 50; j++)
        {
          if (base_x + 0.02 * j <= radius)
            sum += exp (- SQR (base_x + 0.02 * j) / (2 * SQR (std_dev)));
        }

      cmatrix[i] = sum / 50;
    }

  /* mirror the thing to the bottom half */
  for (i = 0; i <= matrix_length / 2; i++)
    cmatrix[i] = cmatrix[matrix_length - 1 - i];

  /* find center val -- calculate an odd number of quanta to make it symmetric,
   * even if the center point is weighted slightly higher than others. */
  sum = 0;
  for (j = 0; j <= 50; j++)
    sum += exp (- SQR (-0.5 + 0.02 * j) / (2 * SQR (std_dev)));
    /* modified by wanghao, which was:
    sum += exp (- SQR (0.5 + 0.02 * j) / (2 * SQR (std_dev)));
    */
  cmatrix[matrix_length / 2] = sum / 51;

  /* normalize the distribution by scaling the total sum to one */
  sum = 0;
  for (i = 0; i < matrix_length; i++)
    sum += cmatrix[i];

  for (i = 0; i < matrix_length; i++)
    cmatrix[i] = cmatrix[i] / sum;

  return matrix_length;
}
Comment 3 Sven Neumann 2008-04-29 08:29:45 UTC
So far you did not even tell us where this code is located and which procedure is involved.
Comment 4 whwhwhwh 2008-04-29 08:39:12 UTC
sorry, I thought I had specified it somewhere in the pull-down list when I reported the bug.

Here it is:
GIMP version 2.4.5 ---> Plugins ---> unsharp.c ---> gen_convolve_matrix() function.
Comment 5 Sven Neumann 2008-05-05 14:04:27 UTC
The difference is hardly visible, but I still think that the bug report is valid. I have changed the code in both branches:

2008-05-05  Sven Neumann  <sven@gimp.org>

	* plug-ins/common/unsharp-mask.c (gen_convolve_matrix): fixed
	algorithm to calculate the center value. Fixes bug #530077.