[v2,3/5] png: Add support for writing new floating point formats as 16 bpc png.

Submitted by Maarten Lankhorst on Aug. 6, 2018, 2:59 p.m.

Details

Message ID 20180806145931.3774-4-maarten.lankhorst@linux.intel.com
State New
Series "Add support for RGBA128F and RGB96F formats."
Headers show

Commit Message

Maarten Lankhorst Aug. 6, 2018, 2:59 p.m.
_cairo_image_surface_coerce will round down the image to a lower
bpp when using one of the floating point formats, so don't coerce those.
This makes the code actually work for those formats.

Because a float takes more storage than u16, we have to convert float
to u16 before calling png_write_image, because png_write
doesn't give us back the original row data, but an in-place copy.

With these changes we can dump floating point files with the highest
possible accuracy, with floats clamped between 0 and 1.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 src/cairo-png.c | 108 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 97 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/cairo-png.c b/src/cairo-png.c
index b9fc9160a8ab..2e0520ae8fac 100644
--- a/src/cairo-png.c
+++ b/src/cairo-png.c
@@ -105,6 +105,57 @@  unpremultiply_data (png_structp png, png_row_infop row_info, png_bytep data)
     }
 }
 
+static uint16_t f_to_u16(float val)
+{
+    if (val < 0)
+	return 0;
+    else if (val > 1)
+	return 65535;
+    else
+	return (uint16_t)(val * 65535.f);
+}
+
+static void
+unpremultiply_float (float *f, uint16_t *d16, unsigned width)
+{
+    unsigned int i;
+
+    for (i = 0; i < width; i++) {
+	float r, g, b, a;
+
+	r = *f++;
+	g = *f++;
+	b = *f++;
+	a = *f++;
+
+	if (a > 0) {
+	    *d16++ = f_to_u16(r / a);
+	    *d16++ = f_to_u16(g / a);
+	    *d16++ = f_to_u16(b / a);
+	    *d16++ = f_to_u16(a);
+	} else {
+	    *d16++ = 0;
+	    *d16++ = 0;
+	    *d16++ = 0;
+	    *d16++ = 0;
+	}
+    }
+}
+
+
+static void
+convert_float_to_u16 (float *f, uint16_t *d16, unsigned int width)
+{
+    unsigned int i;
+
+    for (i = 0; i < width; i++) {
+	*d16++ = f_to_u16(*f++);
+	*d16++ = f_to_u16(*f++);
+	*d16++ = f_to_u16(*f++);
+	*d16++ = 0;
+    }
+}
+
 /* Converts native endian xRGB => RGBx bytes */
 static void
 convert_data_to_bytes (png_structp png, png_row_infop row_info, png_bytep data)
@@ -182,6 +233,7 @@  write_png (cairo_surface_t	*surface,
     png_color_16 white;
     int png_color_type;
     int bpc;
+    unsigned char *volatile u16_copy = NULL;
 
     status = _cairo_surface_acquire_source_image (surface,
 						  &image,
@@ -198,11 +250,22 @@  write_png (cairo_surface_t	*surface,
 	goto BAIL1;
     }
 
-    /* Handle the various fallback formats (e.g. low bit-depth XServers)
-     * by coercing them to a simpler format using pixman.
-     */
-    clone = _cairo_image_surface_coerce (image);
-    status = clone->base.status;
+    /* Don't coerce to a lower resolution format */
+    if (image->format == CAIRO_FORMAT_RGB96F ||
+        image->format == CAIRO_FORMAT_RGBA128F) {
+	u16_copy = _cairo_malloc_ab (image->width * 8, image->height);
+	if (!u16_copy) {
+	    status = _cairo_error (CAIRO_STATUS_NO_MEMORY);
+	    goto BAIL1;
+	}
+	clone = (cairo_image_surface_t *)cairo_surface_reference (&image->base);
+    } else {
+	  /* Handle the various fallback formats (e.g. low bit-depth XServers)
+	  * by coercing them to a simpler format using pixman.
+	  */
+	  clone = _cairo_image_surface_coerce (image);
+	  status = clone->base.status;
+    }
     if (unlikely (status))
         goto BAIL1;
 
@@ -212,8 +275,22 @@  write_png (cairo_surface_t	*surface,
 	goto BAIL2;
     }
 
-    for (i = 0; i < clone->height; i++)
-	rows[i] = (png_byte *) clone->data + i * clone->stride;
+    if (!u16_copy) {
+	for (i = 0; i < clone->height; i++)
+	    rows[i] = (png_byte *)clone->data + i * clone->stride;
+    } else {
+	for (i = 0; i < clone->height; i++) {
+	    float *float_line = (float *)&clone->data[i * clone->stride];
+	    uint16_t *u16_line = (uint16_t *)&u16_copy[i * clone->width * 8];
+
+	    if (image->format == CAIRO_FORMAT_RGBA128F)
+		unpremultiply_float (float_line, u16_line, clone->width);
+	    else
+		convert_float_to_u16 (float_line, u16_line, clone->width);
+
+	    rows[i] = (png_byte *)u16_line;
+	}
+    }
 
     png = png_create_write_struct (PNG_LIBPNG_VER_STRING, &status,
 	                           png_simple_error_callback,
@@ -263,10 +340,16 @@  write_png (cairo_surface_t	*surface,
 	png_set_packswap (png);
 #endif
 	break;
-    case CAIRO_FORMAT_INVALID:
-    case CAIRO_FORMAT_RGB16_565:
     case CAIRO_FORMAT_RGB96F:
+	bpc = 16;
+	png_color_type = PNG_COLOR_TYPE_RGB;
+	break;
     case CAIRO_FORMAT_RGBA128F:
+	bpc = 16;
+	png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
+	break;
+    case CAIRO_FORMAT_INVALID:
+    case CAIRO_FORMAT_RGB16_565:
     default:
 	status = _cairo_error (CAIRO_STATUS_INVALID_FORMAT);
 	goto BAIL4;
@@ -298,9 +381,11 @@  write_png (cairo_surface_t	*surface,
     png_write_info (png, info);
 
     if (png_color_type == PNG_COLOR_TYPE_RGB_ALPHA) {
-	png_set_write_user_transform_fn (png, unpremultiply_data);
+	if (clone->format != CAIRO_FORMAT_RGBA128F)
+	    png_set_write_user_transform_fn (png, unpremultiply_data);
     } else if (png_color_type == PNG_COLOR_TYPE_RGB) {
-	png_set_write_user_transform_fn (png, convert_data_to_bytes);
+	if (clone->format != CAIRO_FORMAT_RGB96F)
+	    png_set_write_user_transform_fn (png, convert_data_to_bytes);
 	png_set_filler (png, 0, PNG_FILLER_AFTER);
     }
 
@@ -313,6 +398,7 @@  BAIL3:
     free (rows);
 BAIL2:
     cairo_surface_destroy (&clone->base);
+    free (u16_copy);
 BAIL1:
     _cairo_surface_release_source_image (surface, image, image_extra);
 

Comments

Bill Spitzak Aug. 6, 2018, 7:05 p.m.
Recommend the unpremultiply be written like this:

   if (a > 0 && a < 1) {
      ... r/a ...
   } else {
      .... r, g, b, (a>0)?1:0;
   }

The purpose is to avoid the division for a > 1.0, which will happen when
various filtering operations are done but will produce the wrong color. The
above code also leaves the color alone on transparent and treats NaN in the
alpha as 0.0, both of which I have also found useful.

It would really be nice if cairo provided an option to treat png as
premultiplied, but it is the toy interface. Just be warned that a huge
number of them are premultipled (the spec is ignored a lot!).

On Mon, Aug 6, 2018 at 7:59 AM, Maarten Lankhorst <
maarten.lankhorst@linux.intel.com> wrote:

> _cairo_image_surface_coerce will round down the image to a lower
> bpp when using one of the floating point formats, so don't coerce those.
> This makes the code actually work for those formats.
>
> Because a float takes more storage than u16, we have to convert float
> to u16 before calling png_write_image, because png_write
> doesn't give us back the original row data, but an in-place copy.
>
> With these changes we can dump floating point files with the highest
> possible accuracy, with floats clamped between 0 and 1.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  src/cairo-png.c | 108 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 97 insertions(+), 11 deletions(-)
>
> diff --git a/src/cairo-png.c b/src/cairo-png.c
> index b9fc9160a8ab..2e0520ae8fac 100644
> --- a/src/cairo-png.c
> +++ b/src/cairo-png.c
> @@ -105,6 +105,57 @@ unpremultiply_data (png_structp png, png_row_infop
> row_info, png_bytep data)
>      }
>  }
>
> +static uint16_t f_to_u16(float val)
> +{
> +    if (val < 0)
> +       return 0;
> +    else if (val > 1)
> +       return 65535;
> +    else
> +       return (uint16_t)(val * 65535.f);
> +}
> +
> +static void
> +unpremultiply_float (float *f, uint16_t *d16, unsigned width)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < width; i++) {
> +       float r, g, b, a;
> +
> +       r = *f++;
> +       g = *f++;
> +       b = *f++;
> +       a = *f++;
> +
> +       if (a > 0) {
> +           *d16++ = f_to_u16(r / a);
> +           *d16++ = f_to_u16(g / a);
> +           *d16++ = f_to_u16(b / a);
> +           *d16++ = f_to_u16(a);
> +       } else {
> +           *d16++ = 0;
> +           *d16++ = 0;
> +           *d16++ = 0;
> +           *d16++ = 0;
> +       }
> +    }
> +}
> +
> +
> +static void
> +convert_float_to_u16 (float *f, uint16_t *d16, unsigned int width)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < width; i++) {
> +       *d16++ = f_to_u16(*f++);
> +       *d16++ = f_to_u16(*f++);
> +       *d16++ = f_to_u16(*f++);
> +       *d16++ = 0;
> +    }
> +}
> +
>  /* Converts native endian xRGB => RGBx bytes */
>  static void
>  convert_data_to_bytes (png_structp png, png_row_infop row_info, png_bytep
> data)
> @@ -182,6 +233,7 @@ write_png (cairo_surface_t  *surface,
>      png_color_16 white;
>      int png_color_type;
>      int bpc;
> +    unsigned char *volatile u16_copy = NULL;
>
>      status = _cairo_surface_acquire_source_image (surface,
>                                                   &image,
> @@ -198,11 +250,22 @@ write_png (cairo_surface_t        *surface,
>         goto BAIL1;
>      }
>
> -    /* Handle the various fallback formats (e.g. low bit-depth XServers)
> -     * by coercing them to a simpler format using pixman.
> -     */
> -    clone = _cairo_image_surface_coerce (image);
> -    status = clone->base.status;
> +    /* Don't coerce to a lower resolution format */
> +    if (image->format == CAIRO_FORMAT_RGB96F ||
> +        image->format == CAIRO_FORMAT_RGBA128F) {
> +       u16_copy = _cairo_malloc_ab (image->width * 8, image->height);
> +       if (!u16_copy) {
> +           status = _cairo_error (CAIRO_STATUS_NO_MEMORY);
> +           goto BAIL1;
> +       }
> +       clone = (cairo_image_surface_t *)cairo_surface_reference
> (&image->base);
> +    } else {
> +         /* Handle the various fallback formats (e.g. low bit-depth
> XServers)
> +         * by coercing them to a simpler format using pixman.
> +         */
> +         clone = _cairo_image_surface_coerce (image);
> +         status = clone->base.status;
> +    }
>      if (unlikely (status))
>          goto BAIL1;
>
> @@ -212,8 +275,22 @@ write_png (cairo_surface_t *surface,
>         goto BAIL2;
>      }
>
> -    for (i = 0; i < clone->height; i++)
> -       rows[i] = (png_byte *) clone->data + i * clone->stride;
> +    if (!u16_copy) {
> +       for (i = 0; i < clone->height; i++)
> +           rows[i] = (png_byte *)clone->data + i * clone->stride;
> +    } else {
> +       for (i = 0; i < clone->height; i++) {
> +           float *float_line = (float *)&clone->data[i * clone->stride];
> +           uint16_t *u16_line = (uint16_t *)&u16_copy[i * clone->width *
> 8];
> +
> +           if (image->format == CAIRO_FORMAT_RGBA128F)
> +               unpremultiply_float (float_line, u16_line, clone->width);
> +           else
> +               convert_float_to_u16 (float_line, u16_line, clone->width);
> +
> +           rows[i] = (png_byte *)u16_line;
> +       }
> +    }
>
>      png = png_create_write_struct (PNG_LIBPNG_VER_STRING, &status,
>                                    png_simple_error_callback,
> @@ -263,10 +340,16 @@ write_png (cairo_surface_t        *surface,
>         png_set_packswap (png);
>  #endif
>         break;
> -    case CAIRO_FORMAT_INVALID:
> -    case CAIRO_FORMAT_RGB16_565:
>      case CAIRO_FORMAT_RGB96F:
> +       bpc = 16;
> +       png_color_type = PNG_COLOR_TYPE_RGB;
> +       break;
>      case CAIRO_FORMAT_RGBA128F:
> +       bpc = 16;
> +       png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
> +       break;
> +    case CAIRO_FORMAT_INVALID:
> +    case CAIRO_FORMAT_RGB16_565:
>      default:
>         status = _cairo_error (CAIRO_STATUS_INVALID_FORMAT);
>         goto BAIL4;
> @@ -298,9 +381,11 @@ write_png (cairo_surface_t *surface,
>      png_write_info (png, info);
>
>      if (png_color_type == PNG_COLOR_TYPE_RGB_ALPHA) {
> -       png_set_write_user_transform_fn (png, unpremultiply_data);
> +       if (clone->format != CAIRO_FORMAT_RGBA128F)
> +           png_set_write_user_transform_fn (png, unpremultiply_data);
>      } else if (png_color_type == PNG_COLOR_TYPE_RGB) {
> -       png_set_write_user_transform_fn (png, convert_data_to_bytes);
> +       if (clone->format != CAIRO_FORMAT_RGB96F)
> +           png_set_write_user_transform_fn (png, convert_data_to_bytes);
>         png_set_filler (png, 0, PNG_FILLER_AFTER);
>      }
>
> @@ -313,6 +398,7 @@ BAIL3:
>      free (rows);
>  BAIL2:
>      cairo_surface_destroy (&clone->base);
> +    free (u16_copy);
>  BAIL1:
>      _cairo_surface_release_source_image (surface, image, image_extra);
>
> --
> 2.18.0
>
> --
> cairo mailing list
> cairo@cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
Maarten Lankhorst Aug. 7, 2018, 9:04 a.m.
Hey,

Op 06-08-18 om 21:05 schreef Bill Spitzak:
> Recommend the unpremultiply be written like this:
>
>    if (a > 0 && a < 1) {
>       ... r/a ...
>    } else {
>       .... r, g, b, (a>0)?1:0;
>    }
>
> The purpose is to avoid the division for a > 1.0, which will happen when various filtering operations are done but will produce the wrong color. The above code also leaves the color alone on transparent and treats NaN in the alpha as 0.0, both of which I have also found useful.
>
> It would really be nice if cairo provided an option to treat png as premultiplied, but it is the toy interface. Just be warned that a huge number of them are premultipled (the spec is ignored a lot!).
Yeah I guess premultiplied could be a problem, but I think as long as we stick to the spec and are able to read
files we wrote in the same way, we should be fine. Anything else would really be outside the scope of the png support.  :)
Did you find anything else while looking at the patch series?

~Maarten

> On Mon, Aug 6, 2018 at 7:59 AM, Maarten Lankhorst <maarten.lankhorst@linux.intel.com <mailto:maarten.lankhorst@linux.intel.com>> wrote:
>
>     _cairo_image_surface_coerce will round down the image to a lower
>     bpp when using one of the floating point formats, so don't coerce those.
>     This makes the code actually work for those formats.
>
>     Because a float takes more storage than u16, we have to convert float
>     to u16 before calling png_write_image, because png_write
>     doesn't give us back the original row data, but an in-place copy.
>
>     With these changes we can dump floating point files with the highest
>     possible accuracy, with floats clamped between 0 and 1.
>
>     Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com <mailto:maarten.lankhorst@linux.intel.com>>
>     ---
>      src/cairo-png.c | 108 +++++++++++++++++++++++++++++++++++++++++++-----
>      1 file changed, 97 insertions(+), 11 deletions(-)
>
>     diff --git a/src/cairo-png.c b/src/cairo-png.c
>     index b9fc9160a8ab..2e0520ae8fac 100644
>     --- a/src/cairo-png.c
>     +++ b/src/cairo-png.c
>     @@ -105,6 +105,57 @@ unpremultiply_data (png_structp png, png_row_infop row_info, png_bytep data)
>          }
>      }
>
>     +static uint16_t f_to_u16(float val)
>     +{
>     +    if (val < 0)
>     +       return 0;
>     +    else if (val > 1)
>     +       return 65535;
>     +    else
>     +       return (uint16_t)(val * 65535.f);
>     +}
>     +
>     +static void
>     +unpremultiply_float (float *f, uint16_t *d16, unsigned width)
>     +{
>     +    unsigned int i;
>     +
>     +    for (i = 0; i < width; i++) {
>     +       float r, g, b, a;
>     +
>     +       r = *f++;
>     +       g = *f++;
>     +       b = *f++;
>     +       a = *f++;
>     +
>     +       if (a > 0) {
>     +           *d16++ = f_to_u16(r / a);
>     +           *d16++ = f_to_u16(g / a);
>     +           *d16++ = f_to_u16(b / a);
>     +           *d16++ = f_to_u16(a);
>     +       } else {
>     +           *d16++ = 0;
>     +           *d16++ = 0;
>     +           *d16++ = 0;
>     +           *d16++ = 0;
>     +       }
>     +    }
>     +}
>     +
>     +
>     +static void
>     +convert_float_to_u16 (float *f, uint16_t *d16, unsigned int width)
>     +{
>     +    unsigned int i;
>     +
>     +    for (i = 0; i < width; i++) {
>     +       *d16++ = f_to_u16(*f++);
>     +       *d16++ = f_to_u16(*f++);
>     +       *d16++ = f_to_u16(*f++);
>     +       *d16++ = 0;
>     +    }
>     +}
>     +
>      /* Converts native endian xRGB => RGBx bytes */
>      static void
>      convert_data_to_bytes (png_structp png, png_row_infop row_info, png_bytep data)
>     @@ -182,6 +233,7 @@ write_png (cairo_surface_t  *surface,
>          png_color_16 white;
>          int png_color_type;
>          int bpc;
>     +    unsigned char *volatile u16_copy = NULL;
>
>          status = _cairo_surface_acquire_source_image (surface,
>                                                       &image,
>     @@ -198,11 +250,22 @@ write_png (cairo_surface_t        *surface,
>             goto BAIL1;
>          }
>
>     -    /* Handle the various fallback formats (e.g. low bit-depth XServers)
>     -     * by coercing them to a simpler format using pixman.
>     -     */
>     -    clone = _cairo_image_surface_coerce (image);
>     -    status = clone->base.status;
>     +    /* Don't coerce to a lower resolution format */
>     +    if (image->format == CAIRO_FORMAT_RGB96F ||
>     +        image->format == CAIRO_FORMAT_RGBA128F) {
>     +       u16_copy = _cairo_malloc_ab (image->width * 8, image->height);
>     +       if (!u16_copy) {
>     +           status = _cairo_error (CAIRO_STATUS_NO_MEMORY);
>     +           goto BAIL1;
>     +       }
>     +       clone = (cairo_image_surface_t *)cairo_surface_reference (&image->base);
>     +    } else {
>     +         /* Handle the various fallback formats (e.g. low bit-depth XServers)
>     +         * by coercing them to a simpler format using pixman.
>     +         */
>     +         clone = _cairo_image_surface_coerce (image);
>     +         status = clone->base.status;
>     +    }
>          if (unlikely (status))
>              goto BAIL1;
>
>     @@ -212,8 +275,22 @@ write_png (cairo_surface_t *surface,
>             goto BAIL2;
>          }
>
>     -    for (i = 0; i < clone->height; i++)
>     -       rows[i] = (png_byte *) clone->data + i * clone->stride;
>     +    if (!u16_copy) {
>     +       for (i = 0; i < clone->height; i++)
>     +           rows[i] = (png_byte *)clone->data + i * clone->stride;
>     +    } else {
>     +       for (i = 0; i < clone->height; i++) {
>     +           float *float_line = (float *)&clone->data[i * clone->stride];
>     +           uint16_t *u16_line = (uint16_t *)&u16_copy[i * clone->width * 8];
>     +
>     +           if (image->format == CAIRO_FORMAT_RGBA128F)
>     +               unpremultiply_float (float_line, u16_line, clone->width);
>     +           else
>     +               convert_float_to_u16 (float_line, u16_line, clone->width);
>     +
>     +           rows[i] = (png_byte *)u16_line;
>     +       }
>     +    }
>
>          png = png_create_write_struct (PNG_LIBPNG_VER_STRING, &status,
>                                        png_simple_error_callback,
>     @@ -263,10 +340,16 @@ write_png (cairo_surface_t        *surface,
>             png_set_packswap (png);
>      #endif
>             break;
>     -    case CAIRO_FORMAT_INVALID:
>     -    case CAIRO_FORMAT_RGB16_565:
>          case CAIRO_FORMAT_RGB96F:
>     +       bpc = 16;
>     +       png_color_type = PNG_COLOR_TYPE_RGB;
>     +       break;
>          case CAIRO_FORMAT_RGBA128F:
>     +       bpc = 16;
>     +       png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
>     +       break;
>     +    case CAIRO_FORMAT_INVALID:
>     +    case CAIRO_FORMAT_RGB16_565:
>          default:
>             status = _cairo_error (CAIRO_STATUS_INVALID_FORMAT);
>             goto BAIL4;
>     @@ -298,9 +381,11 @@ write_png (cairo_surface_t *surface,
>          png_write_info (png, info);
>
>          if (png_color_type == PNG_COLOR_TYPE_RGB_ALPHA) {
>     -       png_set_write_user_transform_fn (png, unpremultiply_data);
>     +       if (clone->format != CAIRO_FORMAT_RGBA128F)
>     +           png_set_write_user_transform_fn (png, unpremultiply_data);
>          } else if (png_color_type == PNG_COLOR_TYPE_RGB) {
>     -       png_set_write_user_transform_fn (png, convert_data_to_bytes);
>     +       if (clone->format != CAIRO_FORMAT_RGB96F)
>     +           png_set_write_user_transform_fn (png, convert_data_to_bytes);
>             png_set_filler (png, 0, PNG_FILLER_AFTER);
>          }
>
>     @@ -313,6 +398,7 @@ BAIL3:
>          free (rows);
>      BAIL2:
>          cairo_surface_destroy (&clone->base);
>     +    free (u16_copy);
>      BAIL1:
>          _cairo_surface_release_source_image (surface, image, image_extra);
>      
>     -- 
>     2.18.0
>
>     -- 
>     cairo mailing list
>     cairo@cairographics.org <mailto:cairo@cairographics.org>
>     https://lists.cairographics.org/mailman/listinfo/cairo <https://lists.cairographics.org/mailman/listinfo/cairo>
>
>
Bill Spitzak Aug. 7, 2018, 2:56 p.m.
Yes I think we need to leave premultiplied as it is, since the png api is
supposed to be a toy api and there should not be a reason to complicate it.
It would be nice to write some kind of ImageMagic link to Cairo, maybe
something exists already, that would convert many image formats to source
surfaces. This probably should cache images so they can be read multiple
times, and is thus outside the scope of Cairo itself. If so I have found it
best to assume premultipled no matter what the file format says. The reason
is that it is much easier to see accidental lack of multiplication (which
produces bright edges that are very bad the more transparent a pixel is in
a comp) than double multiplication (which produces dark edges that are
somewhat subtle and worst at 50% transparency), so it means bugs are more
noticable and thus fixed.

It was an unfortunate decision by png to say it is not premultiplied, or to
not put an indicator in the header.

Your patches look good though I did not really look at how it is doing the
floating point buffers. Support for half data would be nice but requires a
header from ILM. I fully agree that it is pointless to support image
formats with 16 or more bits as floating point is superior in every single
case.

Not related, but support for floating point transform matrix would be a
good idea too. Cairo also needs to start supporting float in all it's
coordinates, in particular before clipping is done.


On Tue, Aug 7, 2018 at 2:04 AM, Maarten Lankhorst <
maarten.lankhorst@linux.intel.com> wrote:

> Hey,
>
> Op 06-08-18 om 21:05 schreef Bill Spitzak:
> > Recommend the unpremultiply be written like this:
> >
> >    if (a > 0 && a < 1) {
> >       ... r/a ...
> >    } else {
> >       .... r, g, b, (a>0)?1:0;
> >    }
> >
> > The purpose is to avoid the division for a > 1.0, which will happen when
> various filtering operations are done but will produce the wrong color. The
> above code also leaves the color alone on transparent and treats NaN in the
> alpha as 0.0, both of which I have also found useful.
> >
> > It would really be nice if cairo provided an option to treat png as
> premultiplied, but it is the toy interface. Just be warned that a huge
> number of them are premultipled (the spec is ignored a lot!).
> Yeah I guess premultiplied could be a problem, but I think as long as we
> stick to the spec and are able to read
> files we wrote in the same way, we should be fine. Anything else would
> really be outside the scope of the png support.  :)
> Did you find anything else while looking at the patch series?
>
> ~Maarten
>
> > On Mon, Aug 6, 2018 at 7:59 AM, Maarten Lankhorst <
> maarten.lankhorst@linux.intel.com <mailto:maarten.lankhorst@
> linux.intel.com>> wrote:
> >
> >     _cairo_image_surface_coerce will round down the image to a lower
> >     bpp when using one of the floating point formats, so don't coerce
> those.
> >     This makes the code actually work for those formats.
> >
> >     Because a float takes more storage than u16, we have to convert float
> >     to u16 before calling png_write_image, because png_write
> >     doesn't give us back the original row data, but an in-place copy.
> >
> >     With these changes we can dump floating point files with the highest
> >     possible accuracy, with floats clamped between 0 and 1.
> >
> >     Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> <mailto:maarten.lankhorst@linux.intel.com>>
> >     ---
> >      src/cairo-png.c | 108 ++++++++++++++++++++++++++++++
> +++++++++++++-----
> >      1 file changed, 97 insertions(+), 11 deletions(-)
> >
> >     diff --git a/src/cairo-png.c b/src/cairo-png.c
> >     index b9fc9160a8ab..2e0520ae8fac 100644
> >     --- a/src/cairo-png.c
> >     +++ b/src/cairo-png.c
> >     @@ -105,6 +105,57 @@ unpremultiply_data (png_structp png,
> png_row_infop row_info, png_bytep data)
> >          }
> >      }
> >
> >     +static uint16_t f_to_u16(float val)
> >     +{
> >     +    if (val < 0)
> >     +       return 0;
> >     +    else if (val > 1)
> >     +       return 65535;
> >     +    else
> >     +       return (uint16_t)(val * 65535.f);
> >     +}
> >     +
> >     +static void
> >     +unpremultiply_float (float *f, uint16_t *d16, unsigned width)
> >     +{
> >     +    unsigned int i;
> >     +
> >     +    for (i = 0; i < width; i++) {
> >     +       float r, g, b, a;
> >     +
> >     +       r = *f++;
> >     +       g = *f++;
> >     +       b = *f++;
> >     +       a = *f++;
> >     +
> >     +       if (a > 0) {
> >     +           *d16++ = f_to_u16(r / a);
> >     +           *d16++ = f_to_u16(g / a);
> >     +           *d16++ = f_to_u16(b / a);
> >     +           *d16++ = f_to_u16(a);
> >     +       } else {
> >     +           *d16++ = 0;
> >     +           *d16++ = 0;
> >     +           *d16++ = 0;
> >     +           *d16++ = 0;
> >     +       }
> >     +    }
> >     +}
> >     +
> >     +
> >     +static void
> >     +convert_float_to_u16 (float *f, uint16_t *d16, unsigned int width)
> >     +{
> >     +    unsigned int i;
> >     +
> >     +    for (i = 0; i < width; i++) {
> >     +       *d16++ = f_to_u16(*f++);
> >     +       *d16++ = f_to_u16(*f++);
> >     +       *d16++ = f_to_u16(*f++);
> >     +       *d16++ = 0;
> >     +    }
> >     +}
> >     +
> >      /* Converts native endian xRGB => RGBx bytes */
> >      static void
> >      convert_data_to_bytes (png_structp png, png_row_infop row_info,
> png_bytep data)
> >     @@ -182,6 +233,7 @@ write_png (cairo_surface_t  *surface,
> >          png_color_16 white;
> >          int png_color_type;
> >          int bpc;
> >     +    unsigned char *volatile u16_copy = NULL;
> >
> >          status = _cairo_surface_acquire_source_image (surface,
> >                                                       &image,
> >     @@ -198,11 +250,22 @@ write_png (cairo_surface_t        *surface,
> >             goto BAIL1;
> >          }
> >
> >     -    /* Handle the various fallback formats (e.g. low bit-depth
> XServers)
> >     -     * by coercing them to a simpler format using pixman.
> >     -     */
> >     -    clone = _cairo_image_surface_coerce (image);
> >     -    status = clone->base.status;
> >     +    /* Don't coerce to a lower resolution format */
> >     +    if (image->format == CAIRO_FORMAT_RGB96F ||
> >     +        image->format == CAIRO_FORMAT_RGBA128F) {
> >     +       u16_copy = _cairo_malloc_ab (image->width * 8,
> image->height);
> >     +       if (!u16_copy) {
> >     +           status = _cairo_error (CAIRO_STATUS_NO_MEMORY);
> >     +           goto BAIL1;
> >     +       }
> >     +       clone = (cairo_image_surface_t *)cairo_surface_reference
> (&image->base);
> >     +    } else {
> >     +         /* Handle the various fallback formats (e.g. low bit-depth
> XServers)
> >     +         * by coercing them to a simpler format using pixman.
> >     +         */
> >     +         clone = _cairo_image_surface_coerce (image);
> >     +         status = clone->base.status;
> >     +    }
> >          if (unlikely (status))
> >              goto BAIL1;
> >
> >     @@ -212,8 +275,22 @@ write_png (cairo_surface_t *surface,
> >             goto BAIL2;
> >          }
> >
> >     -    for (i = 0; i < clone->height; i++)
> >     -       rows[i] = (png_byte *) clone->data + i * clone->stride;
> >     +    if (!u16_copy) {
> >     +       for (i = 0; i < clone->height; i++)
> >     +           rows[i] = (png_byte *)clone->data + i * clone->stride;
> >     +    } else {
> >     +       for (i = 0; i < clone->height; i++) {
> >     +           float *float_line = (float *)&clone->data[i *
> clone->stride];
> >     +           uint16_t *u16_line = (uint16_t *)&u16_copy[i *
> clone->width * 8];
> >     +
> >     +           if (image->format == CAIRO_FORMAT_RGBA128F)
> >     +               unpremultiply_float (float_line, u16_line,
> clone->width);
> >     +           else
> >     +               convert_float_to_u16 (float_line, u16_line,
> clone->width);
> >     +
> >     +           rows[i] = (png_byte *)u16_line;
> >     +       }
> >     +    }
> >
> >          png = png_create_write_struct (PNG_LIBPNG_VER_STRING, &status,
> >                                        png_simple_error_callback,
> >     @@ -263,10 +340,16 @@ write_png (cairo_surface_t        *surface,
> >             png_set_packswap (png);
> >      #endif
> >             break;
> >     -    case CAIRO_FORMAT_INVALID:
> >     -    case CAIRO_FORMAT_RGB16_565:
> >          case CAIRO_FORMAT_RGB96F:
> >     +       bpc = 16;
> >     +       png_color_type = PNG_COLOR_TYPE_RGB;
> >     +       break;
> >          case CAIRO_FORMAT_RGBA128F:
> >     +       bpc = 16;
> >     +       png_color_type = PNG_COLOR_TYPE_RGB_ALPHA;
> >     +       break;
> >     +    case CAIRO_FORMAT_INVALID:
> >     +    case CAIRO_FORMAT_RGB16_565:
> >          default:
> >             status = _cairo_error (CAIRO_STATUS_INVALID_FORMAT);
> >             goto BAIL4;
> >     @@ -298,9 +381,11 @@ write_png (cairo_surface_t *surface,
> >          png_write_info (png, info);
> >
> >          if (png_color_type == PNG_COLOR_TYPE_RGB_ALPHA) {
> >     -       png_set_write_user_transform_fn (png, unpremultiply_data);
> >     +       if (clone->format != CAIRO_FORMAT_RGBA128F)
> >     +           png_set_write_user_transform_fn (png,
> unpremultiply_data);
> >          } else if (png_color_type == PNG_COLOR_TYPE_RGB) {
> >     -       png_set_write_user_transform_fn (png,
> convert_data_to_bytes);
> >     +       if (clone->format != CAIRO_FORMAT_RGB96F)
> >     +           png_set_write_user_transform_fn (png,
> convert_data_to_bytes);
> >             png_set_filler (png, 0, PNG_FILLER_AFTER);
> >          }
> >
> >     @@ -313,6 +398,7 @@ BAIL3:
> >          free (rows);
> >      BAIL2:
> >          cairo_surface_destroy (&clone->base);
> >     +    free (u16_copy);
> >      BAIL1:
> >          _cairo_surface_release_source_image (surface, image,
> image_extra);
> >
> >     --
> >     2.18.0
> >
> >     --
> >     cairo mailing list
> >     cairo@cairographics.org <mailto:cairo@cairographics.org>
> >     https://lists.cairographics.org/mailman/listinfo/cairo <
> https://lists.cairographics.org/mailman/listinfo/cairo>
> >
> >
>
>