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 3622445C69 for ; Thu, 30 Mar 2023 01:41:52 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3DD7268C3AA; Thu, 30 Mar 2023 04:41:49 +0300 (EEST) Received: from mail-oi1-f177.google.com (mail-oi1-f177.google.com [209.85.167.177]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 28CAB68C10D for ; Thu, 30 Mar 2023 04:41:43 +0300 (EEST) Received: by mail-oi1-f177.google.com with SMTP id f17so13101136oiw.10 for ; Wed, 29 Mar 2023 18:41:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1680140501; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=W2CISfjNCiEubylmO1Bfixpl+64WRA66rU/OKSZ3QVE=; b=TlGIk1r9Eh+CT7UOhurmjql/ge0mknvFmPOiIVbp0XTAss29ieEq6UNLk+c2pqbA8B UPjL97h++poKwn7AihXwlfiSuvlRLMPCEKtw3uCwpVKcjG+T90ONjRUr9WATPzSaAPfB O7EU9NSNDhIQ/kaFR+rP8keWdiCDdvbG++6qy+qvxkUw4IhvlvBIJqOF/104MYYLWV2F PH9xBF9K7ef0KUUibAPX994KJmoLGFp9a1OXVAbcFfV1WWGUR5zo7n0B3a+AGILwpFwK 1byCKlIz6ypziJqPlblF925Pkn1qTurmSSGlkIagikorVK9REY7t0oA0IWtEnsEYh/uz zeCg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680140501; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=W2CISfjNCiEubylmO1Bfixpl+64WRA66rU/OKSZ3QVE=; b=qe90EDqIGCbXafjbnDQs0FCzbEcuxioVD6y/VuhIdz3USr5VGqUkVuoHiJdEN24jW1 UOSqUcfz1W0b7a3vHMacGe4s7nwIOm9pDUUhAEqg8UAWjpO6m1C0NP1VmOim2xkpUP2D 7wcQPU5ygioY/u6eSqBVPs6UliE1T8QDRe3qvtZffISWjeQoJk6PXHnZi7I1jSksg5AQ EwJ4UKMKS48hvQ9xBmSW019PpXIOGAAqnrgAWSHKUgoHG264m3foD6032vNKfFctF7HF GW5lkkdbhdec7yBpAN3JNGCA6rPnsMvIzPMl0zprymQojnzVUpXM2S/Ub6jQUDMgcuJH 8ELg== X-Gm-Message-State: AO0yUKXq87O7A7J3fTIsV3DcPc84p6OQVgb/VewMwFqJWi4/C97cwaqp paU2Vluv0T9JytEBSrXF8SKiPCY4atI= X-Google-Smtp-Source: AK7set/5Sf7pNtLRKbaPgj8OLItPU60H4dBcoDa8WYKnEw9uMW0X+m2BXZqafRE6RgE1v7ymFBvWcg== X-Received: by 2002:aca:6708:0:b0:386:94a8:f9af with SMTP id z8-20020aca6708000000b0038694a8f9afmr9655289oix.52.1680140501222; Wed, 29 Mar 2023 18:41:41 -0700 (PDT) Received: from [192.168.0.14] (host197.190-225-105.telecom.net.ar. [190.225.105.197]) by smtp.gmail.com with ESMTPSA id v12-20020a4aad8c000000b0053b909a5229sm7852702oom.4.2023.03.29.18.41.39 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 29 Mar 2023 18:41:40 -0700 (PDT) Message-ID: <2f26f550-ac00-14a9-46d9-dca7153c0b1b@gmail.com> Date: Wed, 29 Mar 2023 22:41:39 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20230324155213.3493-1-jamrial@gmail.com> <20230324155213.3493-6-jamrial@gmail.com> From: James Almer In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH v3 6/7] avformat/matroskaenc: support writing Dynamic HDR10+ packet side data 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 3/29/2023 9:44 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer >> --- >> libavformat/matroskaenc.c | 91 ++++++++++++++++--- >> tests/ref/fate/aac-autobsf-adtstoasc | 4 +- >> tests/ref/fate/matroska-avoid-negative-ts | 4 +- >> tests/ref/fate/matroska-dovi-write-config7 | 4 +- >> tests/ref/fate/matroska-dovi-write-config8 | 4 +- >> tests/ref/fate/matroska-dvbsub-remux | 4 +- >> tests/ref/fate/matroska-encoding-delay | 14 +-- >> tests/ref/fate/matroska-flac-extradata-update | 4 +- >> tests/ref/fate/matroska-h264-remux | 4 +- >> .../fate/matroska-mastering-display-metadata | 4 +- >> tests/ref/fate/matroska-move-cues-to-front | 4 +- >> tests/ref/fate/matroska-mpegts-remux | 4 +- >> tests/ref/fate/matroska-ms-mode | 4 +- >> tests/ref/fate/matroska-ogg-opus-remux | 10 +- >> tests/ref/fate/matroska-opus-remux | 10 +- >> tests/ref/fate/matroska-pgs-remux | 4 +- >> tests/ref/fate/matroska-pgs-remux-durations | 4 +- >> tests/ref/fate/matroska-qt-mode | 4 +- >> tests/ref/fate/matroska-spherical-mono-remux | 4 +- >> tests/ref/fate/matroska-vp8-alpha-remux | 4 +- >> tests/ref/fate/matroska-zero-length-block | 4 +- >> tests/ref/fate/rgb24-mkv | 4 +- >> tests/ref/fate/shortest-sub | 4 +- >> tests/ref/lavf-fate/av1.mkv | 4 +- >> tests/ref/lavf/mka | 4 +- >> tests/ref/lavf/mkv | 4 +- >> tests/ref/lavf/mkv_attachment | 4 +- >> 27 files changed, 141 insertions(+), 76 deletions(-) >> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c >> index dfc3fbbe95..3427e93619 100644 >> --- a/libavformat/matroskaenc.c >> +++ b/libavformat/matroskaenc.c >> @@ -44,6 +44,7 @@ >> #include "libavutil/channel_layout.h" >> #include "libavutil/crc.h" >> #include "libavutil/dict.h" >> +#include "libavutil/hdr_dynamic_metadata.h" >> #include "libavutil/intfloat.h" >> #include "libavutil/intreadwrite.h" >> #include "libavutil/lfg.h" >> @@ -1612,6 +1613,10 @@ static void mkv_write_blockadditionmapping(AVFormatContext *s, MatroskaMuxContex >> // we either write the default value here, or a void element. Either of them will >> // be overwritten when finishing the track. >> put_ebml_uint(mkv->track.bc, MATROSKA_ID_TRACKMAXBLKADDID, 0); >> + // Similarly, reserve space for an eventual HDR10+ ITU T.35 metadata BlockAdditionMapping. >> + put_ebml_void(pb, 3 /* BlockAdditionMapping */ >> + + 4 /* BlockAddIDValue */ >> + + 4 /* BlockAddIDType */); >> } >> >> if (dovi && dovi->dv_profile <= 10) { >> @@ -2618,17 +2623,34 @@ static int webm_reformat_vtt(MatroskaMuxContext *mkv, AVIOContext *pb, >> return 0; >> } >> >> +static void mkv_write_blockadditional(EbmlWriter *writer, const uint8_t *buf, >> + size_t size, enum AVPacketSideDataType type, >> + uint64_t additional_id) >> +{ >> + size_t offset = 0; >> + >> + if (type == AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL) >> + offset = 8; > > The offset should not exist; the caller should provide the correct data > and size. Removed. > >> + >> + ebml_writer_open_master(writer, MATROSKA_ID_BLOCKMORE); >> + ebml_writer_add_uint(writer, MATROSKA_ID_BLOCKADDID, additional_id); >> + ebml_writer_add_bin (writer, MATROSKA_ID_BLOCKADDITIONAL, >> + buf + offset, size - offset); >> + ebml_writer_close_master(writer); >> +} >> + >> static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv, >> AVIOContext *pb, const AVCodecParameters *par, >> mkv_track *track, const AVPacket *pkt, >> int keyframe, int64_t ts, uint64_t duration, >> int force_blockgroup, int64_t relative_packet_pos) >> { >> - uint8_t *side_data; >> + uint8_t *side_data, *buf = NULL; >> size_t side_data_size; >> - uint64_t additional_id; >> + uint64_t additional_id, max_blockaddid = 0; > > max_blockaddid should not exist; instead, you should unconditionally > open MATROSKA_ID_BLOCKADDITIONS and close it with > ebml_writer_close_or_discard_master(). Oh, that's quite nice. Changed. > >> unsigned track_number = track->track_num; >> - EBML_WRITER(9); >> + int ret; >> + EBML_WRITER(13); >> >> mkv->cur_block.track = track; >> mkv->cur_block.pkt = pkt; >> @@ -2670,17 +2692,51 @@ static int mkv_write_block(void *logctx, MatroskaMuxContext *mkv, >> // Only the Codec-specific BlockMore (id == 1) is currently supported. >> (additional_id = AV_RB64(side_data)) == MATROSKA_BLOCK_ADD_ID_TYPE_OPAQUE_DATA) { >> ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKADDITIONS); >> - ebml_writer_open_master(&writer, MATROSKA_ID_BLOCKMORE); >> - /* Until dbc50f8a our demuxer used a wrong default value >> - * of BlockAddID, so we write it unconditionally. */ >> - ebml_writer_add_uint(&writer, MATROSKA_ID_BLOCKADDID, additional_id); >> - ebml_writer_add_bin (&writer, MATROSKA_ID_BLOCKADDITIONAL, >> - side_data + 8, side_data_size - 8); >> - ebml_writer_close_master(&writer); >> - ebml_writer_close_master(&writer); >> - track->max_blockaddid = additional_id; >> + mkv_write_blockadditional(&writer, side_data, side_data_size, >> + AV_PKT_DATA_MATROSKA_BLOCKADDITIONAL, >> + additional_id); >> + max_blockaddid = track->max_blockaddid = FFMAX(track->max_blockaddid, >> + additional_id); >> + } >> + >> + side_data = av_packet_get_side_data(pkt, >> + AV_PKT_DATA_DYNAMIC_HDR10_PLUS, >> + &side_data_size); >> + if (side_data_size) { >> + uint8_t *payload; >> + size_t payload_size, buf_size; >> + int ret = av_dynamic_hdr_plus_to_t35((AVDynamicHDRPlus *)side_data, &payload, >> + &payload_size); > > I have to say, I really hate these allocations here and consider > av_dynamic_hdr_plus_to_t35() to be ill-designed now. What would be better? Passing an existing buffer to it and having it return an error if it's not big enough? I would not oppose you sending a patch to change it. It's quite recent and nothing but this patch uses it yet. But others may disagree. _______________________________________________ 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".