[FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for packets with top/bottom field

[Patrick Keroulas]

----- Original Message -----
> From: "Rostislav Pehlivanov" <atomnuker at gmail.com>
> To: "FFmpeg development discussions and patches" <ffmpeg-devel at ffmpeg.org>
> Sent: Wednesday, May 23, 2018 3:25:34 PM
> Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for packets with top/bottom field

> On 23 May 2018 at 20:01, wm4 <nfxjfg at googlemail.com> wrote:
> 
>> On Wed, 23 May 2018 14:29:38 -0400 (EDT)
>> Patrick Keroulas <patrick.keroulas at savoirfairelinux.com> wrote:
>> 
>> > ----- Original Message -----
>> > > From: "wm4" <nfxjfg at googlemail.com>
>> > > To: ffmpeg-devel at ffmpeg.org
>> > > Sent: Wednesday, May 23, 2018 12:02:45 PM
>> > > Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for
>> packets with top/bottom field
>> > 
>> > > On Wed, 23 May 2018 16:46:17 +0100
>> > > Rostislav Pehlivanov <atomnuker at gmail.com> wrote:
>> > > 
>> > >> On 23 May 2018 at 16:18, wm4 <nfxjfg at googlemail.com> wrote:
>> > >> 
>> > >> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
>> > >> > Patrick Keroulas <patrick.keroulas at savoirfairelinux.com> wrote:
>> > >> > 
>> > >> > > ----- Original Message -----
>> > >> > > > From: "Rostislav Pehlivanov" <atomnuker at gmail.com>
>> > >> > > > To: "FFmpeg development discussions and patches" <
>> > >> > ffmpeg-devel at ffmpeg.org>
>> > >> > > > Sent: Friday, May 18, 2018 5:28:42 PM
>> > >> > > > Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags
>> for
>> > >> > packets with top/bottom field
>> > >> > > 
>> > >> > > > On 18 May 2018 at 22:17, wm4 <nfxjfg at googlemail.com> wrote:
>> > >> > > 
>> > >> > > 
>> > >> > > 
>> > >> > > > > But I think a new side data type would be much saner. We
>> could even
>> > >> > > > > just make it something generic, like AV_PKT_DATA_ANCILLARY or
>> > >> > > > > something. It's apparently just packet data which somehow
>> couldn't go
>> > >> > > > > into the packet data.
>> > >> > > 
>> > >> > > 
>> > >> > > > I agree, a generic ancillary side data type sounds better. It
>> would
>> > >> > have to
>> > >> > > > be handled the same way as mastering metadata (e.g. to allocate
>> it
>> > >> > you'd
>> > >> > > > need to use a separate function), since the size of the data
>> struct
>> > >> > can't
>> > >> > > > be part of the API if we intend to add fields later.
>> > >> > > > Patrick, if you're okay with that you should submit a patch
>> which bases
>> > >> > > > such side data on libavutil/mastering_display_metadata.h/.c
>> > >> > > 
>> > >> > > No problem for transmitting field flags through side data. But
>> the given
>> > >> > > example (libavutil/mastering_display_metadata.h/.c) attaches
>> data to
>> > >> > > AVFrame, not AVPacket, so I'm not sure where to place this
>> separate
>> > >> > > allocator function. Do you recommend to create a new
>> > >> > > libavcodec/ancillary.c/h utility?
>> > >> > 
>> > >> > The example you mentioned exists for AVPacket too (it's just not
>> easy
>> > >> > to see how it can end up in AVPacket, because no demuxer does that
>> > >> > directly).
>> > >> > 
>> > >> > Anyway, ancillary side data would just be an untyped byte array, so
>> I
>> > >> > don't think it needs any helpers. Just an addition to the packet
>> side
>> > >> > data enum (I think it's somewhere in avcodec.h).
>> > >> > _______________________________________________
>> > >> > ffmpeg-devel mailing list
>> > >> > ffmpeg-devel at ffmpeg.org
>> > >> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > >> > 
>> > >> 
>> > >> I'd rather have it as a well defined typed array rather than a bunch
>> of
>> > >> bytes. Otherwise we'd start sending unknown side data info and users
>> > >> wouldn't know what to do with it.
>> > > 
>> > > Unless you're adding some meta object system for describing arbitrary
>> > > types at runtime I don't know how you'd do that.
>> > 
>> > Is that ok if I simply define a basic struct to hold the field?
>> > 
>> > Any suggestion on where to insert the definition of this data and the
>> > accessors in lavc? In a new source file?
>> 
>> If you make it a struct, then make a new file in libavutil, with
>> at least a helper to get the struct size (this is for ABI reasons, so
>> we can extend the struct later). But then this side data would need a
>> specific name, not a generic one like "ancillary".
>> 
>> The display mastering thing is valid for both packets and frames, which
>> might be confusing. The thing you add is needed for packets only.
>> 
>> I'd prefer the "ancillary" name and making it just a flat byte array
>> instead of a struct and something specific. The former would be like
>> extradata, just per packet.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
> 
> A flat array would be useless and very codec specific (e.g. if you throw
> that side data at one codec it would act in a different way than another
> codec), a struct is the way to go here. I don't mind adding another untyped
> data if there was a reason, but what we're trying to solve here is very
> well defined - determine the field of each packet.
> 
> Patrick, like I said, use libavutil/mastering_display_metadata.c/h as a
> template.

Taking mastering_display_metadata as an example will work for the new struct
definition and allocator only. The side_data accessors can't be defined in the
same place because there is no concept of AVPacket in libavutil. But they may
not be necessary, and using av_packet_*_side_data directly in the demux and 
the decoder would be fine.