From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 458B24105B for ; Tue, 15 Mar 2022 06:44:22 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 377F368A5BE; Tue, 15 Mar 2022 08:44:20 +0200 (EET) Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-oln040092066082.outbound.protection.outlook.com [40.92.66.82]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C37D2680246 for ; Tue, 15 Mar 2022 08:44:13 +0200 (EET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ci28mQUt7RIjRhKl5oqDxhr42cEJ8DZ6E4HQ6YV/c/UFBPH6wV2Q/dWYjvGAYPgVBrO2zj8ZMZyjiMeR9tXauUek6ml6dKVwDxsI1pOGrQUp+kWdLKKtRetWWrF+Q4fpjwE6X4szzk1bm7e9WKcwdm7JYz4MsE1k8SXMs+w8tD7zGXAoEkJ3foZxn6i+NmOJM7YRIdxabHvBFWu6aj2zcUDK1ZYHCP6tMHgmezid41pJGoIMqjy/frdg/jY8QVh7LYpmaTwHhX6Apfiy8sXwmzTGt10FeYAS0l5u4ybjBE9nXWLe2lvDz7I/PTwOpr5SmViWgUMaiLsNQr7Z5WNnQg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=uRKMKsqhfcJDl3Jz4X0Enk5kmHVyQC42TmOvlzYQN90=; b=GmYE7mXFQVUC9xr5QpMzQnaN/QAiae5lSvMEISXB0rGODTaPhZ5/1uCyImEvlT4eaqJBK7WzHfWSzkh6a7M9D+hVuQUnEJRbUHUkLCEaUfl1E5yd91RsdpfKq+IYJt8onURN0GWMt7FjD/LTGIklSkB7yVz7PeKuwZE9HOuS9pMTLE8sstfWrmgjT+Wf9oaypeHZjZumr4bP/8pJxrOtifGHDmgW5T5FQfa1SrQZKVcHoqsjslT3SQWzkQ8SbgndfFJg+oD0Hw7v5APyqygMLXUVzL593R0xRnM/YElBEZs5HRXMOuGHJ8QmAJLsNkJHMhZ9+a9aGgxot1mUF5VBpw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=uRKMKsqhfcJDl3Jz4X0Enk5kmHVyQC42TmOvlzYQN90=; b=jnoY0mYc1eekr+/7351zHpE/rsdpIHuwhi2+XvKGFXQR8Ss6RY3tQXquGwgvjZ3PwAWFYmb4KCkd6O8qec/gMePjdVQI0p7nBvxSQ37GHu8mM6323TWo6xIe8X3l+w+u5xOpeFsKOsAZMh7j/8LqoQPAPbhLh8vLxgM9EFtNckfDGwN1XW6ie0mlKanaCUXVO7upj9ISZby97R1bf1alEWfcPVwJBMRgdCjdExVohuhiV1ZVo07t7p6QbxlQxj7TZJQ7b/Wgh4E6xQhK8/c7L5b6m6tjWnMV+kw4+tXiC34lUOv+OjhXKE4XJxujy9u+mQLKbliQM7+flJgCn1Xtgg== Received: from AS1PR01MB9564.eurprd01.prod.exchangelabs.com (2603:10a6:20b:4d1::16) by HE1PR0101MB2203.eurprd01.prod.exchangelabs.com (2603:10a6:3:2c::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5061.22; Tue, 15 Mar 2022 06:44:12 +0000 Received: from AS1PR01MB9564.eurprd01.prod.exchangelabs.com ([fe80::9070:a5fd:e532:bdf8]) by AS1PR01MB9564.eurprd01.prod.exchangelabs.com ([fe80::9070:a5fd:e532:bdf8%4]) with mapi id 15.20.5061.028; Tue, 15 Mar 2022 06:44:12 +0000 Message-ID: Date: Tue, 15 Mar 2022 07:44:10 +0100 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220312121047.GB102875@haasn.xyz> <20220312120428.103248-1-ffmpeg@haasn.xyz> From: Andreas Rheinhardt In-Reply-To: <20220312120428.103248-1-ffmpeg@haasn.xyz> X-TMN: [tTLvbuE2pgCx9cRFobNimkYZpcAcBhDG] X-ClientProxiedBy: AM6PR01CA0052.eurprd01.prod.exchangelabs.com (2603:10a6:20b:e0::29) To AS1PR01MB9564.eurprd01.prod.exchangelabs.com (2603:10a6:20b:4d1::16) X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 00842d23-19b1-4e24-d2b9-08da064f36a0 X-MS-TrafficTypeDiagnostic: HE1PR0101MB2203:EE_ X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: jqcd81kHzNeVzhjwX7qi6X4CD36kM4KtCtjja5vag1lxeUgXk7+kJeYcNySSMP3e0CbwLli63bPpbHDENJuTcMrfQwCqtfQQLRJ08iEYicgGH0TyW3zY+x1fwPXI+3rsCHLpwAfY3P+HA/0nz4TEvbNwEFFq1RFsG3UpVTObuTiwicZ0r1urCov+bfDX/3DLSaI0WnFMvCrpkiH83dE+P5LeFkGelLnuXqc1rm61gFAI4ch/Y/5GsEbAYw/HrVKMxzBr0lfUym7ehkKPfe16BWYsLN9N59SsfNGCE8lU8T/1VQzCkBpFEoiBDeyiupNPhrbexm3Sd0aakLDBNDILju6YziBb62G77NSWI7uLqfm8Zc0OxbTLI6fY+J880l8FJABjivXuIk8B6huazQP9hSAeKVAgoWhgiSAGjIOSq9Oqi60PXT/Et+B+x30G6O0fZsF+3XXPU1En/e8l1m73+70YmuM83WgCtY1TttjvIV2XDg8Bz9vry83+rf/lO/3VqmxLi1GxxiMYtcy7NOjWyuU5bWpe/1RClXphD+k8Gd/E9vMSm9vp2M/Vfg02JgLcENceDbLlwZszL3QTVyxxAQ== X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N052U2tOOGdjdkEvY1pkaUNiNFdLd3NsYzR1QjZYL1BVU1o4ZTNWWUsrd1pt?= =?utf-8?B?NVgrbFpOOWk1aGdrRjhqSkFnT05MWld0Rml3NlZwMGZxbEZNMXVMK0hoa1Uy?= =?utf-8?B?TS83MitKMCt4S3NkNEluQzdkWEthaUdndGpLeC9pNzcvREplMVNmRmJzTE1q?= =?utf-8?B?VmhOL1ByNTNBRktuRmczQlI2c1RVWHdaVzYxdC9icDAwT0VLZE5jUlNiYVkv?= =?utf-8?B?NzEvS2lVb3ZyZ25kdjlVT29kODZZWml5ZnlCNk9BZVhOdzFFYWU5QXhNYzNH?= =?utf-8?B?Y1hVSDgreU14QjlsdmhwQ0tpYlBoSFF4MUJvSElFUkZzRGgrL3dpTHpxM0t1?= =?utf-8?B?ZWFsZE9QZzdPaUwzQnNBd2tvRDBqSlZHdnh2YnNpWUUzV0d4SWNhbXBMUGxu?= =?utf-8?B?SFlZSy8zNFVqOWpiWXRHVGFBZ0VneVRwMDM5UEFqK0dKMzF6T29zODNsZ1J6?= =?utf-8?B?aGd2UFZtZ2crZlZaUTVTdFBPOHZsRWR6S25DWSt3eGVaWlBIUlFacFp6dU5r?= =?utf-8?B?b1puOU1leGswUXBwaWNDSGp3a3h3cERwNVlHZkZXT3VRNWxkelZmTlNTUGxL?= =?utf-8?B?bEgyNUtWVGFsdk1VTDN2cDMwR3ZOMDFYeE9TcWlYNS9jbmY1QlRCRC96V0d3?= =?utf-8?B?SGh6b0ZJNHBKOWxZVWhTanpiZnU0VVpVMGNOKzdNbERBRHB4cmo4THU4UTdk?= =?utf-8?B?WXBkVW10SjNxcWhmeGF5dEk4bm05RFFEeDF6U1VJdnFsNlRaZVF2bTNGK1Uv?= =?utf-8?B?aG9zbHhIZmQ1bm9VMUhxMExLUGtJbkJEK3c1eXZzZ0RuelNZRlpIWmRJUHRq?= =?utf-8?B?UGR4NmFyTnFJbUJWSUFLaUNEaDlGS2hHRWdDWjRDOWdkZ1JqazZvdnZZc1d6?= =?utf-8?B?bmNBNHNFb1FtcDRzVnlUcjF6OW9TdEswbDhvODdTeGRrUE5wemgvOTR3WTFX?= =?utf-8?B?clEzNmk0MXJEWTV0RnAwQVZSOURnRnUzdjhqQk1zQWxKRVErNFFxYmJSdmpp?= =?utf-8?B?RWM0WjFTUFFKRGFCYm9ETmkrdnNrVXhDaXJNVXpEc2VoMStGR05KUGZENDNQ?= =?utf-8?B?TDVDNUZDTlA0Ty9CSHk3eVJRRHBFZkJDdTA2UGJCcFhDbU45SnNSbmhHVUNP?= =?utf-8?B?aFFsS3JIUHJBdURScVpJTE1DTW4xU1U3OG1WaTA5UlphUkpMMmg3Q00rV3BU?= =?utf-8?B?bHc2Tk9aSWtsMGkzUUFPSjlLdkNseUZiMHdTWWhCdHdodFNUcEpTc3F0Tkd5?= =?utf-8?B?MjBsQ3d0TmJxcVJRMnhEc0d3RGlISy9mREQ4MUd6RzUwWlBMWk94V01FejNr?= =?utf-8?B?NVZPaFNRS0dkeFlNYkVVREJQb29EdjdodGpLM3RCWHFXS0xGOERGdnZvaW1m?= =?utf-8?B?N3llc3orOVZKd29Ta0dPaEVSTmh4NTVQcmFvVkpMTWdZWkFCU2ZMclBDOEdr?= =?utf-8?B?aHpxOGRhY3Q3NWsxR3BPMnVXVy8zc0s5bm90Qlc0c0pqeCtJdjRtR2NDa0xO?= =?utf-8?B?VDRmamROS1RmNEhDU2JMTE5HRzA4eUVDWVN3bDRuYnYrSjdXVmpHOEJOVTcr?= =?utf-8?Q?b9/VYIljEITRsnjSR4+i0dt/W03bw8/KX3IOTrNatF7VrC?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 00842d23-19b1-4e24-d2b9-08da064f36a0 X-MS-Exchange-CrossTenant-AuthSource: AS1PR01MB9564.eurprd01.prod.exchangelabs.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Mar 2022 06:44:12.2370 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0101MB2203 Subject: Re: [FFmpeg-devel] [PATCH v3] avcodec/pngenc: support writing iCCP chunks X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Niklas Haas: > From: Niklas Haas > > 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".