10000 fix alpha shift during colourspace conversion by jcupitt · Pull Request #4302 · libvips/libvips · GitHub
[go: up one dir, main page]
Skip to content

fix alpha shift during colourspace conversion #4302

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 4, 2024
Merged

fix alpha shift during colourspace conversion #4302

merged 2 commits into from
Dec 4, 2024

Conversation

jcupitt
Copy link
Member
@jcupitt jcupitt commented Dec 4, 2024

The colour functions were not shifting alpha channels, just casting them, so
operations which changed depth, like icc_transform --depth 8 on a
16-bit image, could have an incorrectly scaled alpha.

See #4301

Thanks frederikrosenberg

@jcupitt
fix alpha shift during colourspace conversion
89284bf 
The colour functions were not shifting alpha channels, just casting them, so
operations which changed depth, like `icc_transform --depth 8` on a
16-bit image, could have an incorrectly scaled alpha.

See #4301

Thanks frederikrosenberg
@jcupitt
credit in changelog
9eda7dc
@kleisauke kleisauke changed the title Fix icc depth fix alpha shift during colourspace conversion Dec 4, 2024
@kleisauke
Copy link
Member
kleisauke commented Dec 4, 2024 •
edited
Loading

As an aside, I noticed that shifting (un)signed integers seems to produce non-deterministic results.

test.py:

#!/usr/bin/env python3

import sys
import pyvips

im = pyvips.Image.new_from_file(sys.argv[1])

for fmt in ["uchar", "ushort", "uint",
            "char", "short", "int",
            "float", "double",
            "complex", "dpcomplex"]:
    x = im.cast(fmt, shift=True)
    print(fmt, x.avg())
$ curl -LO https://wsrv.nl/zebra.jpg
$ diff <(python test.py zebra.jpg) <(python test.py zebra.jpg)
3c3
< uint 1632942049.5186381
---
> uint 1632942049.5186388
6c6
< int 520656999.7738505
---
> int 520656999.7738506
UBSan details 
$ LD_PRELOAD=$ASAN_DSO python test.py zebra.jpg
uchar 96.83188217684959
ushort 24916.21924385296
../libvips/conversion/cast.c:380:4: runtime error: left shift of 160 by 24 places cannot be represented in type 'int'
    #0 0x7f894197dab0 in vips_cast_gen /home/kleisauke/libvips/build/../libvips/conversion/cast.c:380:4
    #1 0x7f8941cc14f7 in vips_region_generate /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1624:6
    #2 0x7f8941ca97de in vips_region_fill /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:881:7
    #3 0x7f8941cc0fd5 in vips_region_prepare /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1692:7
    #4 0x7f89419470f7 in vips_copy_gen /home/kleisauke/libvips/build/../libvips/conversion/copy.c:140:6
    #5 0x7f8941cc14f7 in vips_region_generate /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1624:6
    #6 0x7f8941ca97de in vips_region_fill /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:881:7
    #7 0x7f8941cc0fd5 in vips_region_prepare /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1692:7
    #8 0x7f8941c58ca7 in vips_image_write_gen /home/kleisauke/libvips/build/../libvips/iofuncs/image.c:2606:6
    #9 0x7f8941cc14f7 in vips_region_generate /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1624:6
    #10 0x7f8941ca97de in vips_region_fill /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:881:7
    #11 0x7f8941cc0fd5 in vips_region_prepare /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1692:7
    #12 0x7f8941c77f60 in sink_work /home/kleisauke/libvips/build/../libvips/iofuncs/sink.c:416:11
    #13 0x7f8941c0bb52 in vips_worker_work_unit /home/kleisauke/libvips/build/../libvips/iofuncs/threadpool.c:373:6
    #14 0x7f8941c0bb52 in vips_thread_main_loop /home/kleisauke/libvips/build/../libvips/iofuncs/threadpool.c:400:3
    #15 0x7f8941c07b36 in vips_threadset_work /home/kleisauke/libvips/build/../libvips/iofuncs/threadset.c:182:3
    #16 0x7f8941c04d31 in vips_thread_run /home/kleisauke/libvips/build/../libvips/iofuncs/thread.c:148:11
    #17 0x7f8941153d42 
8000
 (/lib64/libglib-2.0.so.0+0x72d42) (BuildId: 865a88f51bbf48869c7166397a8c69259849ea68)
    #18 0x7f89546d340a  (/usr/bin/../lib/clang/19/lib/x86_64-redhat-linux-gnu/libclang_rt.asan.so+0xd340a) (BuildId: 307751fccc0dd258dd57b3df6a8253b687e8b363)
    #19 0x7f8953e7ccd6 in start_thread (/lib64/libc.so.6+0x70cd6) (BuildId: b6c381bfdcb5e08ea82c1c39cf16580181fb6cfc)
    #20 0x7f8953f00c8b in __GI___clone3 (/lib64/libc.so.6+0xf4c8b) (BuildId: b6c381bfdcb5e08ea82c1c39cf16580181fb6cfc)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../libvips/conversion/cast.c:380:4 
uint 1632942049.5186386
char 30.534526897833825
short 7944.0962924249225
../libvips/conversion/cast.c:380:4: runtime error: left shift of 153 by 24 places cannot be represented in type 'int'
    #0 0x7f8941989700 in vips_cast_gen /home/kleisauke/libvips/build/../libvips/conversion/cast.c:380:4
    #1 0x7f8941cc14f7 in vips_region_generate /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1624:6
    #2 0x7f8941ca97de in vips_region_fill /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:881:7
    #3 0x7f8941cc0fd5 in vips_region_prepare /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1692:7
    #4 0x7f89419470f7 in vips_copy_gen /home/kleisauke/libvips/build/../libvips/conversion/copy.c:140:6
    #5 0x7f8941cc14f7 in vips_region_generate /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1624:6
    #6 0x7f8941ca97de in vips_region_fill /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:881:7
    #7 0x7f8941cc0fd5 in vips_region_prepare /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1692:7
    #8 0x7f8941c58ca7 in vips_image_write_gen /home/kleisauke/libvips/build/../libvips/iofuncs/image.c:2606:6
    #9 0x7f8941cc14f7 in vips_region_generate /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1624:6
    #10 0x7f8941ca97de in vips_region_fill /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:881:7
    #11 0x7f8941cc0fd5 in vips_region_prepare /home/kleisauke/libvips/build/../libvips/iofuncs/region.c:1692:7
    #12 0x7f8941c77f60 in sink_work /home/kleisauke/libvips/build/../libvips/iofuncs/sink.c:416:11
    #13 0x7f8941c0bb52 in vips_worker_work_unit /home/kleisauke/libvips/build/../libvips/iofuncs/threadpool.c:373:6
    #14 0x7f8941c0bb52 in vips_thread_main_loop /home/kleisauke/libvips/build/../libvips/iofuncs/threadpool.c:400:3
    #15 0x7f8941c07b36 in vips_threadset_work /home/kleisauke/libvips/build/../libvips/iofuncs/threadset.c:182:3
    #16 0x7f8941c04d31 in vips_thread_run /home/kleisauke/libvips/build/../libvips/iofuncs/thread.c:148:11
    #17 0x7f8941153d42  (/lib64/libglib-2.0.so.0+0x72d42) (BuildId: 865a88f51bbf48869c7166397a8c69259849ea68)
    #18 0x7f89546d340a  (/usr/bin/../lib/clang/19/lib/x86_64-redhat-linux-gnu/libclang_rt.asan.so+0xd340a) (BuildId: 307751fccc0dd258dd57b3df6a8253b687e8b363)
    #19 0x7f8953e7ccd6 in start_thread (/lib64/libc.so.6+0x70cd6) (BuildId: b6c381bfdcb5e08ea82c1c39cf16580181fb6cfc)
    #20 0x7f8953f00c8b in __GI___clone3 (/lib64/libc.so.6+0xf4c8b) (BuildId: b6c381bfdcb5e08ea82c1c39cf16580181fb6cfc)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../libvips/conversion/cast.c:380:4 
int 520656999.77385056
float 96.83188217684959
double 96.83188217684959
complex 96.83188217684959
dpcomplex 96.83188217684959

@jcupitt
Copy link
Member Author
jcupitt commented Dec 4, 2024

Yes, that's very odd. I see:

$ vips cast zebra.jpg x.v uint --shift && vips avg x.v
1632942049.518639
$ vips cast zebra.jpg x.v uint --shift && vips avg x.v
1632942049.518638

I had to repeat a few times to get a different answer. ushort seems fine in the repeats I did.

@jcupitt
Copy link
Member Author
jcupitt commented Dec 4, 2024

Ah, it's avg, of course:

$ vips cast zebra.jpg x.v uint --shift
$ vips avg x.v
1632942049.518639
$ vips avg x.v
1632942049.518638

I suppose the accumulator is overflowing.

@jcupitt
Copy link
Member Author
jcupitt commented Dec 4, 2024

Ah I see (sorry), avg is using double for the accumulator, so there's not enough precision. Depending on the order that subareas of the sum compute, you'll get slightly different rounding.

Maybe we should use an int64 accumulator for int types (though this is probably a minor problem).

@jcupitt jcupitt merged commit 3ba36ce into 8.16 Dec 4, 2024
10 checks passed
@jcupitt jcupitt deleted the fix-icc-depth branch December 4, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleisauke kleisauke kleisauke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcupitt @kleisauke
0