Skip to content

Code Review #2

@Stephen-Seo

Description

@Stephen-Seo

Code Review

Handling of Alpha Channel

I'm having some troubles running the code. I figured out that I needed to convert images to .pxa format with the supplied Python scripts, but there are some issues I've noticed.

This bool constant is used in a way that doesn't actually check if the supplied image has an alpha channel:

#define NO_ALPHA true // SET THIS FALSE IF YOU WANT TO PROCESS AN ALPHA CHANNEL

As a result, the following snippet will use the RG channels in an RGB image by default (using a depth of 2 instead of 3):

int effectiveDepth = depth;
if (NO_ALPHA) {
effectiveDepth -= 1;
cudaMemcpy(outputPixelArrays[depth - 1], inputPixelArrays[depth - 1], width * height * sizeof(double),
cudaMemcpyHostToHost);
}

I think it would be better to count the number of channels, and from there determine based on the bool constant to remove a channel or not.

Buffer Overflow Invalidating Pointer

I've been getting segmentation faults trying out the program, and I've debugged it to the point where I think I found a buffer overflow.

void imageHandling::loadImage(FILE *file, double **pixelArrays, int width, int height, int depth) {
int offset = 0;
while (!feof(file)) {
for (int i = 0; i < depth; i++) {
double tval;
fscanf(file, "%lf", &tval);
pixelArrays[i][offset] = tval;
}
offset += 1;
}
fclose(file);
}

I debugged the value of offset and discovered that pixelArrays is being assigned to 1 extra time, which overwrote the outputPixelArrays[0] pointer to an invalid value (on my machine), causing the segmentation fault. The following snippet should fix this issue:

void imageHandling::loadImage(FILE *file, double **pixelArrays, int width, int height, int depth) {
  int offset = 0;
  while (!feof(file)) {
      for (int i = 0; i < depth; i++) {
          double tval;
          fscanf(file, "%lf", &tval);
          if (feof(file)) {
              break;
          }
          pixelArrays[i][offset] = tval;
      }
      offset += 1;
  }
  fclose(file);
}

Note I've added a if (feof(file)) { break; } which should prevent that extra write from occurring (I've tested this to work).

Clarity

Looking through the code, there are comments within functions that help describe what's going on, but I think it is lacking comments that describe the purpose and usage of each function, which I find confusing. If I have to do some research on my own to understand the code, then I think more comments would be useful here (though I can't really say I'm not somewhat guilty of having the same issue in my code).

ETC

I tested all the available options and I'm not sure if I'm setting the parameters correctly as I end up with images with colored grain added to the original (I'm not sure if there's an option to use the denoiser without adding noise, and I'm not sure if that's necessary for this to work). I think it would be helpful to post reasonable default values as a reference.

Other than the buffer overflow, things look good. I'm just confused on how to use this properly though.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions