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

Wed May 23 23:45:38 EEST 2018

On 23 May 2018 at 20:49, wm4 <nfxjfg at googlemail.com> wrote:

> On Wed, 23 May 2018 20:25:34 +0100
> Rostislav Pehlivanov <atomnuker at gmail.com> wrote:
>
> > 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.
>
> I see it rather as: some obscure codec needs some bytes per packet, but
> out of band, so let's add a side data that does that. That side data
> would of course be codec specific by definition.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

I don't think it should be codec specific, other use cases for it may
appear, like V210 interlacing. And because we can't change AVPackets, other
fields that should have gone there could go in the ancillary side data. So
I think it should be done just as mastering display metadata is done.