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

BGR;15/16 scaling #7970

Closed
Yay295 opened this issue Apr 12, 2024 · 7 comments
Closed

BGR;15/16 scaling #7970

Yay295 opened this issue Apr 12, 2024 · 7 comments

Comments

@Yay295
Copy link
Contributor

Yay295 commented Apr 12, 2024

Currently when getting a BGR;15/16 pixel value, the result is scaled from 0-31 to 0-255.

static void
get_pixel_BGR15(Imaging im, int x, int y, void *color) {
UINT8 *in = (UINT8 *)&im->image8[y][x * 2];
UINT16 pixel = in[0] + (in[1] << 8);
char *out = color;
out[0] = (pixel & 31) * 255 / 31;
out[1] = ((pixel >> 5) & 31) * 255 / 31;
out[2] = ((pixel >> 10) & 31) * 255 / 31;
}
static void
get_pixel_BGR16(Imaging im, int x, int y, void *color) {
UINT8 *in = (UINT8 *)&im->image8[y][x * 2];
UINT16 pixel = in[0] + (in[1] << 8);
char *out = color;
out[0] = (pixel & 31) * 255 / 31;
out[1] = ((pixel >> 5) & 63) * 255 / 63;
out[2] = ((pixel >> 11) & 31) * 255 / 31;
}

Storing one of these pixels is also scaled, though using a different algorithm.

Pillow/src/_imaging.c

Lines 607 to 623 in de18f55

if (!strcmp(im->mode, "BGR;15")) {
UINT16 v = ((((UINT16)r) << 7) & 0x7c00) +
((((UINT16)g) << 2) & 0x03e0) +
((((UINT16)b) >> 3) & 0x001f);
ink[0] = (UINT8)v;
ink[1] = (UINT8)(v >> 8);
ink[2] = ink[3] = 0;
return ink;
} else if (!strcmp(im->mode, "BGR;16")) {
UINT16 v = ((((UINT16)r) << 8) & 0xf800) +
((((UINT16)g) << 3) & 0x07e0) +
((((UINT16)b) >> 3) & 0x001f);
ink[0] = (UINT8)v;
ink[1] = (UINT8)(v >> 8);
ink[2] = ink[3] = 0;
return ink;

Should scaling be done at all for these modes when getting/setting pixels? Also, the pixel order is backwards. It seems that these functions are actually converting to/from RGB rather than just returning the BGR;15/16 values.

@radarhere
Copy link
Member

radarhere commented Apr 13, 2024

These modes aren't actually used by any image format (the rawmodes are, but not the modes). So a discussion about how they should behave is just about consistency with how the rest of Pillow behaves.

But speaking of the fact that they aren't used

If your goal is to progressively ensure that all Pillow operations are supported for all modes, I don't think building out support for them in a test-driven development fashion is a worthwhile endeavour if there's no evidence that they would be used.

I'm more in favour of the idea from #2228 of just removing BGR;15, BGR;16 and BGR;24 as modes entirely.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 13, 2024

I agree with removing them, but I think that will require a deprecation period. Slightly changing how they work doesn't require a deprecation period, so it could be done at the same time.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 13, 2024

Also, the two converters changed in #7972 should be added as packers. There's not really any difference between a packer, an unpacker, and a converter, so they could all be grouped into one list.

@radarhere
Copy link
Member

radarhere commented Apr 15, 2024

I've created #7978 to deprecate them.

Slightly changing how they work doesn't require a deprecation period, so it could be done at the same time.

The modes are currently supported for image creation, converting to the mode, access, unpacking and putdata(). There's no way to use the final image result for these modes except by getting the pixel values. So if you're using these modes, and then you find the scale of the pixel values returned is different, you have to rewrite your code.

Introducing a breaking change to a feature at the same time as deprecating it seems counterproductive.

Also, the two converters changed in #7972 should be added as packers. There's not really any difference between a packer, an unpacker, and a converter, so they could all be grouped into one list.

I don't see anything special about converting RGB to BGR;15 or BGR;16 that means we need packers for them specifically. If you want to restructure packers, unpackers and converters in general, then I suggest you start a new conversation about that.

@Yay295
Copy link
Contributor Author

Yay295 commented Apr 15, 2024

How does packing work without a packer? There is currently a packer from BGR;15/16 to BGR;15/16, but there is not a packer from RGB to BGR;15/16. There already are unpackers from BGR;15/16 to RGB.

@radarhere
Copy link
Member

How does packing work without a packer?

You think that Pillow is currently packing without a packer, for some scenario? Could you provide an example of that scenario?

There is currently a packer from BGR;15/16 to BGR;15/16,

Yes.

but there is not a packer from RGB to BGR;15/16.

...so? I don't see how that contradicts anything, or why there would have to be at the moment.

There already are unpackers from BGR;15/16 to RGB.

Yes.

I think you're drifting off-topic from BGR;* scaling, and this would be better asked in a new conversation.

@radarhere
Copy link
Member

#7978 has been merged, deprecating these modes, so I'm in favour of closing this, rather than introducing a breaking change to deprecated functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants