5

Im using a formula from this question:

uint8_t *rgbBuffer = malloc(imageWidth * imageHeight * 3);

// .. iterate over height and width

// from ITU-R BT.601, rounded to integers
rgbOutput[0] = (298 * (y - 16) + 409 * cr - 223) >> 8;
rgbOutput[1] = (298 * (y - 16) + 100 * cb + 208 * cr + 136) >> 8;
rgbOutput[2] = (298 * (y - 16) + 516 * cb - 277) >> 8;

Which I assume is based in the ITU-R_BT.601 formula in the wiki article.

enter image description here

However I think the formula is not quite right because the output image looks like this:

I don't actually have green hair

How do I fix the formula?

Community
  • 1
  • 1
Robert
  • 37,670
  • 37
  • 171
  • 213

2 Answers2

4

Assuming max values for the first calculation (y == 255 and cr == 255):

rgbOutput[0] = (298 * (255 - 16) + 409 * 255 - 223) >> 8;
rgbOutput[0] = (298 * 239 + 104295 - 223) >> 8;
rgbOutput[0] = (71222 + 104295 - 223) >> 8;
rgbOutput[0] = 175294 >> 8; // 175294 == 0x2ACBE
rgbOutput[0] = 684; // 684 == 0x2AC

The maximum value that rgbOutput[0] can hold is 255. You're attempting to assign 684 to it, resulting in truncation. The actual value assigned to it is 172 (0xAC).

EDIT 1

According to the formula you posted, your first calculation should be as follows:

rgbOutput[0] = ((298 * y) >> 8) + ((409 * cr) >> 8) - 223;

This results in a value of (assuming max values for y and cr) of 480, which results in truncation as well.

EDIT 2

The following equation is said to be recommended:

equation

Using this instead, your first calculation should be like this:

rgbOutput[0] = ((255 * (y - 16)) / 219) + ((179 * (cr - 128)) / 112;

This results in a value of (assuming max values for y and cr) of 480 (the same answer in EDIT 1), which results in truncation as well.

EDIT 3

See answer from @Robert for complete solution.

EDIT 4

When y == 0 and cr == 0, the value that is written to y will also result in truncation unless clamping is performed.

Fiddling Bits
  • 8,712
  • 3
  • 28
  • 46
  • Yes - you are right. I added the formula that its based on into the question. It also appears there are also some missing constants. – Robert Aug 08 '14 at 20:03
  • Great - the image looks much better now! There a just a few strange areas that I think are a result of truncation. Do you know what the most efficient way to truncate a value is? Do I just store it in an `int` and then check its size with an if statement? – Robert Aug 08 '14 at 20:10
  • Actually no worries, I found this http://codereview.stackexchange.com/q/6502/30438. Ill give the clamping a try myself! – Robert Aug 08 '14 at 20:14
  • @Robert You don't want truncation, because the RGB values should fall between 0 and 255, right? – Fiddling Bits Aug 08 '14 at 20:19
  • Yes - I meant clamping not truncation. I just modified the formula and it now works beautifully. Thank you for your help! :) Ill submit my modifications for completeness. – Robert Aug 08 '14 at 20:21
  • Okay, that sounds good. Clamp values above 255 to 255. Good luck. – Fiddling Bits Aug 08 '14 at 20:22
1

With help from @Fiddling Bits. The corrected code is like so:

uint8_t ClampIntToByte(int n) {
    n = n > 255 ? 255 : n;
    return n < 0 ? 0 : n;
}

rgbOutput[0] = ClampIntToByte(((298 * (y - 16) + 409 * cr) >> 8) - 223);
rgbOutput[1] = ClampIntToByte(((298 * (y - 16) - 100 * cb - 208 * cr) >> 8) + 136);
rgbOutput[2] = ClampIntToByte(((298 * (y - 16) + 516 * cb) >> 8) - 277);
Robert
  • 37,670
  • 37
  • 171
  • 213