Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename 16-bit RGB rawmodes #8158

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Jun 21, 2024

Fixes #8021. Supersedes #7965.
This is based on #8026, but that pull request has been open for nearly two months now without any activity, and the next version of Pillow is coming soon. I'd like for this change to go out with the #7978 changes so that the deprecation timelines match.

RGB;15 → XBGR;1555
RGB;16 → BGR;565
BGR;5 → XRGB;1555
BGR;15 → XRGB;1555
BGR;16 → RGB;565
RGB;4B → XBGR;4
RGBA;4B → ABGR;4
RGBA;15 → ARGB;1555
BGRA;15 → ABGR;1555

Packers were added for XRGB;1555, RGB;565, XBGR;1555, and BGR;565. Previously it was possible to use the BGR;15 and BGR;16 modes for XRGB;1555 and RGB;565 data, but since those modes are being deprecated, these new packers are needed to keep the ability to write that data.

The unpackers were rewritten so that their names match the new rawmodes, and to reduce the usage of multiplication and division for performance reasons.

@radarhere radarhere changed the title Rename 16bit RGB rawmodes Rename 16-bit RGB rawmodes Jun 22, 2024
@radarhere
Copy link
Member

The test suite is printing a number of deprecation warnings here - https://github.com/python-pillow/Pillow/actions/runs/9616954928/job/26527638175?pr=8158#step:10:5144

For comparison, see main where deprecation warnings are not printed by the test suite - https://github.com/python-pillow/Pillow/actions/runs/9622876608/job/26544673674#step:10:4868

@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 374be0c to d763dc9 Compare June 22, 2024 14:24
@Yay295
Copy link
Contributor Author

Yay295 commented Jun 22, 2024

Those warnings should be gone now.

@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch 2 times, most recently from 1463c5a to 79d5f38 Compare June 23, 2024 15:53
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 79d5f38 to b74ff65 Compare June 25, 2024 15:59
@radarhere
Copy link
Member

Packers were added for XRGB;1555, RGB;565, XBGR;1555, and BGR;565. Previously it was possible to use the BGR;15 and BGR;16 modes for XRGB;1555 and RGB;565 data, but since those modes are being deprecated, these new packers are needed to keep the ability to write that data.

If I understand correctly, you're saying that at the moment, you can pack BGR;15 to BGR;15 and BGR;16 to BGR;16, but because those modes are deprecated, you'd like to add the ability to pack from RGB and RGBX to those rawmodes?

The modes are being deprecated because they aren't used. I don't think there is any need to try and retain their functionality.

@radarhere
Copy link
Member

radarhere commented Jun 26, 2024

to reduce the usage of multiplication and division for performance reasons.

Could you explain this? I'm not seeing anywhere that you've rewritten the C code of how a mode is packed or unpacked, so I don't understand why performance is affected.

@radarhere
Copy link
Member

The following code doesn't raise a deprecation warning.

>>> from PIL import Image
>>> im = Image.new("P", (1, 1))
>>> im.putpalette(b"0", "BGR;15")

@Yay295
Copy link
Contributor Author

Yay295 commented Jun 26, 2024

void
ImagingUnpackRGB4B(UINT8 *out, const UINT8 *in, int pixels) {
    int i, pixel;
    /* RGB, 4 bits per pixel, little-endian */
    for (i = 0; i < pixels; i++) {
        pixel = in[0] + (in[1] << 8);
        out[R] = (pixel & 15) * 17;
        out[G] = ((pixel >> 4) & 15) * 17;
        out[B] = ((pixel >> 8) & 15) * 17;
        out[A] = 255;
        out += 4;
        in += 2;
    }
}
static void
ImagingUnpackXBGR4(UINT8 *out, const UINT8 *in, const int pixels) {
    /* XBGR, 4 bits per pixel, little-endian */
    for (int i = 0; i < pixels; i++) {
        const UINT8 b = in[1] & 0x0F;
        const UINT8 g = in[0] & 0xF0;
        const UINT8 r = in[0] & 0x0F;
        out[R] = (r << 4) | r;
        out[G] = g | (g >> 4);
        out[B] = (b << 4) | b;
        out[A] = 255;
        out += 4;
        in += 2;
    }
}

The existing function multiplies each value by 17 (17 == 255 // 15) to scale from 4 to 8 bits, while the new function uses a shift and an or. The math looks like this in Python:

bytes4 = list(range(2**4))
bytes4to8accurate = [round(x * 255 / 15) for x in bytes4]
bytes4to8current = [x * 255 // 15 for x in bytes4]
bytes4to8new = [x << 4 | x for x in bytes4]

@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch 2 times, most recently from f59c072 to b3c6eac Compare June 27, 2024 22:41
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from b3c6eac to 09607dc Compare June 28, 2024 20:34
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch 2 times, most recently from 1e8cdf5 to 9ceeef8 Compare June 29, 2024 03:21
docs/deprecations.rst Outdated Show resolved Hide resolved
@radarhere
Copy link
Member

radarhere commented Jun 29, 2024

to reduce the usage of multiplication and division for performance reasons.

The existing function multiplies each value by 17 (17 == 255 // 15) to scale from 4 to 8 bits, while the new function uses a shift and an or.

Oh, I had expected that you'd rewritten the existing functions.

Taking a look,

from PIL import Image
rawmodes = {
  "RGB;15": "XBGR;1555",
  "RGB;16": "BGR;565",
  "BGR;5": "XRGB;1555",
  "BGR;15": "XRGB;1555",
  "BGR;16": "RGB;565",
  "RGB;4B": "XBGR;4",
  "RGBA;4B": "ABGR;4",
  "RGBA;15": "ABGR;1555",
  "BGRA;15": "ARGB;1555",
  "BGRA;15Z": "ARGB;1555Z",
}
for old, new in rawmodes.items():
    mode = "RGBA" if "A" in old else "RGB"
    data = bytes()
    for i in range(65536):
        data += (i).to_bytes(2, 'big')
    im_old = Image.frombytes(mode, (65536, 1), data, "raw", old, 0, 1)
    im_new = Image.frombytes(mode, (65536, 1), data, "raw", new, 0, 1)
    if im_old.tobytes() != im_new.tobytes():
        print("Differences found:", old, new)
    else:
        print("Identical:", old, new)

I see

Differences found: RGB;15 XBGR;1555
Differences found: RGB;16 BGR;565
Differences found: BGR;5 XRGB;1555
Differences found: BGR;15 XRGB;1555
Differences found: BGR;16 RGB;565
Identical: RGB;4B XBGR;4
Identical: RGBA;4B ABGR;4
Differences found: RGBA;15 ABGR;1555
Differences found: BGRA;15 ARGB;1555
Differences found: BGRA;15Z ARGB;1555Z

The ones that are identical should be able to immediately replace the existing functions - Yay295#24

However, as for the replacements that produce different output, do you think the new output is more or less accurate?

@Yay295
Copy link
Contributor Author

Yay295 commented Jun 29, 2024

I did mention it in the description of one of the commits, but I also have some Python to show the difference. For going from 5 bits to 8 bits:

bytes5 = list(range(2**5))
bytes5to8accurate = [round(x * 255 / 31) for x in bytes5]
bytes5to8current = [x * 255 // 31 for x in bytes5]
bytes5to8fast = [x << 3 | x >> 2 for x in bytes5]

The current method differs from the accurate rounding for 15 numbers, while the new method differs for only 4 numbers. For each number the difference is only 1.

For going from 6 to 8 bits the current method differs from the accurate rounding for 30 numbers, while the new method differs for only 10 numbers. Again for each number the difference is only 1.

bytes6 = list(range(2**6))
bytes6to8accurate = [round(x * 255 / 63) for x in bytes6]
bytes6to8current = [x * 255 // 63 for x in bytes6]
bytes6to8fast = [x << 2 | x >> 4 for x in bytes6]

Another benefit is that the new methods are roundtrippable. Currently going from 5 to 8 to 5 bits (or 6 to 8 to 6) will not produce the original values for most numbers.

@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 9ceeef8 to 6bfff1e Compare June 29, 2024 05:10
@radarhere
Copy link
Member

Could you mention in the documentation that the output will be different?

@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from be154cf to 46fbd74 Compare June 29, 2024 05:25
@Yay295
Copy link
Contributor Author

Yay295 commented Jun 29, 2024

I could have just committed that instead of rebasing, but I've been doing so much rebasing today I didn't think of it...

@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from c8a58cf to 58dd0ab Compare July 9, 2024 00:24
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 58dd0ab to dffa26d Compare July 16, 2024 13:10
@radarhere radarhere added the Deprecation Feature that will be removed in the future label Jul 19, 2024
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch 2 times, most recently from 587f0c5 to cd293dc Compare July 26, 2024 12:56
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from cd293dc to 461455e Compare August 1, 2024 14:58
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 461455e to 7c0a1a8 Compare August 14, 2024 13:27
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 7c0a1a8 to 1c8b533 Compare August 28, 2024 13:09
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 4678862 to 703ae0b Compare October 7, 2024 13:54
Yay295 and others added 9 commits October 12, 2024 18:33
The existing 16-bit RGB rawmodes do not follow the naming convention given in Unpack.c. These new modes do follow that convention, except since these modes do not all use the same number of bits for each band, the sizes of each band are listed.

Old → New
RGB;15 → XBGR;1555
RGB;16 → BGR;565
BGR;5 → XRGB;1555
BGR;15 → XRGB;1555
BGR;16 → RGB;565
RGB;4B → XBGR;4
RGBA;4B → ABGR;4
RGBA;15 → ABGR;1555
BGRA;15 → ARGB;1555
BGRA;15Z → ARGB;1555Z

These new rawmodes also use a slightly different conversion method. The most accurate conversion from 5 to 8 bits is "round(x * 255 / 31.0)". However, that involves floating point numbers and rounding, so it's not as fast. The current method doesn't include the rounding, allowing us to also use integer instead of floating point division. This is faster, but unfortunately not roundtrippable - when converting from 5 to 8 to 5 bits not every value stays the same. The new method is roundtrippable, even faster than the current method since it uses basic bitwise operations instead of multiplication and division, and if you compare the result to what you get with rounding and floating point numbers, it is actually more accurate.
The "BGR;15" and "BGR;16" modes being deprecated is separate from the "BGR;15" and "BGR;16" rawmodes being deprecated.
@Yay295 Yay295 force-pushed the 16bit_rgb_rawmodes branch from 703ae0b to 89bafd3 Compare October 12, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecation Feature that will be removed in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some unpackers are misnamed
3 participants