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 E9D7D4755D for ; Mon, 11 Sep 2023 18:00:36 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C252368C977; Mon, 11 Sep 2023 21:00:33 +0300 (EEST) Received: from mail-ot1-f53.google.com (mail-ot1-f53.google.com [209.85.210.53]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6092168C8AD for ; Mon, 11 Sep 2023 21:00:27 +0300 (EEST) Received: by mail-ot1-f53.google.com with SMTP id 46e09a7af769-6c0822f46a1so3309419a34.0 for ; Mon, 11 Sep 2023 11:00:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1694455225; x=1695060025; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=hf6y2eLHpIEnEzZ/AKHvpPVowQncQPL3i4WEf3qTSj4=; b=mrOL8gezk9wNh9lUwhgy09t1fWEmYL9bZkV+OLPUhkFQzp58QAm3yUk/H2JkeDipSQ p8NYjEZggnj4/JAZFsmoEGhglusoUC4mIzetGK6i7DddbFPDNoQZ6ZXtnLnAuAf/I138 4Etd+cjmQH8ovRWw/1dugXJ54Y8liNloYmLxaq7IsdyMgCv1kfnd+IgP6cwKe3PmvIY3 tmJj0jSeOYwsDq4mW0MEt+eJYdCaLf36uyRdD8N42NzANs2dOfWnGkYpRjaXsk2SEmDg Y6UaXdWtGNJ93y22FmLRDmNoIEQzj37BS6Toa9pXUyS0u+YAPi6oS0T8cidk9CA1KL/m WSjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694455225; x=1695060025; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=hf6y2eLHpIEnEzZ/AKHvpPVowQncQPL3i4WEf3qTSj4=; b=TnfF91x7WBV0gjisQx+huMbKkhGWsnMOKuisgGOAlJLl+j/OkcoUBqlAmP//YT+juB zThcgPH/Xa7DIefvR5gPjZl1rKb9BxZheRpSkaFEfs4y2TDuBcTNnP5YqEYTUETCF/8D VsNTxT+vMEkM4MvdQd6qYONmrTO3txJHsPYLI3A9ns8mZ2y28g8A6zDC+N3Arp7LfS8q IOqZvtcKbgue3BqyW3q3lsdlw3hZnw0TSLtoiO5oVj62r3HvfxkIRtqH095fEwpQgp/l xZQzJp55EN6US9u0PAiCfpsjHIjVMycxQpvNjuqnzrcm74WS6cfEYRUSl/Gtk5+7iKkQ XU9g== X-Gm-Message-State: AOJu0YzgQmEU52Mi9Je1BRpse3/qb9khRA3zg9txRK1s/Y48VPA6xPeD q8bt0b4CuGuDyVqkfIsGmhoy+qYn2Lk= X-Google-Smtp-Source: AGHT+IEU8hyOOb3vyYvz65UdvwAvVxyYQ3jw/LWHITPwH7tT+p41TvRODdmkT771tHTVfh07mjRSrg== X-Received: by 2002:a05:6830:18e7:b0:6b9:b1b1:135 with SMTP id d7-20020a05683018e700b006b9b1b10135mr10664468otf.13.1694455225259; Mon, 11 Sep 2023 11:00:25 -0700 (PDT) Received: from [192.168.0.10] (host197.190-225-105.telecom.net.ar. [190.225.105.197]) by smtp.gmail.com with ESMTPSA id m19-20020a9d7ad3000000b006b58616daa1sm3312819otn.2.2023.09.11.11.00.24 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 11 Sep 2023 11:00:24 -0700 (PDT) Message-ID: Date: Mon, 11 Sep 2023 15:00:33 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 To: ffmpeg-devel@ffmpeg.org References: <20230906174431.45558-1-jamrial@gmail.com> <20230906174431.45558-2-jamrial@gmail.com> Content-Language: en-US From: James Almer In-Reply-To: Subject: Re: [FFmpeg-devel] [PATCH 01/10] avcodec/packet: add side data set struct and helpers 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 9/11/2023 2:41 PM, Andreas Rheinhardt wrote: > James Almer: >> This will be useful in the following commits. >> >> Signed-off-by: James Almer >> --- >> libavcodec/avpacket.c | 99 +++++++++++++++++++++++++++++++++++++++++++ >> libavcodec/packet.h | 74 ++++++++++++++++++++++++++++++++ >> 2 files changed, 173 insertions(+) >> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >> index 5fef65e97a..5b133c5d8a 100644 >> --- a/libavcodec/avpacket.c >> +++ b/libavcodec/avpacket.c >> @@ -645,3 +645,102 @@ int ff_side_data_set_prft(AVPacket *pkt, int64_t timestamp) >> >> return 0; >> } >> + >> +AVPacketSideData *av_packet_side_data_set_get(const AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type) >> +{ >> + for (int i = 0; i < set->nb_sd; i++) >> + if (set->sd[i]->type == type) >> + return set->sd[i]; >> + >> + return NULL; >> +} >> + >> +static AVPacketSideData *add_side_data_to_set(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type, >> + uint8_t *data, size_t size) >> +{ >> + AVPacketSideData *sd, **tmp; >> + >> + for (int i = 0; i < set->nb_sd; i++) { >> + sd = set->sd[i]; >> + if (sd->type != type) >> + continue; >> + >> + av_freep(&sd->data); >> + sd->data = data; >> + sd->size = size; >> + return sd; >> + } >> + >> + if (set->nb_sd + 1U > INT_MAX) >> + return NULL; >> + >> + tmp = av_realloc_array(set->sd, set->nb_sd + 1, sizeof(*tmp)); >> + if (!tmp) >> + return NULL; >> + >> + set->sd = tmp; >> + >> + sd = av_mallocz(sizeof(*sd)); >> + if (!sd) >> + return NULL; >> + >> + sd->type = type; >> + sd->data = data; >> + sd->size = size; >> + >> + set->sd[set->nb_sd++] = sd; >> + >> + return sd; >> +} >> + >> +AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type, >> + uint8_t *data, size_t size, >> + int flags) >> +{ >> + return add_side_data_to_set(set, type, data, size); >> +} >> + >> +AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type, >> + size_t size, int flags) >> +{ >> + AVPacketSideData *sd = NULL; >> + uint8_t *data = av_malloc(size); > > How about padding in case we have new extradata? Ok. Despite side data of new extradata type not being something we'll see in global side data, in the future AVPacket could also start using this struct and helpers, so it's worth including padding now. > >> + >> + if (!data) >> + return NULL; >> + >> + sd = add_side_data_to_set(set, type, data, size); >> + if (!sd) >> + av_freep(&data); >> + >> + return sd; >> +} >> + >> +void av_packet_side_data_set_remove(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type) >> +{ >> + for (int i = set->nb_sd - 1; i >= 0; i--) { >> + AVPacketSideData *sd = set->sd[i]; >> + if (sd->type != type) >> + continue; >> + av_free(set->sd[i]->data); >> + av_free(set->sd[i]); >> + set->sd[i] = set->sd[--set->nb_sd]; >> + break; >> + } >> +} >> + >> +void av_packet_side_data_set_free(AVPacketSideDataSet *set) >> +{ >> + for (int i = 0; i < set->nb_sd; i++) { >> + av_free(set->sd[i]->data); >> + av_free(set->sd[i]); >> + } >> + set->nb_sd = 0; >> + >> + av_freep(&set->sd); >> +} >> diff --git a/libavcodec/packet.h b/libavcodec/packet.h >> index f28e7e7011..87720ab909 100644 >> --- a/libavcodec/packet.h >> +++ b/libavcodec/packet.h >> @@ -318,6 +318,20 @@ typedef struct AVPacketSideData { >> enum AVPacketSideDataType type; >> } AVPacketSideData; >> >> +/** >> + * Structure to hold a set of AVPacketSideData > > This should mention whether it is legal or not to have multiple side > datas of the same type in the same set. Ok. > >> + * >> + * @see av_packet_side_data_set_new >> + * @see av_packet_side_data_set_add >> + * @see av_packet_side_data_set_get >> + * @see av_packet_side_data_set_remove >> + * @see av_packet_side_data_set_free >> + */ > > This should mention that its size is part of the ABI. Ok. > >> +typedef struct AVPacketSideDataSet { >> + AVPacketSideData **sd; > > Allocating the AVPacketSideDatas independently is a big break from how > it is done with AVPackets. Is this really intended? IMO not worth it. I did it to have this struct in line with Jan's frame one (and frame side data in general). This way helpers can also share a common signature. > >> + int nb_sd; > > int? Why not something unsigned given that it will never be negative? Entry count is int pretty much everywhere. Is it worth making it unsigned only here? > >> +} AVPacketSideDataSet; >> + >> /** >> * This structure stores compressed data. It is typically exported by demuxers >> * and then passed as input to decoders, or received as output from encoders and >> @@ -724,6 +738,66 @@ int av_packet_make_writable(AVPacket *pkt); >> */ >> void av_packet_rescale_ts(AVPacket *pkt, AVRational tb_src, AVRational tb_dst); >> >> +/** >> + * Allocate a new side data entry into to a set. >> + * >> + * @param set a set to which the side data should be added >> + * @param type side data type >> + * @param size side data size >> + * @param flags currently unused > > must be zero Will add. > >> + * @return pointer to freshly allocated side data entry on success, or NULL >> + * otherwise. >> + */ >> +AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type, >> + size_t size, int flags); >> + >> +/** >> + * Wrap an existing array as a packet side data into a set. >> + * >> + * @param set a set to which the side data should be added >> + * @param type side data type >> + * @param data a data array. It must be allocated with the av_malloc() family >> + * of functions. The ownership of the data is transferred to the >> + * set on success >> + * @param size size of the data array >> + * @param flags currently unused >> + * @return pointer to freshly allocated side data entry on success, or NULL >> + * otherwise. On failure, the set is unchanged and the data remains >> + * owned by the caller. >> + */ >> +AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type, >> + uint8_t *data, size_t size, > > data should be void*; the days in which side data was just a buffer and > not a structure are long gone. Ok. > Changing AVPacketSideData is a different issue. > >> + int flags); >> + >> +/** >> + * Remove side data of the given type from a set. >> + * >> + * @param set a set from which the side data should be removed >> + * @param type side information type >> + */ >> +void av_packet_side_data_set_remove(AVPacketSideDataSet *set, >> + enum AVPacketSideDataType type); >> + >> +/** >> + * Get side information from set. >> + * >> + * @param set a set from which the side data should be fetched >> + * @param type desired side information type >> + * >> + * @return pointer to side data if present or NULL otherwise >> + */ >> +AVPacketSideData *av_packet_side_data_set_get(const AVPacketSideDataSet *set, > > Is it really good to give the user the right to modify the entries of a > set that they don't own? Will make it const. > >> + enum AVPacketSideDataType type); >> + >> +/** >> + * Convenience function to free all the side data stored in a set. >> + * >> + * @param set the set to free >> + */ >> +void av_packet_side_data_set_free(AVPacketSideDataSet *set); >> + >> /** >> * @} >> */ This aside, do you have an opinion regarding the inclusion of a new field of AVPacketSideDataSet to AVCodecContext, like in the previous iteration of this set, instead of using coded_side_data? I really prefer the former, as currently there's no way for users to allocate or fill coded_side_data other than doing it manually, and that's pretty ugly and unintuitive. Since coded_side_data is barely used right now, deprecating it will be completely painless. Anton suggested dropping the set struct altogether and just use the underlying fields directly in AVCodecParameters, then make the helpers take IN|OUT pointer to pointer parameters to change the array and the element count fields, which IMO is also pretty ugly. _______________________________________________ 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".