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 3E2584707D for ; Mon, 25 Sep 2023 20:02:41 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id AC6CF68C9A2; Mon, 25 Sep 2023 23:02:39 +0300 (EEST) Received: from mail-ua1-f54.google.com (mail-ua1-f54.google.com [209.85.222.54]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id C1B4B68C622 for ; Mon, 25 Sep 2023 23:02:33 +0300 (EEST) Received: by mail-ua1-f54.google.com with SMTP id a1e0cc1a2514c-7ab9f1efecfso1531845241.3 for ; Mon, 25 Sep 2023 13:02:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695672152; x=1696276952; darn=ffmpeg.org; h=to:subject:message-id:date:from:references:in-reply-to:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=YiJr7JxIzk9m2dbriPAsEgSpCujb1zwkOy58MGA1Z4k=; b=hAW9YVWDTDcFLUxNkm/IipK36l8penXCgGBAVJuyJVYgqXkQPTiwF5dcgbrC8dMd0c xo6PIPehetbwIJZ4DVziaKnns93RVoL3r5keN2V9NdLwyItIVDZXTolNNTWTMPetLmdT Ohm+R4YlVVzi7xkvOrphhZJv02wqEERGbjnOxukaZ9xSssmUsoE/my3kEkyOrsGUyIrv AhZO/+f8noDQLdbkM0rkEoZ43R5CRPLOQJu32oOV8IkZvyB3xHnoND1mLDuOOpuV2MwB eTzlg5Tf3tG4qvztKlFL6sR5XjipBsuWkELt6fRlMn06C6VmydOgXeIvkCw9aO1k7yLI Ag1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695672152; x=1696276952; h=to:subject:message-id:date:from:references:in-reply-to:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=YiJr7JxIzk9m2dbriPAsEgSpCujb1zwkOy58MGA1Z4k=; b=NhJRzD78WJB43LZTnuNJhV0K5lkRIbslBvrgcHG7XyrDrJX32jcbUTjgxHIYoPSWZ5 EMKBzAKznLKmzaKpskYIlpopILwnEukVXQ+l5y2Lnfg4SDGu1hZycd1bHQPbabgKBwSj WKibvRclo3wVhv48gOPiFqguJTBpr0MNhKLsF5+O5Tx5fo2NL6a1onYJL9dKpJxy1ewY 7yg6bHQn8fRS9iPm7DrluGMeAKBZyocpacHTHK3l9QAY8HJHN0DWrnTG4pLHCkeqwLn2 I/gmXVzr85nWUb///33n0dpYKVC/Hckjnl7+EefNrgHLteoecspFA9uhLoy8Fqs7sCe5 tQVQ== X-Gm-Message-State: AOJu0YzxNMJdVOeHpY8XGdYPOiU4h4Fmx+eTPIsmBH+UWJnID0Ys70f6 HAvo1TBAqXMZsujuHy2Xmar/Z041Yco7SWv2Rzv3u7i9ZdQ= X-Google-Smtp-Source: AGHT+IEwskih7uWIuZezl+RtZl2BlIC9ZzamAQGz/Heg+rQ0P53NaP7K9FhUyjey+tsRmr+OS2vzOxCKGZX76EKaKSg= X-Received: by 2002:a1f:4a41:0:b0:495:ec90:997e with SMTP id x62-20020a1f4a41000000b00495ec90997emr4523490vka.7.1695672152101; Mon, 25 Sep 2023 13:02:32 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a59:bddb:0:b0:403:6aab:43d5 with HTTP; Mon, 25 Sep 2023 13:02:31 -0700 (PDT) In-Reply-To: <04fc70a5-b697-aa62-b05e-e77507052c66@gmail.com> References: <20230906174431.45558-1-jamrial@gmail.com> <169470621879.20400.7203675987362320361@lain.khirnov.net> <04fc70a5-b697-aa62-b05e-e77507052c66@gmail.com> From: Paul B Mahol Date: Mon, 25 Sep 2023 22:02:31 +0200 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH 00/10 v4][RFC] AVCodecContext and AVCodecParameters 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-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: On 9/25/23, James Almer wrote: > On 9/14/2023 1:40 PM, James Almer wrote: >> On 9/14/2023 12:43 PM, Anton Khirnov wrote: >>> Quoting James Almer (2023-09-06 19:44:20) >>>> Changes since the previous version: >>>> - Removed the AVPacketSideDataSet from AVCodecContext, using instead the >>>> existing coded_side_data field, at Anton's request. >>>> >>>> I still prefer using a new field of type AVPacketSideDataSet, given >>>> that the >>>> user can currently only fill coded_side_data manually (See the >>>> implementation >>>> in the codecpar copy functions, which non lavf users would need to >>>> replicate), >>>> and more so considering coded_side_data is a field pretty much >>>> nothing but lavf >>>> uses. >>>> >>>> Opinions are very welcome and needed. >>> >>> To provide more context on the issue: >>> >>> AVPackets can have side data attached to them, in the form of >>> AVPacket.{side_data,side_data_elems} array+count. >>> >>> Some of this side data can occasionally be stored in global headers, >>> e.g. in containers or extradata. We have some limited support for this: >>> * AVStream.{side_data,nb_side_data} array+count allows demuxers to >>> export this to user, and muxers to accept it from the user >>> * AVCodecContext.{coded_side_data,nb_coded_side_data} allows decoders to >>> accept it from the user (in principle, in practice it is not used), >>> and encoders to export it to the user (this is used, but not very >>> much) >>> >>> The broad idea for this set is adding stream-global "coded/packet" side >>> data to AVCodecParameters, so that it can be naturally communicated from >>> demuxers to bitstream filters to decoders, and from encoders to >>> bitstream filters to muxers. Since AVStream already contains an >>> AVCodecParameters instance, the abovementioned AVStream.side_data >>> becomes redundant and is deprecated, the muxer/demuxer stream-global >>> side data would be passed through AVStream.codecpar. >>> >>> The original version proposed by James adds a new struct, that bundles >>> the side data array+count, and a set of helpers operating on this >>> struct. Then this new struct is added to AVCodecParameters and >>> AVCodecContext, which replaces the abovementioned AVStream and >>> AVCodecContext side data array+count. Notably AVPacket is left as-is, >>> because its side data is widely used and replacing array+count by this >>> new struct would be a very intrusive break for little gain. >> >> In the future, packet side data could be made ref counted. For this, >> we'd need to add a new AVBufferRef field to AVPacketSideData, which is >> currently tied to the ABI. >> Ideally, if this happens, sizeof(AVPacketSideData) would be made not a >> part of the ABI, putting it in line with the AVFrame counterpart. Doing >> this is pretty much not an option for the existing array+count fields in >> AVPacket and AVCodecContext, because everyone, from external API users >> to our own API users like the CLI last i checked, loop through it >> manually as nothing prevents them to. But if a new field of the new set >> struct is added as replacement (a struct where the AVPacketSideData >> field is an array of pointers, and whose helpers can be defined as "must >> use to handle this struct"), then this would not be an issue. >> >> I probably said it several times, including in the above paragraph, but >> one of my objectives is trying to sync frame and packet side data >> structs, fields and helpers, since right now both are considerably >> different. >> Jan has a patchset also introducing a side data set for frames, so this >> is the time to try and align both as much as possible (which i already >> talked to him about) and laying the ground for future changes. >> >>> >>> My objections to this approach are >>> * coded side data is now stored differently in AVPacket and in >>> everything else >>> * the case for replacing AVCodecContext.coded_side_data does not seem >>> sufficiently strong to me >> >> It's used only by a handful of encoders to export CPB side data, which >> probably only lavf looks at, and by no decoder at all. It's a field that >> would be completely painless to replace. >> >>> >>> My alternative suggestion was: >>> * avoid adding a new struct (which merely bundles array+count) >>> * add a set of helpers that accept array+count as parameters and operate >>> on them >> >> See above my comment about sizeof(AVPacketSideData) and users accessing >> the array it directly. >> >>> * use these helpers for all operations on side data across AVPacket, >>> AVCodecContext, and AVCodecParameters >>> >>> I have a moderately strong preference for this approach, but James >>> disagrees. More opinions are very much welcome. > > Ping. We really need opinions on what approach to use. > > Here's some visual representation of what is being discussed. > Currently, things look like this: > > ######### > Side data definition > ----- > struct AVPacketSideData { > uint8_t *data; > size_t size; > enum AVPacketSideDataType type; > }; > ----- > > Users > ----- > struct AVCodecContext { > [...] > } > > struct AVCodecParameters { > [...] > } > > struct AVPacket { > [...] > AVPacketSideData *side_data; > int side_data_elems; > [...] > } > uint8_t* av_packet_new_side_data(AVPacket *pkt, enum > AVPacketSideDataType type, > size_t size); > int av_packet_add_side_data(AVPacket *pkt, > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > uint8_t* av_packet_get_side_data(const AVPacket *pkt, > enum AVPacketSideDataType type, > size_t *size); > > struct AVStream { > [...] > AVCodecParameters *codec_par; > AVPacketSideData *side_data; > int nb_side_data; > [...] > }; > uint8_t* av_stream_new_side_data(AVStream *stream, > enum AVPacketSideDataType type, > size_t size); > int av_stream_add_side_data(AVStream *st, > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > uint8_t* av_stream_get_side_data(const AVStream *stream, > enum AVPacketSideDataType type, > size_t *size); > ----- > ######### > > The only way for global side data exported by demuxers to hit decoders > or bsfs is by injecting it into the first output packet. This means it's > not available at init(). > The solution is obviously being able to store the side data in the > decoder context, which the caller would copy from the demuxer context. > My suggestion is to introduce a new struct to hold a set of side data, > and helpers that work with it, whereas Anton's suggestion is to just > storing the pointer to side data array and side data array size directly > without wrapping them. > > My suggestion would have things look like this: > > ######### > Side data definition > ----- > struct AVPacketSideData { > uint8_t *data; > size_t size; > enum AVPacketSideDataType type; > }; > > struct AVPacketSideDataSet { > AVPacketSideData **sd; > int nb_sd; > }; > > AVPacketSideData *av_packet_side_data_set_new(AVPacketSideDataSet *set, > enum AVPacketSideDataType type, > size_t size); > AVPacketSideData *av_packet_side_data_set_add(AVPacketSideDataSet *set, > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > AVPacketSideData *av_packet_side_data_set_get(AVPacketSideDataSet *set, > enum AVPacketSideDataType type); > void av_packet_side_data_set_remove(AVPacketSideDataSet *set, > enum AVPacketSideDataType type); > void av_packet_side_data_set_uninit(AVPacketSideDataSet *set); > ----- > > Users > ----- > struct AVCodecContext { > [...] > AVPacketSideDataSet *side_data; > [...] > } > > struct AVCodecParameters { > [...] > AVPacketSideDataSet *side_data; > [...] > } > > AVPacket and its helpers untouched. > > struct AVStream { > [...] > AVCodecParameters *codec_par; > [...] > }; > ----- > > In which you'd do for example > > av_packet_side_data_set_new(avctx->side_data, > AV_PKT_DATA_DISPLAYMATRIX, > sizeof(int32_t) * 9); > av_packet_side_data_set_new(st->codec_par->side_data, > AV_PKT_DATA_DISPLAYMATRIX, > sizeof(int32_t) * 9); > av_packet_side_data_set_get(avctx->side_data, > AV_PKT_DATA_DISPLAYMATRIX); > av_packet_side_data_set_get(st->codec_par->side_data, > AV_PKT_DATA_DISPLAYMATRIX); > ######### > > Whereas Anton's would have things look like this: > > ######### > Side data definition > ----- > struct AVPacketSideData { > uint8_t *data; > size_t size; > enum AVPacketSideDataType type; > }; > > AVPacketSideData *av_packet_side_data_set_new( > AVPacketSideData **sd, // INOUT > size_t *sd_size, // INOUT > enum AVPacketSideDataType type, > size_t size); > AVPacketSideData *av_packet_side_data_set_add( > AVPacketSideData **sd, // INOUT > size_t *sd_size, // INOUT > enum AVPacketSideDataType type, > uint8_t *data, size_t size); > AVPacketSideData *av_packet_side_data_set_get(AVPacketSideData *sd, > size_t sd_size, > enum AVPacketSideDataType type); > void av_packet_side_data_set_remove(AVPacketSideData *sd, > size_t *sd_size, > enum AVPacketSideDataType type); > void av_packet_side_data_set_uninit(AVPacketSideData *sd); > ----- > > Users > ----- > struct AVCodecContext { > [...] > AVPacketSideData *coded_side_data; > int nb_coded_side_data; > [...] > } > > struct AVCodecParameters { > [...] > AVPacketSideData *side_data; > int nb_side_data; > [...] > } > > AVPacket and its helpers untouched. > > struct AVStream { > [...] > AVCodecParameters *codec_par; > [...] > }; > ----- > > In which you'd do for example > > av_packet_side_data_set_new(&avctx->coded_side_data, > &avctx->nb_coded_side_data, > AV_PKT_DATA_DISPLAYMATRIX, > av_packet_side_data_set_new(&st->codec_par->side_data, > &st->codec_par->nb_side_data, > AV_PKT_DATA_DISPLAYMATRIX, > sizeof(int32_t) * 9); > av_packet_side_data_set_get(avctx->coded_side_data, > avctx->nb_coded_side_data, > AV_PKT_DATA_DISPLAYMATRIX); > av_packet_side_data_set_get(st->codec_par->side_data, > st->codec_par->nb_side_data, > AV_PKT_DATA_DISPLAYMATRIX); > ######### > > Does anyone have a preference? Addition of newer API to just reduce number of arguments when using it looks to me too much for little gain. > _______________________________________________ > 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". > _______________________________________________ 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".