4 ms·

What could go wrong!

jeffbee 6y ago
It looks like this comment is not right. There is: public static int blue (int color) Anyway this thing has range [0, 255] and adding three of them together as an index of an int[256] doesn't seem like it would ever work regardless of the colorspace.
- mtklein 6y ago
  Red, green, and blue have been scaled by .2126, .7152, and .0722, which add up to 1.0 exactly (even in single-precision float), and if those values really were all in [0,255] range, the maximum value that could be produced by this math is only 254, due to rounding. I think what's happening here is that the image is in a format that's holding those original red, green, and blue values in a format that can hold values outside logical [0,1]. Update: sorry, the comment below me doesn't seem to have a 'respond' link so I'll just edit one in here. I totally agree with you it's good practice to document your invariants in code, but in practice it would result in the same thing... an unhandled failure with nothing better to do than crash the process. In a way (if you squint) indexing into an array of size 256 is itself documenting the invariant that the index is less than 256. It's just that the invariant itself is wrong.
  - jeffbee 6y ago
    On my planet we write those invariants in code, such as CHECK(Color.red(pixel) < 55); Or whatever.
    - gruez 6y ago
      You mean an assertion? I'm not sure how that would help. The code is still going to crash, and unless you tested for that specific case, it's not going to show up in testing either.
  - jeffbee 6y ago
    Replying to your update: the reason you write out the invariants is then it becomes perfectly obvious in code review that the method only works for certain images and isn't protected against being called with unusable images, at which point the review can say "don't land this". Truth in advertising. If this had been named "getHistogramOfSRGBBitmapElseOOBE" nobody would have stamped it because that's obviously dumb.
  - JadeNB 6y ago
    > sorry, the comment below me doesn't seem to have a 'respond' link so I'll just edit one in here. For future reference: I'll often see that for a post in the context of a larger thread, but the 'reply' link has always appeared when I navigate to the post itself (by clicking on the timestamp). Maybe you already tried that, though.
- jsmith45 6y ago
  It does normally work because they apply a luminosity matrix to make a "greyscale" version. The actual greyscale value being the sum of the three components, after applying this matrix. (Yeah, I know, the sane way is to use the matrix's inherent ability to sum the values, I mention that at the end). This is done by using the formula: ".2126f * r + .7152f * g + .0722f * b" Apparently this will not yield out of bounds values if the colors are in normal range. Yes, I've got some alarm bells going off in the back of my head about the float to integer rounding (or float multiplication rounding itself), possibly causing a value slightly too high, but it seems like that might not actually happen. Or perhaps it can only happen for some in range values that can only occur after a color profile correction. (i.e. the float versions of 8-bit sRGB never cause bad rounding, but coming from very specific other profiles might create such values). Lastly there is the possibility that the color values started out of range, so after the multiplication they can still sum to more than 256. In any case, the sane way to do this, would be to create a true grayscale image with the matrix, and pick an arbitrary color component to look at. (I.E. using the matrix for both multiplication and addition, rather than only for multiplication, and then doing addition afterwards.) I'm guessing the `blue` et al static methods clamp their outputs, so this bug would have been avoided.

It looks like this comment is not right. There is: public static int blue (int color) Anyway this thing has range [0, 255] and adding three of them together as an index of an int[256] doesn't seem like it would ever work regardless of the colorspace.

mtklein 6y ago
Red, green, and blue have been scaled by .2126, .7152, and .0722, which add up to 1.0 exactly (even in single-precision float), and if those values really were all in [0,255] range, the maximum value that could be produced by this math is only 254, due to rounding. I think what's happening here is that the image is in a format that's holding those original red, green, and blue values in a format that can hold values outside logical [0,1]. Update: sorry, the comment below me doesn't seem to have a 'respond' link so I'll just edit one in here. I totally agree with you it's good practice to document your invariants in code, but in practice it would result in the same thing... an unhandled failure with nothing better to do than crash the process. In a way (if you squint) indexing into an array of size 256 is itself documenting the invariant that the index is less than 256. It's just that the invariant itself is wrong.
- jeffbee 6y ago
  On my planet we write those invariants in code, such as CHECK(Color.red(pixel) < 55); Or whatever.
  - gruez 6y ago
    You mean an assertion? I'm not sure how that would help. The code is still going to crash, and unless you tested for that specific case, it's not going to show up in testing either.
- jeffbee 6y ago
  Replying to your update: the reason you write out the invariants is then it becomes perfectly obvious in code review that the method only works for certain images and isn't protected against being called with unusable images, at which point the review can say "don't land this". Truth in advertising. If this had been named "getHistogramOfSRGBBitmapElseOOBE" nobody would have stamped it because that's obviously dumb.
- JadeNB 6y ago
  > sorry, the comment below me doesn't seem to have a 'respond' link so I'll just edit one in here. For future reference: I'll often see that for a post in the context of a larger thread, but the 'reply' link has always appeared when I navigate to the post itself (by clicking on the timestamp). Maybe you already tried that, though.
jsmith45 6y ago
It does normally work because they apply a luminosity matrix to make a "greyscale" version. The actual greyscale value being the sum of the three components, after applying this matrix. (Yeah, I know, the sane way is to use the matrix's inherent ability to sum the values, I mention that at the end). This is done by using the formula: ".2126f * r + .7152f * g + .0722f * b" Apparently this will not yield out of bounds values if the colors are in normal range. Yes, I've got some alarm bells going off in the back of my head about the float to integer rounding (or float multiplication rounding itself), possibly causing a value slightly too high, but it seems like that might not actually happen. Or perhaps it can only happen for some in range values that can only occur after a color profile correction. (i.e. the float versions of 8-bit sRGB never cause bad rounding, but coming from very specific other profiles might create such values). Lastly there is the possibility that the color values started out of range, so after the multiplication they can still sum to more than 256. In any case, the sane way to do this, would be to create a true grayscale image with the matrix, and pick an arbitrary color component to look at. (I.E. using the matrix for both multiplication and addition, rather than only for multiplication, and then doing addition afterwards.) I'm guessing the `blue` et al static methods clamp their outputs, so this bug would have been avoided.