diff mbox series

[FFmpeg-devel,v2] avcodec/pngenc: support writing iCCP chunks

Message ID 20220311101741.104545-1-ffmpeg@haasn.xyz
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/pngenc: support writing iCCP chunks | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 fail Make fate failed
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson fail Make fate failed
andriy/make_armv7_RPi4 success Make finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed

Commit Message

Niklas Haas March 11, 2022, 10:17 a.m. UTC 
From: Niklas Haas <git@haasn.dev>

encode_zbuf is mostly a mirror image of decode_zbuf. Other than that,
the code is pretty straightforward. Special care needs to be taken to
avoid writing more than 79 characters of the profile description (the
maximum supported).

Also add a FATE transcode test to ensure that the ICC profile gets
encoded correctly.
---
Oops. `-c copy` doesn't actually test the PNG writing code. Need to use
`-c png` instead. Fixed in v2.
---
 libavcodec/pngenc.c    | 77 +++++++++++++++++++++++++++++++++++++++++-
 tests/fate/image.mak   |  3 ++
 tests/ref/fate/png-icc |  8 +++++
 3 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100644 tests/ref/fate/png-icc

Comments

Niklas Haas March 11, 2022, 10:21 a.m. UTC | #1 
On Fri, 11 Mar 2022 11:17:42 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> Oops. `-c copy` doesn't actually test the PNG writing code. Need to use
> `-c png` instead. Fixed in v2.

Hmm, actually, even this doesn't work. I can comment out the iCCP
writing code and the iCCP chunk still gets written, somehow. Even though
the file hash is different from the `-c copy` case!

Any idea how to force a re-encode?
Andreas Rheinhardt March 11, 2022, 11:05 a.m. UTC | #2 
Niklas Haas:
> On Fri, 11 Mar 2022 11:17:42 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
>> Oops. `-c copy` doesn't actually test the PNG writing code. Need to use
>> `-c png` instead. Fixed in v2.
> 
> Hmm, actually, even this doesn't work. I can comment out the iCCP
> writing code and the iCCP chunk still gets written, somehow. Even though
> the file hash is different from the `-c copy` case!
> 
> Any idea how to force a re-encode?

What makes you believe that an iCCP chunk gets written? Is it the size
of the framecrc output? The reason for this is that this is the output
of the decoded png frame and not the hash of the demuxed packet or the
output file. The latter is included in the
+7e412f6a9e2c7fcb674336e5c104518d *tests/data/fate/png-icc.image2.
Comparing +49398 tests/data/fate/png-icc.image2 and the relevant line
from V1 shows that there is indeed more output.
You could use -c copy on the encoded file; and you can also use ffprobe
to directly inspect the side data.

- Andreas
Andreas Rheinhardt March 11, 2022, 11:18 a.m. UTC | #3 
Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> encode_zbuf is mostly a mirror image of decode_zbuf. Other than that,
> the code is pretty straightforward. Special care needs to be taken to
> avoid writing more than 79 characters of the profile description (the
> maximum supported).
> 
> Also add a FATE transcode test to ensure that the ICC profile gets
> encoded correctly.
> ---
> Oops. `-c copy` doesn't actually test the PNG writing code. Need to use
> `-c png` instead. Fixed in v2.
> ---
>  libavcodec/pngenc.c    | 77 +++++++++++++++++++++++++++++++++++++++++-
>  tests/fate/image.mak   |  3 ++
>  tests/ref/fate/png-icc |  8 +++++
>  3 files changed, 87 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ref/fate/png-icc
> 
> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
> index 3ebcc1e571..24530bb62f 100644
> --- a/libavcodec/pngenc.c
> +++ b/libavcodec/pngenc.c
> @@ -28,6 +28,7 @@
>  #include "apng.h"
>  
>  #include "libavutil/avassert.h"
> +#include "libavutil/bprint.h"
>  #include "libavutil/crc.h"
>  #include "libavutil/libm.h"
>  #include "libavutil/opt.h"
> @@ -343,6 +344,65 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
>      return 1;
>  }
>  
> +static int encode_zbuf(AVBPrint *bp, const uint8_t *data, size_t size)
> +{
> +    z_stream zstream;
> +    unsigned char *buf;
> +    unsigned buf_size;
> +    int ret;
> +
> +    zstream.zalloc = ff_png_zalloc,
> +    zstream.zfree  = ff_png_zfree,
> +    zstream.opaque = NULL;
> +    if (deflateInit(&zstream, Z_DEFAULT_COMPRESSION) != Z_OK)
> +        return AVERROR_EXTERNAL;
> +    zstream.next_in  = data;
> +    zstream.avail_in = size;
> +
> +    for (;;) {
> +        av_bprint_get_buffer(bp, 2, &buf, &buf_size);
> +        if (buf_size < 2) {
> +            deflateEnd(&zstream);
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        zstream.next_out  = buf;
> +        zstream.avail_out = buf_size - 1;
> +        ret = deflate(&zstream, Z_FINISH);
> +        if (ret != Z_OK && ret != Z_STREAM_END) {
> +            deflateEnd(&zstream);
> +            return AVERROR_EXTERNAL;
> +        }
> +
> +        bp->len += zstream.next_out - buf;
> +        if (ret == Z_STREAM_END) {
> +            deflateEnd(&zstream);
> +            return 0;
> +        }
> +    }
> +}
> +
> +static int png_get_iccp(AVBPrint *bp, const AVFrameSideData *sd, char **buf_out)
> +{
> +    const AVDictionaryEntry *name;
> +    int ret;
> +
> +    av_bprint_init(bp, 0, AV_BPRINT_SIZE_UNLIMITED);
> +
> +    /* profile header */
> +    name = av_dict_get(sd->metadata, "name", NULL, 0);
> +    av_bprintf(bp, "%.79s", (name && name->value[0]) ? name->value : "icc");
> +    av_bprint_chars(bp, 0, 2); /* terminating \0 and compression method */
> +
> +    /* profile data */
> +    if ((ret = encode_zbuf(bp, sd->data, sd->size))) {
> +        av_bprint_finalize(bp, NULL);
> +        return ret;
> +    }
> +
> +    return av_bprint_finalize(bp, buf_out);

1. This is not how should work with an AVBPrint -- you are throwing the
small-string optimization away here.
2. Using an AVBPrint with its dynamic reallocations is probably not good
here at all: It is easy to get a good upper bound via deflateBound()
which allows to omit the reallocations/the loop. (I should probably have
applied
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210317163202.672493-1-andreas.rheinhardt@gmail.com/)

> +}
> +
>  static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
>  {
>      AVFrameSideData *side_data;
> @@ -399,7 +459,22 @@ static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
>      if (png_get_gama(pict->color_trc, s->buf))
>          png_write_chunk(&s->bytestream, MKTAG('g', 'A', 'M', 'A'), s->buf, 4);
>  
> -    /* put the palette if needed */
> +    side_data = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
> +    if (side_data && side_data->size) {
> +        AVBPrint bp;
> +        char *buf;
> +        int ret;
> +
> +        if ((ret = png_get_iccp(&bp, side_data, &buf))) {
> +            av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk: %s\n",
> +                   av_err2str(ret));

3. You should error out in case of error.

> +        } else {
> +            png_write_chunk(&s->bytestream, MKTAG('i', 'C', 'C', 'P'), buf, bp.size);
4. The size of this chunk is not accounted for in the max_packet_size.
(You are lucky that the current estimate for max_packet_size is too
generous (the zstream is not reset/flushed for each row, so it should be
deflatebound(alldata) instead of height*deflatebound(data_from_one_row)).)

> +            av_free(buf);
> +        }
> +    }
> +
> +    /* put the palette if needed, must be after colorspace information */
>      if (s->color_type == PNG_COLOR_TYPE_PALETTE) {
>          int has_alpha, alpha, i;
>          unsigned int v;
> diff --git a/tests/fate/image.mak b/tests/fate/image.mak
> index 573d398915..da4f3709e9 100644
> --- a/tests/fate/image.mak
> +++ b/tests/fate/image.mak
> @@ -385,6 +385,9 @@ FATE_PNG_PROBE += fate-png-side-data
>  fate-png-side-data: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_frames \
>      -i $(TARGET_SAMPLES)/png1/lena-int_rgb24.png
>  
> +FATE_PNG_PROBE += fate-png-icc
> +fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png"
> +
>  FATE_PNG-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG)
>  FATE_PNG_PROBE-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG_PROBE)
>  FATE_IMAGE += $(FATE_PNG-yes)
> diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
> new file mode 100644
> index 0000000000..d3cf55263e
> --- /dev/null
> +++ b/tests/ref/fate/png-icc
> @@ -0,0 +1,8 @@
> +7e412f6a9e2c7fcb674336e5c104518d *tests/data/fate/png-icc.image2
> +49398 tests/data/fate/png-icc.image2
> +#tb 0: 1/25
> +#media_type 0: video
> +#codec_id 0: rawvideo
> +#dimensions 0: 128x128
> +#sar 0: 2835/2835
> +0,          0,          0,        1,    49152, 0xe0013dee
Niklas Haas March 11, 2022, 1:11 p.m. UTC | #4 
On Fri, 11 Mar 2022 12:05:13 +0100 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> Niklas Haas:
> > On Fri, 11 Mar 2022 11:17:42 +0100 Niklas Haas <ffmpeg@haasn.xyz> wrote:
> >> Oops. `-c copy` doesn't actually test the PNG writing code. Need to use
> >> `-c png` instead. Fixed in v2.
> > 
> > Hmm, actually, even this doesn't work. I can comment out the iCCP
> > writing code and the iCCP chunk still gets written, somehow. Even though
> > the file hash is different from the `-c copy` case!
> > 
> > Any idea how to force a re-encode?
> 
> What makes you believe that an iCCP chunk gets written? Is it the size
> of the framecrc output? The reason for this is that this is the output
> of the decoded png frame and not the hash of the demuxed packet or the
> output file. The latter is included in the
> +7e412f6a9e2c7fcb674336e5c104518d *tests/data/fate/png-icc.image2.
> Comparing +49398 tests/data/fate/png-icc.image2 and the relevant line
> from V1 shows that there is indeed more output.
> You could use -c copy on the encoded file; and you can also use ffprobe
> to directly inspect the side data.
> 
> - Andreas

I was running the ffmpeg command (as printed by `make fate-png-icc V=1`)
directly and using `exiftool` to look at the png-icc.image2 file it
wrote.

But it looks as though I accidentally ran the `-c copy` command twice
during testing. With the `-c png`, the iCCP chunk is written by the new
code, as intended.

So never mind this comment. V2 appears to be testing correctly.
Niklas Haas March 11, 2022, 1:37 p.m. UTC | #5 
On Fri, 11 Mar 2022 12:18:13 +0100 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 1. This is not how should work with an AVBPrint -- you are throwing the
> small-string optimization away here.

Noted, though consider: many ICC profiles used in practice are small
enough to fit inside the 1000 byte buffer. Especially the (absurdly
common) case of an embedded sRGB profile.

As an example, exporting a blank image in GIMP to PNG using the default
settings produces a file with a 388-byte deflate compressed iCCP chunk.

But I don't think this is performance critical enough to warrant
skipping the `malloc` call, and it's definitely to allocate once than
re-allocate in a loop.

> 2. Using an AVBPrint with its dynamic reallocations is probably not good
> here at all: It is easy to get a good upper bound via deflateBound()
> which allows to omit the reallocations/the loop. (I should probably have
> applied

This is a good idea. I didn't realize this existed. I'll switch to using
this function.
Niklas Haas March 12, 2022, 11:10 a.m. UTC | #6 
On Fri, 11 Mar 2022 12:18:13 +0100 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 2. Using an AVBPrint with its dynamic reallocations is probably not good
> here at all: It is easy to get a good upper bound via deflateBound()
> which allows to omit the reallocations/the loop. (I should probably have
> applied

So, I rewrote the code to only use a single av_bprint_get_buffer() call,
rather than looping through it. This is semantically identical to doing
an extra malloc(), but also allows the 1K buffer on the stack.

I did a survey of all (compressed) iCCP chunks found in PNG files in my
"collection" (home folder..), and this is what I found:

   Min. 1st Qu.  Median    Mean 3rd Qu.    Max.
    275    2619    2639    3000    2639   14813

Only roughly 11.57% of files are below the "1K" cutoff threshold for
using the small buffer optimization.

In light of this, I don't think the 1K optimization is hugely important.
But, using the AVBPrint does make the code slightly easier to work with
in my eyes.

The cleanest alternative, I think, would be to store the deflateBound
on the ICC profile somewhere in the PNGEncContext, and then use
av_malloc to get a temporary buffer inside `png_get_iccp`. It's of
course possible to somehow write directly to the output packet buffer,
but I don't think avoiding a ~4K malloc/memcpy is worth the hassle and
bug risk of sidestepping png_write_chunk in favor of some custom chunk
writing code.

Thoughts?
diff mbox series

Patch

diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index 3ebcc1e571..24530bb62f 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -28,6 +28,7 @@ 
 #include "apng.h"
 
 #include "libavutil/avassert.h"
+#include "libavutil/bprint.h"
 #include "libavutil/crc.h"
 #include "libavutil/libm.h"
 #include "libavutil/opt.h"
@@ -343,6 +344,65 @@  static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
     return 1;
 }
 
+static int encode_zbuf(AVBPrint *bp, const uint8_t *data, size_t size)
+{
+    z_stream zstream;
+    unsigned char *buf;
+    unsigned buf_size;
+    int ret;
+
+    zstream.zalloc = ff_png_zalloc,
+    zstream.zfree  = ff_png_zfree,
+    zstream.opaque = NULL;
+    if (deflateInit(&zstream, Z_DEFAULT_COMPRESSION) != Z_OK)
+        return AVERROR_EXTERNAL;
+    zstream.next_in  = data;
+    zstream.avail_in = size;
+
+    for (;;) {
+        av_bprint_get_buffer(bp, 2, &buf, &buf_size);
+        if (buf_size < 2) {
+            deflateEnd(&zstream);
+            return AVERROR(ENOMEM);
+        }
+
+        zstream.next_out  = buf;
+        zstream.avail_out = buf_size - 1;
+        ret = deflate(&zstream, Z_FINISH);
+        if (ret != Z_OK && ret != Z_STREAM_END) {
+            deflateEnd(&zstream);
+            return AVERROR_EXTERNAL;
+        }
+
+        bp->len += zstream.next_out - buf;
+        if (ret == Z_STREAM_END) {
+            deflateEnd(&zstream);
+            return 0;
+        }
+    }
+}
+
+static int png_get_iccp(AVBPrint *bp, const AVFrameSideData *sd, char **buf_out)
+{
+    const AVDictionaryEntry *name;
+    int ret;
+
+    av_bprint_init(bp, 0, AV_BPRINT_SIZE_UNLIMITED);
+
+    /* profile header */
+    name = av_dict_get(sd->metadata, "name", NULL, 0);
+    av_bprintf(bp, "%.79s", (name && name->value[0]) ? name->value : "icc");
+    av_bprint_chars(bp, 0, 2); /* terminating \0 and compression method */
+
+    /* profile data */
+    if ((ret = encode_zbuf(bp, sd->data, sd->size))) {
+        av_bprint_finalize(bp, NULL);
+        return ret;
+    }
+
+    return av_bprint_finalize(bp, buf_out);
+}
+
 static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
 {
     AVFrameSideData *side_data;
@@ -399,7 +459,22 @@  static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
     if (png_get_gama(pict->color_trc, s->buf))
         png_write_chunk(&s->bytestream, MKTAG('g', 'A', 'M', 'A'), s->buf, 4);
 
-    /* put the palette if needed */
+    side_data = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
+    if (side_data && side_data->size) {
+        AVBPrint bp;
+        char *buf;
+        int ret;
+
+        if ((ret = png_get_iccp(&bp, side_data, &buf))) {
+            av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk: %s\n",
+                   av_err2str(ret));
+        } else {
+            png_write_chunk(&s->bytestream, MKTAG('i', 'C', 'C', 'P'), buf, bp.size);
+            av_free(buf);
+        }
+    }
+
+    /* put the palette if needed, must be after colorspace information */
     if (s->color_type == PNG_COLOR_TYPE_PALETTE) {
         int has_alpha, alpha, i;
         unsigned int v;
diff --git a/tests/fate/image.mak b/tests/fate/image.mak
index 573d398915..da4f3709e9 100644
--- a/tests/fate/image.mak
+++ b/tests/fate/image.mak
@@ -385,6 +385,9 @@  FATE_PNG_PROBE += fate-png-side-data
 fate-png-side-data: CMD = run ffprobe$(PROGSSUF)$(EXESUF) -show_frames \
     -i $(TARGET_SAMPLES)/png1/lena-int_rgb24.png
 
+FATE_PNG_PROBE += fate-png-icc
+fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png"
+
 FATE_PNG-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG)
 FATE_PNG_PROBE-$(call DEMDEC, IMAGE2, PNG) += $(FATE_PNG_PROBE)
 FATE_IMAGE += $(FATE_PNG-yes)
diff --git a/tests/ref/fate/png-icc b/tests/ref/fate/png-icc
new file mode 100644
index 0000000000..d3cf55263e
--- /dev/null
+++ b/tests/ref/fate/png-icc
@@ -0,0 +1,8 @@ 
+7e412f6a9e2c7fcb674336e5c104518d *tests/data/fate/png-icc.image2
+49398 tests/data/fate/png-icc.image2
+#tb 0: 1/25
+#media_type 0: video
+#codec_id 0: rawvideo
+#dimensions 0: 128x128
+#sar 0: 2835/2835
+0,          0,          0,        1,    49152, 0xe0013dee