From 0d665ba5a39b6d1f70b6c57e8c25745ce1ca9222 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 24 Aug 2023 12:19:02 +0100 Subject: [PATCH 1/5] strip icc profile when gamma changes When converting scRGB to and from sRGB, the image gamma changes. This breaks all ICC profiles causing a range of problems downstream. This patch removes any profile on sRGB <-> scRGB conversion. See https://github.com/libvips/libvips/issues/3621 --- libvips/colour/sRGB2scRGB.c | 18 ++++++++++++------ libvips/colour/scRGB2sRGB.c | 16 ++++++++++++---- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/libvips/colour/sRGB2scRGB.c b/libvips/colour/sRGB2scRGB.c index d793f50741..5656c13c12 100644 --- a/libvips/colour/sRGB2scRGB.c +++ b/libvips/colour/sRGB2scRGB.c @@ -230,18 +230,24 @@ vips_sRGB2scRGB_build(VipsObject *object) if (vips_check_bands_atleast(class->nickname, in, 3)) return -1; + // if we're coming from 8-bit sRGB, we are changing the gamma, so any + // profile on the image can no longer work (and will cause horrible + // problems in any downstream colour handling) + if (in->Type == VIPS_INTERPRETATION_sRGB) { + if (vips_copy(in, &t[0], NULL)) + return -1; + in = t[0]; + vips_image_remove(in, VIPS_META_ICC_NAME); + } + format = in->Type == VIPS_INTERPRETATION_RGB16 ? VIPS_FORMAT_USHORT : VIPS_FORMAT_UCHAR; if (in->BandFmt != format) { - if (vips_cast(in, &t[0], format, NULL)) + if (vips_cast(in, &t[1], format, NULL)) return -1; + in = t[1]; } - else { - t[0] = in; - g_object_ref(t[0]); - } - in = t[0]; out = vips_image_new(); if (vips_image_pipelinev(out, diff --git a/libvips/colour/scRGB2sRGB.c b/libvips/colour/scRGB2sRGB.c index c0ed57e5f1..468b41245d 100644 --- a/libvips/colour/scRGB2sRGB.c +++ b/libvips/colour/scRGB2sRGB.c @@ -210,17 +210,25 @@ vips_scRGB2sRGB_build(VipsObject *object) case 8: interpretation = VIPS_INTERPRETATION_sRGB; format = VIPS_FORMAT_UCHAR; + + // 8-bit RGB has a gamma, so any profile on the image can no longer + // work (and will cause horrible problems in any downstream colour + // handling) + if (vips_copy(in, &t[0], NULL)) + return -1; + in = t[0]; + vips_image_remove(in, VIPS_META_ICC_NAME); + break; default: - vips_error(class->nickname, - "%s", _("depth must be 8 or 16")); + vips_error(class->nickname, "%s", _("depth must be 8 or 16")); return -1; } - if (vips_cast_float(in, &t[0], NULL)) + if (vips_cast_float(in, &t[1], NULL)) return -1; - in = t[0]; + in = t[1]; out = vips_image_new(); if (vips_image_pipelinev(out, From 3a6d39a67f7914bcfc7629535feea598bb14eea3 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 24 Aug 2023 13:53:13 +0100 Subject: [PATCH 2/5] handle scRGB in convert for saveable --- libvips/foreign/foreign.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libvips/foreign/foreign.c b/libvips/foreign/foreign.c index 21b9724f38..65e2f59624 100644 --- a/libvips/foreign/foreign.c +++ b/libvips/foreign/foreign.c @@ -1433,7 +1433,6 @@ vips__foreign_convert_saveable(VipsImage *in, VipsImage **ready, in->Type != VIPS_INTERPRETATION_CMYK && in->Type != VIPS_INTERPRETATION_sRGB && in->Type != VIPS_INTERPRETATION_RGB16 && - in->Type != VIPS_INTERPRETATION_scRGB && vips_colourspace_issupported(in) && (saveable == VIPS_SAVEABLE_RGB || saveable == VIPS_SAVEABLE_RGBA || From f15643a1d216c19e008ce83585c019e7fe443b93 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 24 Aug 2023 13:53:54 +0100 Subject: [PATCH 3/5] note changes --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index 9d61e75230..c37bc743a7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -22,6 +22,7 @@ - swap built-in profiles with ICC v4 variants [kleisauke] - remove libgsf dependency in favor of libarchive [kleisauke] - better chunking for small shrinks [jcupitt] +- improve scRGB handling [jcupitt] 15/8/23 8.14.4 From dc148f998d8e6ca3410c1c3faad2c2c6597e1072 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 24 Aug 2023 14:07:32 +0100 Subject: [PATCH 4/5] also drop profile for scRGB <-> RGB16 ... since RGB16 also has a gamma --- libvips/colour/LabQ2sRGB.c | 3 +-- libvips/colour/sRGB2scRGB.c | 16 +++++++--------- libvips/colour/scRGB2sRGB.c | 17 ++++++++--------- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/libvips/colour/LabQ2sRGB.c b/libvips/colour/LabQ2sRGB.c index 822d5adb07..c5c953f987 100644 --- a/libvips/colour/LabQ2sRGB.c +++ b/libvips/colour/LabQ2sRGB.c @@ -372,8 +372,7 @@ vips_col_scRGB2sRGB_16(float R, float G, float B, { vips_col_make_tables_RGB_16(); - return vips_col_scRGB2sRGB(65536, vips_Y2v_16, - R, G, B, r, g, b, og); + return vips_col_scRGB2sRGB(65536, vips_Y2v_16, R, G, B, r, g, b, og); } /* Turn scRGB into BW. Return or=1 for out of gamut - g will contain an diff --git a/libvips/colour/sRGB2scRGB.c b/libvips/colour/sRGB2scRGB.c index 5656c13c12..44264ef32d 100644 --- a/libvips/colour/sRGB2scRGB.c +++ b/libvips/colour/sRGB2scRGB.c @@ -230,15 +230,13 @@ vips_sRGB2scRGB_build(VipsObject *object) if (vips_check_bands_atleast(class->nickname, in, 3)) return -1; - // if we're coming from 8-bit sRGB, we are changing the gamma, so any - // profile on the image can no longer work (and will cause horrible - // problems in any downstream colour handling) - if (in->Type == VIPS_INTERPRETATION_sRGB) { - if (vips_copy(in, &t[0], NULL)) - return -1; - in = t[0]; - vips_image_remove(in, VIPS_META_ICC_NAME); - } + // we are changing the gamma, so any profile on the image can no longer + // work (and will cause horrible problems in any downstream colour + // handling) + if (vips_copy(in, &t[0], NULL)) + return -1; + in = t[0]; + vips_image_remove(in, VIPS_META_ICC_NAME); format = in->Type == VIPS_INTERPRETATION_RGB16 ? VIPS_FORMAT_USHORT diff --git a/libvips/colour/scRGB2sRGB.c b/libvips/colour/scRGB2sRGB.c index 468b41245d..fef95e92d9 100644 --- a/libvips/colour/scRGB2sRGB.c +++ b/libvips/colour/scRGB2sRGB.c @@ -201,6 +201,14 @@ vips_scRGB2sRGB_build(VipsObject *object) if (vips_check_bands_atleast(class->nickname, in, 3)) return -1; + // we are changing the gamma, so any profile on the image can no longer + // work (and will cause horrible problems in any downstream colour + // handling) + if (vips_copy(in, &t[0], NULL)) + return -1; + in = t[0]; + vips_image_remove(in, VIPS_META_ICC_NAME); + switch (scRGB2sRGB->depth) { case 16: interpretation = VIPS_INTERPRETATION_RGB16; @@ -210,15 +218,6 @@ vips_scRGB2sRGB_build(VipsObject *object) case 8: interpretation = VIPS_INTERPRETATION_sRGB; format = VIPS_FORMAT_UCHAR; - - // 8-bit RGB has a gamma, so any profile on the image can no longer - // work (and will cause horrible problems in any downstream colour - // handling) - if (vips_copy(in, &t[0], NULL)) - return -1; - in = t[0]; - vips_image_remove(in, VIPS_META_ICC_NAME); - break; default: From 59e8b6c98444f4f58a2a892d87f939ec33d29531 Mon Sep 17 00:00:00 2001 From: Kleis Auke Wolthuizen Date: Fri, 13 Oct 2023 17:03:23 +0200 Subject: [PATCH 5/5] Fix CI --- ChangeLog | 2 +- libvips/foreign/jxlsave.c | 19 +++++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 68d42d7e5d..28c213b3d6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -30,12 +30,12 @@ - threaded write in tiffsave for tiled JPEG and JPEG2000 [jcupitt] - add vips_thread_execute() to the public API [jcupitt] - add "keep" flag to foreign savers, deprecate "strip" [a3mar] +- improve scRGB handling [jcupitt] 18/9/23 8.14.5 - fix a crash with alpha plus icc_import and icc_export [jcupitt] - fix a crash in jxlsave [jcupitt] -- improve scRGB handling [jcupitt] 15/8/23 8.14.4 diff --git a/libvips/foreign/jxlsave.c b/libvips/foreign/jxlsave.c index 8d3b13169a..19230e6e53 100644 --- a/libvips/foreign/jxlsave.c +++ b/libvips/foreign/jxlsave.c @@ -66,6 +66,8 @@ * - add animation support * * - libjxl is currently missing error messages (I think) + * + * - add support encoding images with > 4 bands. */ #define OUTPUT_BUFFER_SIZE (4096) @@ -226,7 +228,7 @@ vips_foreign_save_jxl_build(VipsObject *object) { VipsForeignSave *save = (VipsForeignSave *) object; VipsForeignSaveJxl *jxl = (VipsForeignSaveJxl *) object; - VipsImage **t = (VipsImage **) vips_object_local_array(object, 5); + VipsImage **t = (VipsImage **) vips_object_local_array(object, 2); #ifdef HAVE_LIBJXL_0_7 JxlEncoderFrameSettings *frame_settings; @@ -282,6 +284,17 @@ vips_foreign_save_jxl_build(VipsObject *object) return -1; in = t[0]; + /* Mimics VIPS_SAVEABLE_RGBA. + * FIXME: add support encoding images with > 4 bands. + */ + if (in->Bands > 4) { + if (vips_extract_band(in, &t[1], 0, + "n", 4, + NULL)) + return -1; + in = t[1]; + } + JxlEncoderInitBasicInfo(&jxl->info); switch (in->BandFmt) { @@ -523,9 +536,7 @@ vips_foreign_save_jxl_class_init(VipsForeignSaveJxlClass *class) foreign_class->suffs = vips__jxl_suffs; - /* This lets throuigh scRGB too, which we then save as jxl float. - */ - save_class->saveable = VIPS_SAVEABLE_RGBA; + save_class->saveable = VIPS_SAVEABLE_ANY; save_class->format_table = bandfmt_jxl; VIPS_ARG_INT(class, "tier", 10,