* [FFmpeg-devel] [PATCH] avcodec/pngenc: support writing iCCP chunks
@ 2022-03-11 10:14 Niklas Haas
2022-03-11 10:17 ` [FFmpeg-devel] [PATCH v2] " Niklas Haas
0 siblings, 1 reply; 12+ messages in thread
From: Niklas Haas @ 2022-03-11 10:14 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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.
---
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);
+}
+
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..e7b156836f 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 copy"
+
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..5bce1ea0c4
--- /dev/null
+++ b/tests/ref/fate/png-icc
@@ -0,0 +1,8 @@
+3cdc46a9c844249cae5c048078d0694d *tests/data/fate/png-icc.image2
+40194 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
--
2.35.1
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 10:14 [FFmpeg-devel] [PATCH] avcodec/pngenc: support writing iCCP chunks Niklas Haas
@ 2022-03-11 10:17 ` Niklas Haas
2022-03-11 10:21 ` Niklas Haas
2022-03-11 11:18 ` Andreas Rheinhardt
0 siblings, 2 replies; 12+ messages in thread
From: Niklas Haas @ 2022-03-11 10:17 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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);
+}
+
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
--
2.35.1
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 10:17 ` [FFmpeg-devel] [PATCH v2] " Niklas Haas
@ 2022-03-11 10:21 ` Niklas Haas
2022-03-11 11:05 ` Andreas Rheinhardt
2022-03-11 11:18 ` Andreas Rheinhardt
1 sibling, 1 reply; 12+ messages in thread
From: Niklas Haas @ 2022-03-11 10:21 UTC (permalink / raw)
To: ffmpeg-devel
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?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 10:21 ` Niklas Haas
@ 2022-03-11 11:05 ` Andreas Rheinhardt
2022-03-11 13:11 ` Niklas Haas Haas
0 siblings, 1 reply; 12+ messages in thread
From: Andreas Rheinhardt @ 2022-03-11 11:05 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 10:17 ` [FFmpeg-devel] [PATCH v2] " Niklas Haas
2022-03-11 10:21 ` Niklas Haas
@ 2022-03-11 11:18 ` Andreas Rheinhardt
2022-03-11 13:37 ` Niklas Haas
2022-03-12 11:10 ` Niklas Haas
1 sibling, 2 replies; 12+ messages in thread
From: Andreas Rheinhardt @ 2022-03-11 11:18 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 11:05 ` Andreas Rheinhardt
@ 2022-03-11 13:11 ` Niklas Haas Haas
0 siblings, 0 replies; 12+ messages in thread
From: Niklas Haas Haas @ 2022-03-11 13:11 UTC (permalink / raw)
To: ffmpeg-devel
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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 11:18 ` Andreas Rheinhardt
@ 2022-03-11 13:37 ` Niklas Haas
2022-03-12 11:10 ` Niklas Haas
1 sibling, 0 replies; 12+ messages in thread
From: Niklas Haas @ 2022-03-11 13:37 UTC (permalink / raw)
To: ffmpeg-devel
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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avcodec/pngenc: support writing iCCP chunks
2022-03-11 11:18 ` Andreas Rheinhardt
2022-03-11 13:37 ` Niklas Haas
@ 2022-03-12 11:10 ` Niklas Haas
2022-03-12 12:04 ` [FFmpeg-devel] [PATCH v3] " Niklas Haas
1 sibling, 1 reply; 12+ messages in thread
From: Niklas Haas @ 2022-03-12 11:10 UTC (permalink / raw)
To: ffmpeg-devel
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?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* [FFmpeg-devel] [PATCH v3] avcodec/pngenc: support writing iCCP chunks
2022-03-12 11:10 ` Niklas Haas
@ 2022-03-12 12:04 ` Niklas Haas
2022-03-15 6:44 ` Andreas Rheinhardt
0 siblings, 1 reply; 12+ messages in thread
From: Niklas Haas @ 2022-03-12 12:04 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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).
To write the (dynamically sized) deflate-encoded data, we allocate extra
space in the packet and use that directly as a scratch buffer. Modify
png_write_chunk slightly to allow pre-writing the chunk contents like
this. This implementation does unfortunately require initializing the
deflate instance twice, but deflateBound() is not redundant with
deflate() so we're not throwing away any significant work.
Also add a FATE transcode test to ensure that the ICC profile gets
encoded correctly.
---
Changes in v3:
- rewrite to write the chunk in-place (inside the packet buffer)
Actually, I implemented an AVBPrint-less version that I think I'm
happier with overall. The extent of the "crimes" needed to support
writing chunks in-place was a single `if` in png_write_chunk and
hard-coding an 8 byte start offset.
I like this the most, since it doesn't require dynamic allocation _at
all_. It also ends up producing a 1 byte smaller test file for some
reason (not as a result of any obvious bug, but simply because zlib
compresses the last few bytes of the stream in a slightly different way,
probably as a result of some internal heuristics related to the buffer
size - the decoded ICC profile checksum is the same).
---
libavcodec/pngenc.c | 93 +++++++++++++++++++++++++++++++++++++++++-
tests/fate/image.mak | 3 ++
tests/ref/fate/png-icc | 8 ++++
3 files changed, 102 insertions(+), 2 deletions(-)
create mode 100644 tests/ref/fate/png-icc
diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index 3ebcc1e571..e9bbe33adf 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -235,7 +235,8 @@ static void png_write_chunk(uint8_t **f, uint32_t tag,
bytestream_put_be32(f, av_bswap32(tag));
if (length > 0) {
crc = av_crc(crc_table, crc, buf, length);
- memcpy(*f, buf, length);
+ if (*f != buf)
+ memcpy(*f, buf, length);
*f += length;
}
bytestream_put_be32(f, ~crc);
@@ -343,10 +344,88 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
return 1;
}
+static size_t zbuf_bound(const uint8_t *data, size_t size)
+{
+ z_stream zstream;
+ size_t bound;
+
+ zstream.zalloc = ff_png_zalloc,
+ zstream.zfree = ff_png_zfree,
+ zstream.opaque = NULL;
+ if (deflateInit(&zstream, Z_DEFAULT_COMPRESSION) != Z_OK)
+ return 0;
+
+ zstream.next_in = data;
+ zstream.avail_in = size;
+ bound = deflateBound(&zstream, size);
+ deflateEnd(&zstream);
+ return bound;
+}
+
+static int encode_zbuf(uint8_t **buf, const uint8_t *buf_end,
+ const uint8_t *data, size_t size)
+{
+ z_stream zstream;
+ 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;
+ zstream.next_out = *buf;
+ zstream.avail_out = buf_end - *buf;
+ ret = deflate(&zstream, Z_FINISH);
+ deflateEnd(&zstream);
+ if (ret != Z_STREAM_END)
+ return AVERROR_EXTERNAL;
+
+ *buf = zstream.next_out;
+ return 0;
+}
+
+static int png_write_iccp(uint8_t **bytestream, const uint8_t *end,
+ const AVFrameSideData *side_data)
+{
+ const AVDictionaryEntry *entry;
+ const char *name;
+ uint8_t *start, *buf;
+ int ret;
+
+ if (!side_data || !side_data->size)
+ return 0;
+
+ /* write the chunk contents first */
+ start = *bytestream + 8; /* make room for iCCP tag + length */
+ buf = start;
+
+ /* profile description */
+ entry = av_dict_get(side_data->metadata, "name", NULL, 0);
+ name = (entry && entry->value[0]) ? entry->value : "icc";
+ for (int i = 0;; i++) {
+ char c = (i == 79) ? 0 : name[i];
+ bytestream_put_byte(&buf, c);
+ if (!c)
+ break;
+ }
+
+ /* compression method and profile data */
+ bytestream_put_byte(&buf, 0);
+ if ((ret = encode_zbuf(&buf, end, side_data->data, side_data->size)))
+ return ret;
+
+ /* rewind to the start and write the chunk header/crc */
+ png_write_chunk(bytestream, MKTAG('i', 'C', 'C', 'P'), start, buf - start);
+ return 0;
+}
+
static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
{
AVFrameSideData *side_data;
PNGEncContext *s = avctx->priv_data;
+ int ret;
/* write png header */
AV_WB32(s->buf, avctx->width);
@@ -399,7 +478,14 @@ 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 ((ret = png_write_iccp(&s->bytestream, s->bytestream_end, side_data))) {
+ av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk: %s\n",
+ av_err2str(ret));
+ return ret;
+ }
+
+ /* 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;
@@ -526,6 +612,7 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
const AVFrame *pict, int *got_packet)
{
PNGEncContext *s = avctx->priv_data;
+ const AVFrameSideData *sd;
int ret;
int enc_row_size;
size_t max_packet_size;
@@ -537,6 +624,8 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
enc_row_size +
12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // IDAT * ceil(enc_row_size / IOBUF_SIZE)
);
+ if ((sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE)))
+ max_packet_size += zbuf_bound(sd->data, sd->size);
if (max_packet_size > INT_MAX)
return AVERROR(ENOMEM);
ret = ff_alloc_packet(avctx, pkt, max_packet_size);
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..fd92a9f949
--- /dev/null
+++ b/tests/ref/fate/png-icc
@@ -0,0 +1,8 @@
+a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
+49397 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
--
2.35.1
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3] avcodec/pngenc: support writing iCCP chunks
2022-03-12 12:04 ` [FFmpeg-devel] [PATCH v3] " Niklas Haas
@ 2022-03-15 6:44 ` Andreas Rheinhardt
2022-03-15 11:10 ` [FFmpeg-devel] [PATCH v4] " Niklas Haas
0 siblings, 1 reply; 12+ messages in thread
From: Andreas Rheinhardt @ 2022-03-15 6:44 UTC (permalink / raw)
To: ffmpeg-devel
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).
>
> To write the (dynamically sized) deflate-encoded data, we allocate extra
> space in the packet and use that directly as a scratch buffer. Modify
> png_write_chunk slightly to allow pre-writing the chunk contents like
> this. This implementation does unfortunately require initializing the
> deflate instance twice, but deflateBound() is not redundant with
> deflate() so we're not throwing away any significant work.
>
> Also add a FATE transcode test to ensure that the ICC profile gets
> encoded correctly.
> ---
>
> Changes in v3:
> - rewrite to write the chunk in-place (inside the packet buffer)
>
> Actually, I implemented an AVBPrint-less version that I think I'm
> happier with overall. The extent of the "crimes" needed to support
> writing chunks in-place was a single `if` in png_write_chunk and
> hard-coding an 8 byte start offset.
>
> I like this the most, since it doesn't require dynamic allocation _at
> all_. It also ends up producing a 1 byte smaller test file for some
> reason (not as a result of any obvious bug, but simply because zlib
> compresses the last few bytes of the stream in a slightly different way,
> probably as a result of some internal heuristics related to the buffer
> size - the decoded ICC profile checksum is the same).
>
> ---
> libavcodec/pngenc.c | 93 +++++++++++++++++++++++++++++++++++++++++-
> tests/fate/image.mak | 3 ++
> tests/ref/fate/png-icc | 8 ++++
> 3 files changed, 102 insertions(+), 2 deletions(-)
> create mode 100644 tests/ref/fate/png-icc
>
> diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
> index 3ebcc1e571..e9bbe33adf 100644
> --- a/libavcodec/pngenc.c
> +++ b/libavcodec/pngenc.c
> @@ -235,7 +235,8 @@ static void png_write_chunk(uint8_t **f, uint32_t tag,
> bytestream_put_be32(f, av_bswap32(tag));
> if (length > 0) {
> crc = av_crc(crc_table, crc, buf, length);
> - memcpy(*f, buf, length);
> + if (*f != buf)
> + memcpy(*f, buf, length);
> *f += length;
> }
> bytestream_put_be32(f, ~crc);
> @@ -343,10 +344,88 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
> return 1;
> }
>
> +static size_t zbuf_bound(const uint8_t *data, size_t size)
> +{
> + z_stream zstream;
> + size_t bound;
> +
> + zstream.zalloc = ff_png_zalloc,
> + zstream.zfree = ff_png_zfree,
Don't surprise people with comma operators.
> + zstream.opaque = NULL;
> + if (deflateInit(&zstream, Z_DEFAULT_COMPRESSION) != Z_OK)
Use the z_stream from the context here and in encode_zbuf() (together
with deflateReset). That saves code, memory and runtime and honours the
user's wishes about compression level. (It will save way more than what
any AVBPrint-small-string-optimization will ever do.)
> + return 0;
> +
> + zstream.next_in = data;
> + zstream.avail_in = size;
> + bound = deflateBound(&zstream, size);
deflateBound uses uLong for the size which is typically a typedef for
unsigned long. In particular, on 64bit Windows size_t is 64bit and uLong
is 32bit. Furthermore, you need to ensure that the chunk length fits
into 32bits (png_write_chunk() even uses an int instead of an uint32_t
for the length). So some length check is necessary here.
(Notice that my zconf.h contains "typedef unsigned long uLong; /* 32
bits or more */", so you may presume uLong to be more encompassing than
uint32_t.)
> + deflateEnd(&zstream);
> + return bound;
> +}
> +
> +static int encode_zbuf(uint8_t **buf, const uint8_t *buf_end,
> + const uint8_t *data, size_t size)
> +{
> + z_stream zstream;
> + 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;
> + zstream.next_out = *buf;
> + zstream.avail_out = buf_end - *buf;
> + ret = deflate(&zstream, Z_FINISH);
> + deflateEnd(&zstream);
> + if (ret != Z_STREAM_END)
> + return AVERROR_EXTERNAL;
> +
> + *buf = zstream.next_out;
> + return 0;
> +}
> +
> +static int png_write_iccp(uint8_t **bytestream, const uint8_t *end,
> + const AVFrameSideData *side_data)
> +{
> + const AVDictionaryEntry *entry;
> + const char *name;
> + uint8_t *start, *buf;
> + int ret;
> +
> + if (!side_data || !side_data->size)
> + return 0;
> +
> + /* write the chunk contents first */
> + start = *bytestream + 8; /* make room for iCCP tag + length */
> + buf = start;
> +
> + /* profile description */
> + entry = av_dict_get(side_data->metadata, "name", NULL, 0);
> + name = (entry && entry->value[0]) ? entry->value : "icc";
> + for (int i = 0;; i++) {
> + char c = (i == 79) ? 0 : name[i];
> + bytestream_put_byte(&buf, c);
> + if (!c)
> + break;
> + }
> +
> + /* compression method and profile data */
> + bytestream_put_byte(&buf, 0);
> + if ((ret = encode_zbuf(&buf, end, side_data->data, side_data->size)))
> + return ret;
> +
> + /* rewind to the start and write the chunk header/crc */
> + png_write_chunk(bytestream, MKTAG('i', 'C', 'C', 'P'), start, buf - start);
> + return 0;
> +}
> +
> static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
> {
> AVFrameSideData *side_data;
> PNGEncContext *s = avctx->priv_data;
> + int ret;
>
> /* write png header */
> AV_WB32(s->buf, avctx->width);
> @@ -399,7 +478,14 @@ 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 ((ret = png_write_iccp(&s->bytestream, s->bytestream_end, side_data))) {
> + av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk: %s\n",
> + av_err2str(ret));
The user already gets the error code (which is always AVERROR_EXTERNAL,
so not really useful); no need to repeat it.
> + return ret;
> + }
> +
> + /* 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;
> @@ -526,6 +612,7 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
> const AVFrame *pict, int *got_packet)
> {
> PNGEncContext *s = avctx->priv_data;
> + const AVFrameSideData *sd;
> int ret;
> int enc_row_size;
> size_t max_packet_size;
> @@ -537,6 +624,8 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
> enc_row_size +
> 12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // IDAT * ceil(enc_row_size / IOBUF_SIZE)
> );
> + if ((sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE)))
> + max_packet_size += zbuf_bound(sd->data, sd->size);
You only account for this when encoding png, yet not for apng;
encode_headers() is also called for them and AV_INPUT_BUFFER_MIN_SIZE
might not suffice; anyway, we should try to avoid using that define --
it is a remnant of an era when users could (had to?) provide buffers in
the hope that they are big enough.
> if (max_packet_size > INT_MAX)
> return AVERROR(ENOMEM);
> ret = ff_alloc_packet(avctx, pkt, max_packet_size);
> 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_PROBE exists for those tests which only need ffprobe, not
ffmpeg. A transcode test always need ffmpeg.
And as I already said: You should use ffprobe to read the side data of
the frames that emanate from decoding the encoded file.
> 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..fd92a9f949
> --- /dev/null
> +++ b/tests/ref/fate/png-icc
> @@ -0,0 +1,8 @@
> +a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
> +49397 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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* [FFmpeg-devel] [PATCH v4] avcodec/pngenc: support writing iCCP chunks
2022-03-15 6:44 ` Andreas Rheinhardt
@ 2022-03-15 11:10 ` Niklas Haas
2022-03-15 11:34 ` [FFmpeg-devel] [PATCH v5] " Niklas Haas
0 siblings, 1 reply; 12+ messages in thread
From: Niklas Haas @ 2022-03-15 11:10 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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).
To write the (dynamically sized) deflate-encoded data, we allocate extra
space in the packet and use that directly as a scratch buffer. Modify
png_write_chunk slightly to allow pre-writing the chunk contents like
this.
Also add a FATE transcode test to ensure that the ICC profile gets
encoded correctly.
---
Changes in v4:
- Remove stray commas on line endings
- Re-use s->zstream instead of creating new deflate instances
- Add size checking and explicit header padding
- Delete the now redundant and overly specialized single-use helpers
- Don't redundantly print error code, just print cause
- Correctly account for max_packet_size in encode_apng as well
- Fix FATE_PNG_PROBE -> FATE_PNG and add -show_frames to test
---
libavcodec/pngenc.c | 86 +++++++++++++++++++++++++++++++++++++++++-
tests/fate/image.mak | 3 ++
tests/ref/fate/png-icc | 43 +++++++++++++++++++++
3 files changed, 130 insertions(+), 2 deletions(-)
create mode 100644 tests/ref/fate/png-icc
diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index 3ebcc1e571..b0091f2b40 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -235,7 +235,8 @@ static void png_write_chunk(uint8_t **f, uint32_t tag,
bytestream_put_be32(f, av_bswap32(tag));
if (length > 0) {
crc = av_crc(crc_table, crc, buf, length);
- memcpy(*f, buf, length);
+ if (*f != buf)
+ memcpy(*f, buf, length);
*f += length;
}
bytestream_put_be32(f, ~crc);
@@ -343,10 +344,53 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
return 1;
}
+static int png_write_iccp(AVCodecContext *avctx, const AVFrameSideData *sd)
+{
+ PNGEncContext *s = avctx->priv_data;
+ const AVDictionaryEntry *entry;
+ const char *name;
+ uint8_t *start, *buf;
+ int ret;
+
+ if (!sd || !sd->size)
+ return 0;
+ s->zstream.next_in = sd->data;
+ s->zstream.avail_in = sd->size;
+
+ /* write the chunk contents first */
+ start = s->bytestream + 8; /* make room for iCCP tag + length */
+ buf = start;
+
+ /* profile description */
+ entry = av_dict_get(sd->metadata, "name", NULL, 0);
+ name = (entry && entry->value[0]) ? entry->value : "icc";
+ for (int i = 0;; i++) {
+ char c = (i == 79) ? 0 : name[i];
+ bytestream_put_byte(&buf, c);
+ if (!c)
+ break;
+ }
+
+ /* compression method and profile data */
+ bytestream_put_byte(&buf, 0);
+ s->zstream.next_out = buf;
+ s->zstream.avail_out = s->bytestream_end - buf;
+ ret = deflate(&s->zstream, Z_FINISH);
+ deflateReset(&s->zstream);
+ if (ret != Z_STREAM_END)
+ return AVERROR_EXTERNAL;
+
+ /* rewind to the start and write the chunk header/crc */
+ png_write_chunk(&s->bytestream, MKTAG('i', 'C', 'C', 'P'), start,
+ s->zstream.next_out - start);
+ return 0;
+}
+
static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
{
AVFrameSideData *side_data;
PNGEncContext *s = avctx->priv_data;
+ int ret;
/* write png header */
AV_WB32(s->buf, avctx->width);
@@ -399,7 +443,13 @@ 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 ((ret = png_write_iccp(avctx, side_data))) {
+ av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk\n");
+ return ret;
+ }
+
+ /* 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;
@@ -522,6 +572,34 @@ the_end:
return ret;
}
+static int add_icc_profile_size(AVCodecContext *avctx, const AVFrame *pict,
+ size_t *max_packet_size)
+{
+ PNGEncContext *s = avctx->priv_data;
+ const AVFrameSideData *sd;
+ uLong bound;
+
+ sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
+ if (!sd || !sd->size)
+ return 0;
+ if (sd->size > UINT_MAX)
+ goto too_large;
+
+ s->zstream.next_in = sd->data;
+ s->zstream.avail_in = sd->size;
+ bound = deflateBound(&s->zstream, sd->size) + 128; /* header */
+ deflateReset(&s->zstream);
+ if (bound > INT_MAX)
+ goto too_large;
+
+ *max_packet_size += bound;
+ return 0;
+
+too_large:
+ av_log(avctx, AV_LOG_WARNING, "ICC profile too large for iCCP chunk\n");
+ return AVERROR_INVALIDDATA;
+}
+
static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
const AVFrame *pict, int *got_packet)
{
@@ -537,6 +615,8 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
enc_row_size +
12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // IDAT * ceil(enc_row_size / IOBUF_SIZE)
);
+ if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
+ return ret;
if (max_packet_size > INT_MAX)
return AVERROR(ENOMEM);
ret = ff_alloc_packet(avctx, pkt, max_packet_size);
@@ -867,6 +947,8 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt,
enc_row_size +
(4 + 12) * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // fdAT * ceil(enc_row_size / IOBUF_SIZE)
);
+ if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
+ return ret;
if (max_packet_size > INT_MAX)
return AVERROR(ENOMEM);
diff --git a/tests/fate/image.mak b/tests/fate/image.mak
index 573d398915..f5fd64e3ee 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 += fate-png-icc
+fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png" "" "" "-show_frames"
+
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..542bb76f9a
--- /dev/null
+++ b/tests/ref/fate/png-icc
@@ -0,0 +1,43 @@
+a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
+49397 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
+[FRAME]
+media_type=video
+stream_index=0
+key_frame=1
+pts=0
+pts_time=0.000000
+pkt_dts=0
+pkt_dts_time=0.000000
+best_effort_timestamp=0
+best_effort_timestamp_time=0.000000
+pkt_duration=1
+pkt_duration_time=0.040000
+pkt_pos=0
+pkt_size=49397
+width=128
+height=128
+pix_fmt=rgb24
+sample_aspect_ratio=1:1
+pict_type=I
+coded_picture_number=0
+display_picture_number=0
+interlaced_frame=0
+top_field_first=0
+repeat_pict=0
+color_range=pc
+color_space=unknown
+color_primaries=unknown
+color_transfer=unknown
+chroma_location=unspecified
+[SIDE_DATA]
+side_data_type=ICC profile
+name=Photoshop ICC profile
+size=3144
+[/SIDE_DATA]
+[/FRAME]
--
2.35.1
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
* [FFmpeg-devel] [PATCH v5] avcodec/pngenc: support writing iCCP chunks
2022-03-15 11:10 ` [FFmpeg-devel] [PATCH v4] " Niklas Haas
@ 2022-03-15 11:34 ` Niklas Haas
0 siblings, 0 replies; 12+ messages in thread
From: Niklas Haas @ 2022-03-15 11:34 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Niklas Haas
From: Niklas Haas <git@haasn.dev>
We re-use the PNGEncContext.zstream for deflate-related operations.
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).
To write the (dynamically sized) deflate-encoded data, we allocate extra
space in the packet and use that directly as a scratch buffer. Modify
png_write_chunk slightly to allow pre-writing the chunk contents like
this.
Also add a FATE transcode test to ensure that the ICC profile gets
encoded correctly.
Signed-off-by: Niklas Haas <git@haasn.dev>
---
Changes in v5:
- Fix FATE failure (possible segfault in encode_apng)
- Fix commit message (still talked about deleted encode_zbuf)
- Add Signed-off-by line
---
libavcodec/pngenc.c | 88 +++++++++++++++++++++++++++++++++++++++++-
tests/fate/image.mak | 3 ++
tests/ref/fate/png-icc | 43 +++++++++++++++++++++
3 files changed, 132 insertions(+), 2 deletions(-)
create mode 100644 tests/ref/fate/png-icc
diff --git a/libavcodec/pngenc.c b/libavcodec/pngenc.c
index 3ebcc1e571..947138e3b5 100644
--- a/libavcodec/pngenc.c
+++ b/libavcodec/pngenc.c
@@ -235,7 +235,8 @@ static void png_write_chunk(uint8_t **f, uint32_t tag,
bytestream_put_be32(f, av_bswap32(tag));
if (length > 0) {
crc = av_crc(crc_table, crc, buf, length);
- memcpy(*f, buf, length);
+ if (*f != buf)
+ memcpy(*f, buf, length);
*f += length;
}
bytestream_put_be32(f, ~crc);
@@ -343,10 +344,53 @@ static int png_get_gama(enum AVColorTransferCharacteristic trc, uint8_t *buf)
return 1;
}
+static int png_write_iccp(AVCodecContext *avctx, const AVFrameSideData *sd)
+{
+ PNGEncContext *s = avctx->priv_data;
+ const AVDictionaryEntry *entry;
+ const char *name;
+ uint8_t *start, *buf;
+ int ret;
+
+ if (!sd || !sd->size)
+ return 0;
+ s->zstream.next_in = sd->data;
+ s->zstream.avail_in = sd->size;
+
+ /* write the chunk contents first */
+ start = s->bytestream + 8; /* make room for iCCP tag + length */
+ buf = start;
+
+ /* profile description */
+ entry = av_dict_get(sd->metadata, "name", NULL, 0);
+ name = (entry && entry->value[0]) ? entry->value : "icc";
+ for (int i = 0;; i++) {
+ char c = (i == 79) ? 0 : name[i];
+ bytestream_put_byte(&buf, c);
+ if (!c)
+ break;
+ }
+
+ /* compression method and profile data */
+ bytestream_put_byte(&buf, 0);
+ s->zstream.next_out = buf;
+ s->zstream.avail_out = s->bytestream_end - buf;
+ ret = deflate(&s->zstream, Z_FINISH);
+ deflateReset(&s->zstream);
+ if (ret != Z_STREAM_END)
+ return AVERROR_EXTERNAL;
+
+ /* rewind to the start and write the chunk header/crc */
+ png_write_chunk(&s->bytestream, MKTAG('i', 'C', 'C', 'P'), start,
+ s->zstream.next_out - start);
+ return 0;
+}
+
static int encode_headers(AVCodecContext *avctx, const AVFrame *pict)
{
AVFrameSideData *side_data;
PNGEncContext *s = avctx->priv_data;
+ int ret;
/* write png header */
AV_WB32(s->buf, avctx->width);
@@ -399,7 +443,13 @@ 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 ((ret = png_write_iccp(avctx, side_data))) {
+ av_log(avctx, AV_LOG_WARNING, "Failed writing iCCP chunk\n");
+ return ret;
+ }
+
+ /* 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;
@@ -522,6 +572,36 @@ the_end:
return ret;
}
+static int add_icc_profile_size(AVCodecContext *avctx, const AVFrame *pict,
+ size_t *max_packet_size)
+{
+ PNGEncContext *s = avctx->priv_data;
+ const AVFrameSideData *sd;
+ uLong bound;
+
+ if (!pict)
+ return 0;
+ sd = av_frame_get_side_data(pict, AV_FRAME_DATA_ICC_PROFILE);
+ if (!sd || !sd->size)
+ return 0;
+ if (sd->size > UINT_MAX)
+ goto too_large;
+
+ s->zstream.next_in = sd->data;
+ s->zstream.avail_in = sd->size;
+ bound = deflateBound(&s->zstream, sd->size) + 128; /* header */
+ deflateReset(&s->zstream);
+ if (bound > INT_MAX)
+ goto too_large;
+
+ *max_packet_size += bound;
+ return 0;
+
+too_large:
+ av_log(avctx, AV_LOG_WARNING, "ICC profile too large for iCCP chunk\n");
+ return AVERROR_INVALIDDATA;
+}
+
static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
const AVFrame *pict, int *got_packet)
{
@@ -537,6 +617,8 @@ static int encode_png(AVCodecContext *avctx, AVPacket *pkt,
enc_row_size +
12 * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // IDAT * ceil(enc_row_size / IOBUF_SIZE)
);
+ if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
+ return ret;
if (max_packet_size > INT_MAX)
return AVERROR(ENOMEM);
ret = ff_alloc_packet(avctx, pkt, max_packet_size);
@@ -867,6 +949,8 @@ static int encode_apng(AVCodecContext *avctx, AVPacket *pkt,
enc_row_size +
(4 + 12) * (((int64_t)enc_row_size + IOBUF_SIZE - 1) / IOBUF_SIZE) // fdAT * ceil(enc_row_size / IOBUF_SIZE)
);
+ if ((ret = add_icc_profile_size(avctx, pict, &max_packet_size)))
+ return ret;
if (max_packet_size > INT_MAX)
return AVERROR(ENOMEM);
diff --git a/tests/fate/image.mak b/tests/fate/image.mak
index 573d398915..f5fd64e3ee 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 += fate-png-icc
+fate-png-icc: CMD = transcode png_pipe $(TARGET_SAMPLES)/png1/lena-int_rgb24.png image2 "-c png" "" "" "-show_frames"
+
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..542bb76f9a
--- /dev/null
+++ b/tests/ref/fate/png-icc
@@ -0,0 +1,43 @@
+a50d37a0e72bddea2fcbba6fb773e2a0 *tests/data/fate/png-icc.image2
+49397 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
+[FRAME]
+media_type=video
+stream_index=0
+key_frame=1
+pts=0
+pts_time=0.000000
+pkt_dts=0
+pkt_dts_time=0.000000
+best_effort_timestamp=0
+best_effort_timestamp_time=0.000000
+pkt_duration=1
+pkt_duration_time=0.040000
+pkt_pos=0
+pkt_size=49397
+width=128
+height=128
+pix_fmt=rgb24
+sample_aspect_ratio=1:1
+pict_type=I
+coded_picture_number=0
+display_picture_number=0
+interlaced_frame=0
+top_field_first=0
+repeat_pict=0
+color_range=pc
+color_space=unknown
+color_primaries=unknown
+color_transfer=unknown
+chroma_location=unspecified
+[SIDE_DATA]
+side_data_type=ICC profile
+name=Photoshop ICC profile
+size=3144
+[/SIDE_DATA]
+[/FRAME]
--
2.35.1
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-03-15 11:36 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-11 10:14 [FFmpeg-devel] [PATCH] avcodec/pngenc: support writing iCCP chunks Niklas Haas
2022-03-11 10:17 ` [FFmpeg-devel] [PATCH v2] " Niklas Haas
2022-03-11 10:21 ` Niklas Haas
2022-03-11 11:05 ` Andreas Rheinhardt
2022-03-11 13:11 ` Niklas Haas Haas
2022-03-11 11:18 ` Andreas Rheinhardt
2022-03-11 13:37 ` Niklas Haas
2022-03-12 11:10 ` Niklas Haas
2022-03-12 12:04 ` [FFmpeg-devel] [PATCH v3] " Niklas Haas
2022-03-15 6:44 ` Andreas Rheinhardt
2022-03-15 11:10 ` [FFmpeg-devel] [PATCH v4] " Niklas Haas
2022-03-15 11:34 ` [FFmpeg-devel] [PATCH v5] " Niklas Haas
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git