diff mbox

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

Message ID 1526336797-21703-1-git-send-email-patrick.keroulas@savoirfairelinux.com
State New
Headers show

Commit Message

Patrick Keroulas May 14, 2018, 10:26 p.m. UTC 
Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
---
 doc/APIchanges       | 3 +++
 libavcodec/avcodec.h | 8 ++++++++
 libavcodec/version.h | 4 ++--
 3 files changed, 13 insertions(+), 2 deletions(-)

Comments

wm4 May 15, 2018, 2:55 p.m. UTC | #1 
On Mon, 14 May 2018 18:26:35 -0400
Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:

> Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> ---
>  doc/APIchanges       | 3 +++
>  libavcodec/avcodec.h | 8 ++++++++
>  libavcodec/version.h | 4 ++--
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index bbefc83..d06868e 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> +  Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> +
>  2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
>    Add AVCUDADeviceContext.stream.
>  
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index fb0c6fa..14811be 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
>   */
>  #define AV_PKT_FLAG_DISPOSABLE 0x0010
>  
> +/**
> + * The packet contains a top field.
> + */
> +#define AV_PKT_FLAG_TOP_FIELD  0x0020
> +/**
> + * The packet contains a bottom field.
> + */
> +#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
>  
>  enum AVSideDataParamChangeFlags {
>      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 3fda743..b9752ce 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,8 +28,8 @@
>  #include "libavutil/version.h"
>  
>  #define LIBAVCODEC_VERSION_MAJOR  58
> -#define LIBAVCODEC_VERSION_MINOR  19
> -#define LIBAVCODEC_VERSION_MICRO 101
> +#define LIBAVCODEC_VERSION_MINOR  20
> +#define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \

So far we could avoid codec-specific packet flags, and I think it
should stay this way. Maybe make it side data, something with naming
specific to the bitpacked codec. Or alternatively, if this codec
is 100% RTP specific and there's no such thing as standard bitpacked
packets (e.g. muxed in other files etc.), add it to the packet
directly. The RTP code "repacks" it already on the libavformat side
anyway.
Rostislav Pehlivanov May 15, 2018, 4:15 p.m. UTC | #2 
On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:

> On Mon, 14 May 2018 18:26:35 -0400
> Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>
> > Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> > ---
> >  doc/APIchanges       | 3 +++
> >  libavcodec/avcodec.h | 8 ++++++++
> >  libavcodec/version.h | 4 ++--
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index bbefc83..d06868e 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> >
> >  API changes, most recent first:
> >
> > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > +  Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > +
> >  2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> >    Add AVCUDADeviceContext.stream.
> >
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index fb0c6fa..14811be 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> >   */
> >  #define AV_PKT_FLAG_DISPOSABLE 0x0010
> >
> > +/**
> > + * The packet contains a top field.
> > + */
> > +#define AV_PKT_FLAG_TOP_FIELD  0x0020
> > +/**
> > + * The packet contains a bottom field.
> > + */
> > +#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
> >
> >  enum AVSideDataParamChangeFlags {
> >      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > index 3fda743..b9752ce 100644
> > --- a/libavcodec/version.h
> > +++ b/libavcodec/version.h
> > @@ -28,8 +28,8 @@
> >  #include "libavutil/version.h"
> >
> >  #define LIBAVCODEC_VERSION_MAJOR  58
> > -#define LIBAVCODEC_VERSION_MINOR  19
> > -#define LIBAVCODEC_VERSION_MICRO 101
> > +#define LIBAVCODEC_VERSION_MINOR  20
> > +#define LIBAVCODEC_VERSION_MICRO 100
> >
> >  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR,
> \
> >
>  LIBAVCODEC_VERSION_MINOR, \
>
> So far we could avoid codec-specific packet flags, and I think it
> should stay this way. Maybe make it side data, something with naming
> specific to the bitpacked codec. Or alternatively, if this codec
> is 100% RTP specific and there's no such thing as standard bitpacked
> packets (e.g. muxed in other files etc.), add it to the packet
> directly. The RTP code "repacks" it already on the libavformat side
> anyway.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

This codec isn't RTP specific, its the same as V210. There are no flags in
the bitstream, its just a sequence of packed pixels. And just like v210
there's a standard for what packets need to look like (rfc4175, and
unfortunately it does specify the fields need to be separate), so packing 2
fields in the muxer isn't really an option.
Side data seems a bit of an overkill for a flag. I'd say the field flags
are not codec specific as some raw codecs and containers can signal fields
per packet. So I don't really mind them.
wm4 May 15, 2018, 5:03 p.m. UTC | #3 
On Tue, 15 May 2018 17:15:05 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Mon, 14 May 2018 18:26:35 -0400
> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> >  
> > > Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> > > ---
> > >  doc/APIchanges       | 3 +++
> > >  libavcodec/avcodec.h | 8 ++++++++
> > >  libavcodec/version.h | 4 ++--
> > >  3 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/doc/APIchanges b/doc/APIchanges
> > > index bbefc83..d06868e 100644
> > > --- a/doc/APIchanges
> > > +++ b/doc/APIchanges
> > > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> > >
> > >  API changes, most recent first:
> > >
> > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > > +  Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > > +
> > >  2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> > >    Add AVCUDADeviceContext.stream.
> > >
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index fb0c6fa..14811be 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> > >   */
> > >  #define AV_PKT_FLAG_DISPOSABLE 0x0010
> > >
> > > +/**
> > > + * The packet contains a top field.
> > > + */
> > > +#define AV_PKT_FLAG_TOP_FIELD  0x0020
> > > +/**
> > > + * The packet contains a bottom field.
> > > + */
> > > +#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
> > >
> > >  enum AVSideDataParamChangeFlags {
> > >      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > > index 3fda743..b9752ce 100644
> > > --- a/libavcodec/version.h
> > > +++ b/libavcodec/version.h
> > > @@ -28,8 +28,8 @@
> > >  #include "libavutil/version.h"
> > >
> > >  #define LIBAVCODEC_VERSION_MAJOR  58
> > > -#define LIBAVCODEC_VERSION_MINOR  19
> > > -#define LIBAVCODEC_VERSION_MICRO 101
> > > +#define LIBAVCODEC_VERSION_MINOR  20
> > > +#define LIBAVCODEC_VERSION_MICRO 100
> > >
> > >  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR,  
> > \  
> > >  
> >  LIBAVCODEC_VERSION_MINOR, \
> >
> > So far we could avoid codec-specific packet flags, and I think it
> > should stay this way. Maybe make it side data, something with naming
> > specific to the bitpacked codec. Or alternatively, if this codec
> > is 100% RTP specific and there's no such thing as standard bitpacked
> > packets (e.g. muxed in other files etc.), add it to the packet
> > directly. The RTP code "repacks" it already on the libavformat side
> > anyway.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >  
> 
> This codec isn't RTP specific, its the same as V210. There are no flags in
> the bitstream, its just a sequence of packed pixels. And just like v210
> there's a standard for what packets need to look like (rfc4175, and
> unfortunately it does specify the fields need to be separate), so packing 2
> fields in the muxer isn't really an option.

Then why are there separate bitpacked and v210 decoders/codec_ids?

> Side data seems a bit of an overkill for a flag. I'd say the field flags
> are not codec specific as some raw codecs and containers can signal fields
> per packet. So I don't really mind them.

You want codec specific flags to accumulate in AVPacket.flags? Can't way
until we change the flags field to int128_t.
Michael Niedermayer May 15, 2018, 6:19 p.m. UTC | #4 
On Tue, May 15, 2018 at 05:15:05PM +0100, Rostislav Pehlivanov wrote:
> On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Mon, 14 May 2018 18:26:35 -0400
> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> >
> > > Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> > > ---
> > >  doc/APIchanges       | 3 +++
> > >  libavcodec/avcodec.h | 8 ++++++++
> > >  libavcodec/version.h | 4 ++--
> > >  3 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/doc/APIchanges b/doc/APIchanges
> > > index bbefc83..d06868e 100644
> > > --- a/doc/APIchanges
> > > +++ b/doc/APIchanges
> > > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> > >
> > >  API changes, most recent first:
> > >
> > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > > +  Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > > +
> > >  2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> > >    Add AVCUDADeviceContext.stream.
> > >
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index fb0c6fa..14811be 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> > >   */
> > >  #define AV_PKT_FLAG_DISPOSABLE 0x0010
> > >
> > > +/**
> > > + * The packet contains a top field.
> > > + */
> > > +#define AV_PKT_FLAG_TOP_FIELD  0x0020
> > > +/**
> > > + * The packet contains a bottom field.
> > > + */
> > > +#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
> > >
> > >  enum AVSideDataParamChangeFlags {
> > >      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > > index 3fda743..b9752ce 100644
> > > --- a/libavcodec/version.h
> > > +++ b/libavcodec/version.h
> > > @@ -28,8 +28,8 @@
> > >  #include "libavutil/version.h"
> > >
> > >  #define LIBAVCODEC_VERSION_MAJOR  58
> > > -#define LIBAVCODEC_VERSION_MINOR  19
> > > -#define LIBAVCODEC_VERSION_MICRO 101
> > > +#define LIBAVCODEC_VERSION_MINOR  20
> > > +#define LIBAVCODEC_VERSION_MICRO 100
> > >
> > >  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR,
> > \
> > >
> >  LIBAVCODEC_VERSION_MINOR, \
> >
> > So far we could avoid codec-specific packet flags, and I think it
> > should stay this way. Maybe make it side data, something with naming
> > specific to the bitpacked codec. Or alternatively, if this codec
> > is 100% RTP specific and there's no such thing as standard bitpacked
> > packets (e.g. muxed in other files etc.), add it to the packet
> > directly. The RTP code "repacks" it already on the libavformat side
> > anyway.
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> This codec isn't RTP specific, its the same as V210. There are no flags in
> the bitstream, its just a sequence of packed pixels. And just like v210
> there's a standard for what packets need to look like (rfc4175, and

> unfortunately it does specify the fields need to be separate), so packing 2
> fields in the muxer isn't really an option.

Just commenting on this without intending to state an oppinion on the
specific codec

Where we draw the line in the implementation between muxer and codec.
That is AVPackets is the FFmpeg teams decission not some specs.

A spec can from defining a container as a absract archive of streams
that is leaving everything including identifying the codec to the codec.
to having a container layer that outputs fully decoded images.

It would be a inconsistent mess if we followed especially the more extreem
cases design wise.

[...]
Rostislav Pehlivanov May 15, 2018, 8:46 p.m. UTC | #5 
On 15 May 2018 at 18:03, wm4 <nfxjfg@googlemail.com> wrote:

> On Tue, 15 May 2018 17:15:05 +0100
> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
>
> > On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> >
> > > On Mon, 14 May 2018 18:26:35 -0400
> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > >
> > > > Signed-off-by: Patrick Keroulas <patrick.keroulas@
> savoirfairelinux.com>
> > > > ---
> > > >  doc/APIchanges       | 3 +++
> > > >  libavcodec/avcodec.h | 8 ++++++++
> > > >  libavcodec/version.h | 4 ++--
> > > >  3 files changed, 13 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/doc/APIchanges b/doc/APIchanges
> > > > index bbefc83..d06868e 100644
> > > > --- a/doc/APIchanges
> > > > +++ b/doc/APIchanges
> > > > @@ -15,6 +15,9 @@ libavutil:     2017-10-21
> > > >
> > > >  API changes, most recent first:
> > > >
> > > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > > > +  Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > > > +
> > > >  2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> > > >    Add AVCUDADeviceContext.stream.
> > > >
> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > index fb0c6fa..14811be 100644
> > > > --- a/libavcodec/avcodec.h
> > > > +++ b/libavcodec/avcodec.h
> > > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> > > >   */
> > > >  #define AV_PKT_FLAG_DISPOSABLE 0x0010
> > > >
> > > > +/**
> > > > + * The packet contains a top field.
> > > > + */
> > > > +#define AV_PKT_FLAG_TOP_FIELD  0x0020
> > > > +/**
> > > > + * The packet contains a bottom field.
> > > > + */
> > > > +#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
> > > >
> > > >  enum AVSideDataParamChangeFlags {
> > > >      AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
> > > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > > > index 3fda743..b9752ce 100644
> > > > --- a/libavcodec/version.h
> > > > +++ b/libavcodec/version.h
> > > > @@ -28,8 +28,8 @@
> > > >  #include "libavutil/version.h"
> > > >
> > > >  #define LIBAVCODEC_VERSION_MAJOR  58
> > > > -#define LIBAVCODEC_VERSION_MINOR  19
> > > > -#define LIBAVCODEC_VERSION_MICRO 101
> > > > +#define LIBAVCODEC_VERSION_MINOR  20
> > > > +#define LIBAVCODEC_VERSION_MICRO 100
> > > >
> > > >  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR,
>
> > > \
> > > >
> > >  LIBAVCODEC_VERSION_MINOR, \
> > >
> > > So far we could avoid codec-specific packet flags, and I think it
> > > should stay this way. Maybe make it side data, something with naming
> > > specific to the bitpacked codec. Or alternatively, if this codec
> > > is 100% RTP specific and there's no such thing as standard bitpacked
> > > packets (e.g. muxed in other files etc.), add it to the packet
> > > directly. The RTP code "repacks" it already on the libavformat side
> > > anyway.
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> >
> > This codec isn't RTP specific, its the same as V210. There are no flags
> in
> > the bitstream, its just a sequence of packed pixels. And just like v210
> > there's a standard for what packets need to look like (rfc4175, and
> > unfortunately it does specify the fields need to be separate), so
> packing 2
> > fields in the muxer isn't really an option.
>
> Then why are there separate bitpacked and v210 decoders/codec_ids?
>

Its a different type of packing.



> > Side data seems a bit of an overkill for a flag. I'd say the field flags
> > are not codec specific as some raw codecs and containers can signal
> fields
> > per packet. So I don't really mind them.
>
> You want codec specific flags to accumulate in AVPacket.flags? Can't way
> until we change the flags field to int128_t.
>
>
No, but I think 2 more bits won't hurt much, especially considering pretty
much all formats supporting interlaced content split each field into a
separate packet.
Derek Buitenhuis May 15, 2018, 8:54 p.m. UTC | #6 
On Mon, May 14, 2018 at 11:26 PM, Patrick Keroulas
<patrick.keroulas@savoirfairelinux.com> wrote:
> Signed-off-by: Patrick Keroulas <patrick.keroulas@savoirfairelinux.com>
> ---
>  doc/APIchanges       | 3 +++
>  libavcodec/avcodec.h | 8 ++++++++
>  libavcodec/version.h | 4 ++--
>  3 files changed, 13 insertions(+), 2 deletions(-)

Doesn't this kind of duplicate AVPictureStructure?

- Derek
Patrick Keroulas May 18, 2018, 7:05 p.m. UTC | #7 
----- Original Message -----
> From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> To: "FFmpeg development discussions and patches" <ffmpeg-devel@ffmpeg.org>
> Sent: Tuesday, May 15, 2018 4:46:02 PM
> Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for packets with top/bottom field

> On 15 May 2018 at 18:03, wm4 <nfxjfg@googlemail.com> wrote:
> 
>> On Tue, 15 May 2018 17:15:05 +0100
>> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
>> 
>> > On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
>> > 
>> > > On Mon, 14 May 2018 18:26:35 -0400
>> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>> > > 
>> > > > Signed-off-by: Patrick Keroulas <patrick.keroulas@
>> savoirfairelinux.com>
>> > > > ---
>> > > > doc/APIchanges | 3 +++
>> > > > libavcodec/avcodec.h | 8 ++++++++
>> > > > libavcodec/version.h | 4 ++--
>> > > > 3 files changed, 13 insertions(+), 2 deletions(-)
>> > > > 
>> > > > diff --git a/doc/APIchanges b/doc/APIchanges
>> > > > index bbefc83..d06868e 100644
>> > > > --- a/doc/APIchanges
>> > > > +++ b/doc/APIchanges
>> > > > @@ -15,6 +15,9 @@ libavutil: 2017-10-21
>> > > > 
>> > > > API changes, most recent first:
>> > > > 
>> > > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
>> > > > + Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
>> > > > +
>> > > > 2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
>> > > > Add AVCUDADeviceContext.stream.
>> > > > 
>> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> > > > index fb0c6fa..14811be 100644
>> > > > --- a/libavcodec/avcodec.h
>> > > > +++ b/libavcodec/avcodec.h
>> > > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
>> > > > */
>> > > > #define AV_PKT_FLAG_DISPOSABLE 0x0010
>> > > > 
>> > > > +/**
>> > > > + * The packet contains a top field.
>> > > > + */
>> > > > +#define AV_PKT_FLAG_TOP_FIELD 0x0020
>> > > > +/**
>> > > > + * The packet contains a bottom field.
>> > > > + */
>> > > > +#define AV_PKT_FLAG_BOTTOM_FIELD 0x0040
>> > > > 
>> > > > enum AVSideDataParamChangeFlags {
>> > > > AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT = 0x0001,
>> > > > diff --git a/libavcodec/version.h b/libavcodec/version.h
>> > > > index 3fda743..b9752ce 100644
>> > > > --- a/libavcodec/version.h
>> > > > +++ b/libavcodec/version.h
>> > > > @@ -28,8 +28,8 @@
>> > > > #include "libavutil/version.h"
>> > > > 
>> > > > #define LIBAVCODEC_VERSION_MAJOR 58
>> > > > -#define LIBAVCODEC_VERSION_MINOR 19
>> > > > -#define LIBAVCODEC_VERSION_MICRO 101
>> > > > +#define LIBAVCODEC_VERSION_MINOR 20
>> > > > +#define LIBAVCODEC_VERSION_MICRO 100
>> > > > 
>> > > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR,
>> 
>> > > \
>> > > > 
>> > > LIBAVCODEC_VERSION_MINOR, \
>> > > 
>> > > So far we could avoid codec-specific packet flags, and I think it
>> > > should stay this way. Maybe make it side data, something with naming
>> > > specific to the bitpacked codec. Or alternatively, if this codec
>> > > is 100% RTP specific and there's no such thing as standard bitpacked
>> > > packets (e.g. muxed in other files etc.), add it to the packet
>> > > directly. The RTP code "repacks" it already on the libavformat side
>> > > anyway.
>> > > _______________________________________________
>> > > ffmpeg-devel mailing list
>> > > ffmpeg-devel@ffmpeg.org
>> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > > 
>> > 
>> > This codec isn't RTP specific, its the same as V210. There are no flags
>> in
>> > the bitstream, its just a sequence of packed pixels. And just like v210
>> > there's a standard for what packets need to look like (rfc4175, and
>> > unfortunately it does specify the fields need to be separate), so
>> packing 2
>> > fields in the muxer isn't really an option.
>> 
>> Then why are there separate bitpacked and v210 decoders/codec_ids?
>> 
> 
> Its a different type of packing.
> 
> 
> 
>> > Side data seems a bit of an overkill for a flag. I'd say the field flags
>> > are not codec specific as some raw codecs and containers can signal
>> fields
>> > per packet. So I don't really mind them.
>> 
>> You want codec specific flags to accumulate in AVPacket.flags? Can't way
>> until we change the flags field to int128_t.
>> 
>> 
> No, but I think 2 more bits won't hurt much, especially considering pretty
> much all formats supporting interlaced content split each field into a
> separate packet.

Recomposing a frame from fields on the demux side would make the bitpacked decoder
completely useless. Can we find a consensus? How about reusing AVPictureStructure
as suggested by Derek?

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Rostislav Pehlivanov May 18, 2018, 7:09 p.m. UTC | #8 
On 18 May 2018 at 20:05, Patrick Keroulas <
patrick.keroulas@savoirfairelinux.com> wrote:

>
> ----- Original Message -----
> > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > To: "FFmpeg development discussions and patches" <
> ffmpeg-devel@ffmpeg.org>
> > Sent: Tuesday, May 15, 2018 4:46:02 PM
> > Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for
> packets with top/bottom field
>
> > On 15 May 2018 at 18:03, wm4 <nfxjfg@googlemail.com> wrote:
> >
> >> On Tue, 15 May 2018 17:15:05 +0100
> >> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
> >>
> >> > On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> >> >
> >> > > On Mon, 14 May 2018 18:26:35 -0400
> >> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> >> > >
> >> > > > Signed-off-by: Patrick Keroulas <patrick.keroulas@
> >> savoirfairelinux.com>
> >> > > > ---
> >> > > > doc/APIchanges | 3 +++
> >> > > > libavcodec/avcodec.h | 8 ++++++++
> >> > > > libavcodec/version.h | 4 ++--
> >> > > > 3 files changed, 13 insertions(+), 2 deletions(-)
> >> > > >
> >> > > > diff --git a/doc/APIchanges b/doc/APIchanges
> >> > > > index bbefc83..d06868e 100644
> >> > > > --- a/doc/APIchanges
> >> > > > +++ b/doc/APIchanges
> >> > > > @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> >> > > >
> >> > > > API changes, most recent first:
> >> > > >
> >> > > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> >> > > > + Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> >> > > > +
> >> > > > 2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> >> > > > Add AVCUDADeviceContext.stream.
> >> > > >
> >> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> >> > > > index fb0c6fa..14811be 100644
> >> > > > --- a/libavcodec/avcodec.h
> >> > > > +++ b/libavcodec/avcodec.h
> >> > > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> >> > > > */
> >> > > > #define AV_PKT_FLAG_DISPOSABLE 0x0010
> >> > > >
> >> > > > +/**
> >> > > > + * The packet contains a top field.
> >> > > > + */
> >> > > > +#define AV_PKT_FLAG_TOP_FIELD 0x0020
> >> > > > +/**
> >> > > > + * The packet contains a bottom field.
> >> > > > + */
> >> > > > +#define AV_PKT_FLAG_BOTTOM_FIELD 0x0040
> >> > > >
> >> > > > enum AVSideDataParamChangeFlags {
> >> > > > AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT = 0x0001,
> >> > > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> >> > > > index 3fda743..b9752ce 100644
> >> > > > --- a/libavcodec/version.h
> >> > > > +++ b/libavcodec/version.h
> >> > > > @@ -28,8 +28,8 @@
> >> > > > #include "libavutil/version.h"
> >> > > >
> >> > > > #define LIBAVCODEC_VERSION_MAJOR 58
> >> > > > -#define LIBAVCODEC_VERSION_MINOR 19
> >> > > > -#define LIBAVCODEC_VERSION_MICRO 101
> >> > > > +#define LIBAVCODEC_VERSION_MINOR 20
> >> > > > +#define LIBAVCODEC_VERSION_MICRO 100
> >> > > >
> >> > > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_
> VERSION_MAJOR,
> >>
> >> > > \
> >> > > >
> >> > > LIBAVCODEC_VERSION_MINOR, \
> >> > >
> >> > > So far we could avoid codec-specific packet flags, and I think it
> >> > > should stay this way. Maybe make it side data, something with naming
> >> > > specific to the bitpacked codec. Or alternatively, if this codec
> >> > > is 100% RTP specific and there's no such thing as standard bitpacked
> >> > > packets (e.g. muxed in other files etc.), add it to the packet
> >> > > directly. The RTP code "repacks" it already on the libavformat side
> >> > > anyway.
> >> > > _______________________________________________
> >> > > ffmpeg-devel mailing list
> >> > > ffmpeg-devel@ffmpeg.org
> >> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >> > >
> >> >
> >> > This codec isn't RTP specific, its the same as V210. There are no
> flags
> >> in
> >> > the bitstream, its just a sequence of packed pixels. And just like
> v210
> >> > there's a standard for what packets need to look like (rfc4175, and
> >> > unfortunately it does specify the fields need to be separate), so
> >> packing 2
> >> > fields in the muxer isn't really an option.
> >>
> >> Then why are there separate bitpacked and v210 decoders/codec_ids?
> >>
> >
> > Its a different type of packing.
> >
> >
> >
> >> > Side data seems a bit of an overkill for a flag. I'd say the field
> flags
> >> > are not codec specific as some raw codecs and containers can signal
> >> fields
> >> > per packet. So I don't really mind them.
> >>
> >> You want codec specific flags to accumulate in AVPacket.flags? Can't way
> >> until we change the flags field to int128_t.
> >>
> >>
> > No, but I think 2 more bits won't hurt much, especially considering
> pretty
> > much all formats supporting interlaced content split each field into a
> > separate packet.
>
> Recomposing a frame from fields on the demux side would make the bitpacked
> decoder
> completely useless. Can we find a consensus? How about reusing
> AVPictureStructure
> as suggested by Derek?
>
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Reusing that structure would mean adding a field to AVPackets. I'd rather
avoid that, so I'm okay with the packet flags.
wm4 May 18, 2018, 8:03 p.m. UTC | #9 
On Fri, 18 May 2018 20:09:02 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 18 May 2018 at 20:05, Patrick Keroulas <
> patrick.keroulas@savoirfairelinux.com> wrote:  
> 
> >
> > ----- Original Message -----  
> > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > To: "FFmpeg development discussions and patches" <  
> > ffmpeg-devel@ffmpeg.org>  
> > > Sent: Tuesday, May 15, 2018 4:46:02 PM
> > > Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for  
> > packets with top/bottom field
> >  
> > > On 15 May 2018 at 18:03, wm4 <nfxjfg@googlemail.com> wrote:
> > >  
> > >> On Tue, 15 May 2018 17:15:05 +0100
> > >> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
> > >>  
> > >> > On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> > >> >  
> > >> > > On Mon, 14 May 2018 18:26:35 -0400
> > >> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > >> > >  
> > >> > > > Signed-off-by: Patrick Keroulas <patrick.keroulas@  
> > >> savoirfairelinux.com>  
> > >> > > > ---
> > >> > > > doc/APIchanges | 3 +++
> > >> > > > libavcodec/avcodec.h | 8 ++++++++
> > >> > > > libavcodec/version.h | 4 ++--
> > >> > > > 3 files changed, 13 insertions(+), 2 deletions(-)
> > >> > > >
> > >> > > > diff --git a/doc/APIchanges b/doc/APIchanges
> > >> > > > index bbefc83..d06868e 100644
> > >> > > > --- a/doc/APIchanges
> > >> > > > +++ b/doc/APIchanges
> > >> > > > @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> > >> > > >
> > >> > > > API changes, most recent first:
> > >> > > >
> > >> > > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > >> > > > + Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > >> > > > +
> > >> > > > 2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> > >> > > > Add AVCUDADeviceContext.stream.
> > >> > > >
> > >> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > >> > > > index fb0c6fa..14811be 100644
> > >> > > > --- a/libavcodec/avcodec.h
> > >> > > > +++ b/libavcodec/avcodec.h
> > >> > > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> > >> > > > */
> > >> > > > #define AV_PKT_FLAG_DISPOSABLE 0x0010
> > >> > > >
> > >> > > > +/**
> > >> > > > + * The packet contains a top field.
> > >> > > > + */
> > >> > > > +#define AV_PKT_FLAG_TOP_FIELD 0x0020
> > >> > > > +/**
> > >> > > > + * The packet contains a bottom field.
> > >> > > > + */
> > >> > > > +#define AV_PKT_FLAG_BOTTOM_FIELD 0x0040
> > >> > > >
> > >> > > > enum AVSideDataParamChangeFlags {
> > >> > > > AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT = 0x0001,
> > >> > > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > >> > > > index 3fda743..b9752ce 100644
> > >> > > > --- a/libavcodec/version.h
> > >> > > > +++ b/libavcodec/version.h
> > >> > > > @@ -28,8 +28,8 @@
> > >> > > > #include "libavutil/version.h"
> > >> > > >
> > >> > > > #define LIBAVCODEC_VERSION_MAJOR 58
> > >> > > > -#define LIBAVCODEC_VERSION_MINOR 19
> > >> > > > -#define LIBAVCODEC_VERSION_MICRO 101
> > >> > > > +#define LIBAVCODEC_VERSION_MINOR 20
> > >> > > > +#define LIBAVCODEC_VERSION_MICRO 100
> > >> > > >
> > >> > > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_  
> > VERSION_MAJOR,  
> > >>  
> > >> > > \  
> > >> > > >  
> > >> > > LIBAVCODEC_VERSION_MINOR, \
> > >> > >
> > >> > > So far we could avoid codec-specific packet flags, and I think it
> > >> > > should stay this way. Maybe make it side data, something with naming
> > >> > > specific to the bitpacked codec. Or alternatively, if this codec
> > >> > > is 100% RTP specific and there's no such thing as standard bitpacked
> > >> > > packets (e.g. muxed in other files etc.), add it to the packet
> > >> > > directly. The RTP code "repacks" it already on the libavformat side
> > >> > > anyway.
> > >> > > _______________________________________________
> > >> > > ffmpeg-devel mailing list
> > >> > > ffmpeg-devel@ffmpeg.org
> > >> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >> > >  
> > >> >
> > >> > This codec isn't RTP specific, its the same as V210. There are no  
> > flags  
> > >> in  
> > >> > the bitstream, its just a sequence of packed pixels. And just like  
> > v210  
> > >> > there's a standard for what packets need to look like (rfc4175, and
> > >> > unfortunately it does specify the fields need to be separate), so  
> > >> packing 2  
> > >> > fields in the muxer isn't really an option.  
> > >>
> > >> Then why are there separate bitpacked and v210 decoders/codec_ids?
> > >>  
> > >
> > > Its a different type of packing.
> > >
> > >
> > >  
> > >> > Side data seems a bit of an overkill for a flag. I'd say the field  
> > flags  
> > >> > are not codec specific as some raw codecs and containers can signal  
> > >> fields  
> > >> > per packet. So I don't really mind them.  
> > >>
> > >> You want codec specific flags to accumulate in AVPacket.flags? Can't way
> > >> until we change the flags field to int128_t.
> > >>
> > >>  
> > > No, but I think 2 more bits won't hurt much, especially considering  
> > pretty  
> > > much all formats supporting interlaced content split each field into a
> > > separate packet.  
> >
> > Recomposing a frame from fields on the demux side would make the bitpacked
> > decoder
> > completely useless. Can we find a consensus? How about reusing
> > AVPictureStructure
> > as suggested by Derek?
> >  
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel  
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >  
> 
> Reusing that structure would mean adding a field to AVPackets. I'd rather
> avoid that, so I'm okay with the packet flags.

We can't add fields to AVPacket (ABI issues). I'm against the flags
though. None of the current packet flags are needed for correct
decoding, why change that suddenly?
Rostislav Pehlivanov May 18, 2018, 8:59 p.m. UTC | #10 
On 18 May 2018 at 21:03, wm4 <nfxjfg@googlemail.com> wrote:

> On Fri, 18 May 2018 20:09:02 +0100
> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
>
> > On 18 May 2018 at 20:05, Patrick Keroulas <
> > patrick.keroulas@savoirfairelinux.com> wrote:
> >
> > >
> > > ----- Original Message -----
> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > > To: "FFmpeg development discussions and patches" <
> > > ffmpeg-devel@ffmpeg.org>
> > > > Sent: Tuesday, May 15, 2018 4:46:02 PM
> > > > Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for
> > > packets with top/bottom field
> > >
> > > > On 15 May 2018 at 18:03, wm4 <nfxjfg@googlemail.com> wrote:
> > > >
> > > >> On Tue, 15 May 2018 17:15:05 +0100
> > > >> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
> > > >>
> > > >> > On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> > > >> >
> > > >> > > On Mon, 14 May 2018 18:26:35 -0400
> > > >> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > > >> > >
> > > >> > > > Signed-off-by: Patrick Keroulas <patrick.keroulas@
> > > >> savoirfairelinux.com>
> > > >> > > > ---
> > > >> > > > doc/APIchanges | 3 +++
> > > >> > > > libavcodec/avcodec.h | 8 ++++++++
> > > >> > > > libavcodec/version.h | 4 ++--
> > > >> > > > 3 files changed, 13 insertions(+), 2 deletions(-)
> > > >> > > >
> > > >> > > > diff --git a/doc/APIchanges b/doc/APIchanges
> > > >> > > > index bbefc83..d06868e 100644
> > > >> > > > --- a/doc/APIchanges
> > > >> > > > +++ b/doc/APIchanges
> > > >> > > > @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> > > >> > > >
> > > >> > > > API changes, most recent first:
> > > >> > > >
> > > >> > > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > > >> > > > + Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > > >> > > > +
> > > >> > > > 2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> > > >> > > > Add AVCUDADeviceContext.stream.
> > > >> > > >
> > > >> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > >> > > > index fb0c6fa..14811be 100644
> > > >> > > > --- a/libavcodec/avcodec.h
> > > >> > > > +++ b/libavcodec/avcodec.h
> > > >> > > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> > > >> > > > */
> > > >> > > > #define AV_PKT_FLAG_DISPOSABLE 0x0010
> > > >> > > >
> > > >> > > > +/**
> > > >> > > > + * The packet contains a top field.
> > > >> > > > + */
> > > >> > > > +#define AV_PKT_FLAG_TOP_FIELD 0x0020
> > > >> > > > +/**
> > > >> > > > + * The packet contains a bottom field.
> > > >> > > > + */
> > > >> > > > +#define AV_PKT_FLAG_BOTTOM_FIELD 0x0040
> > > >> > > >
> > > >> > > > enum AVSideDataParamChangeFlags {
> > > >> > > > AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT = 0x0001,
> > > >> > > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > > >> > > > index 3fda743..b9752ce 100644
> > > >> > > > --- a/libavcodec/version.h
> > > >> > > > +++ b/libavcodec/version.h
> > > >> > > > @@ -28,8 +28,8 @@
> > > >> > > > #include "libavutil/version.h"
> > > >> > > >
> > > >> > > > #define LIBAVCODEC_VERSION_MAJOR 58
> > > >> > > > -#define LIBAVCODEC_VERSION_MINOR 19
> > > >> > > > -#define LIBAVCODEC_VERSION_MICRO 101
> > > >> > > > +#define LIBAVCODEC_VERSION_MINOR 20
> > > >> > > > +#define LIBAVCODEC_VERSION_MICRO 100
> > > >> > > >
> > > >> > > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_
> > > VERSION_MAJOR,
> > > >>
> > > >> > > \
> > > >> > > >
> > > >> > > LIBAVCODEC_VERSION_MINOR, \
> > > >> > >
> > > >> > > So far we could avoid codec-specific packet flags, and I think
> it
> > > >> > > should stay this way. Maybe make it side data, something with
> naming
> > > >> > > specific to the bitpacked codec. Or alternatively, if this codec
> > > >> > > is 100% RTP specific and there's no such thing as standard
> bitpacked
> > > >> > > packets (e.g. muxed in other files etc.), add it to the packet
> > > >> > > directly. The RTP code "repacks" it already on the libavformat
> side
> > > >> > > anyway.
> > > >> > > _______________________________________________
> > > >> > > ffmpeg-devel mailing list
> > > >> > > ffmpeg-devel@ffmpeg.org
> > > >> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >> > >
> > > >> >
> > > >> > This codec isn't RTP specific, its the same as V210. There are
> no
> > > flags
> > > >> in
> > > >> > the bitstream, its just a sequence of packed pixels. And just
> like
> > > v210
> > > >> > there's a standard for what packets need to look like (rfc4175,
> and
> > > >> > unfortunately it does specify the fields need to be separate),
> so
> > > >> packing 2
> > > >> > fields in the muxer isn't really an option.
> > > >>
> > > >> Then why are there separate bitpacked and v210 decoders/codec_ids?
> > > >>
> > > >
> > > > Its a different type of packing.
> > > >
> > > >
> > > >
> > > >> > Side data seems a bit of an overkill for a flag. I'd say the
> field
> > > flags
> > > >> > are not codec specific as some raw codecs and containers can
> signal
> > > >> fields
> > > >> > per packet. So I don't really mind them.
> > > >>
> > > >> You want codec specific flags to accumulate in AVPacket.flags?
> Can't way
> > > >> until we change the flags field to int128_t.
> > > >>
> > > >>
> > > > No, but I think 2 more bits won't hurt much, especially considering
> > > pretty
> > > > much all formats supporting interlaced content split each field into
> a
> > > > separate packet.
> > >
> > > Recomposing a frame from fields on the demux side would make the
> bitpacked
> > > decoder
> > > completely useless. Can we find a consensus? How about reusing
> > > AVPictureStructure
> > > as suggested by Derek?
> > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> >
> > Reusing that structure would mean adding a field to AVPackets. I'd rather
> > avoid that, so I'm okay with the packet flags.
>
> We can't add fields to AVPacket (ABI issues). I'm against the flags
> though. None of the current packet flags are needed for correct
> decoding, why change that suddenly?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

AV_PKT_FLAG_TRUSTED is needed to decode some packets, so it would not be an
entirely new change.
On the other hand, using side data would mean having to use
AV_CODEC_CAP_PARAM_CHANGE, adding a AV_SIDE_DATA_PARAM_CHANGE_FIELD and
adding a new AVCodecContext field to indicate the current field of a
packet. Or adding a new 1-byte large side data type to indicate packet
field.
I think the packet flag solution is much saner than that.
wm4 May 18, 2018, 9:17 p.m. UTC | #11 
On Fri, 18 May 2018 21:59:13 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 18 May 2018 at 21:03, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Fri, 18 May 2018 20:09:02 +0100
> > Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
> >  
> > > On 18 May 2018 at 20:05, Patrick Keroulas <  
> > > patrick.keroulas@savoirfairelinux.com> wrote:  
> > >  
> > > >
> > > > ----- Original Message -----  
> > > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > > > To: "FFmpeg development discussions and patches" <  
> > > > ffmpeg-devel@ffmpeg.org>  
> > > > > Sent: Tuesday, May 15, 2018 4:46:02 PM
> > > > > Subject: Re: [FFmpeg-devel] [PATCH v6 1/3] avcodec: add flags for  
> > > > packets with top/bottom field
> > > >  
> > > > > On 15 May 2018 at 18:03, wm4 <nfxjfg@googlemail.com> wrote:
> > > > >  
> > > > >> On Tue, 15 May 2018 17:15:05 +0100
> > > > >> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
> > > > >>  
> > > > >> > On 15 May 2018 at 15:55, wm4 <nfxjfg@googlemail.com> wrote:
> > > > >> >  
> > > > >> > > On Mon, 14 May 2018 18:26:35 -0400
> > > > >> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > > > >> > >  
> > > > >> > > > Signed-off-by: Patrick Keroulas <patrick.keroulas@  
> > > > >> savoirfairelinux.com>  
> > > > >> > > > ---
> > > > >> > > > doc/APIchanges | 3 +++
> > > > >> > > > libavcodec/avcodec.h | 8 ++++++++
> > > > >> > > > libavcodec/version.h | 4 ++--
> > > > >> > > > 3 files changed, 13 insertions(+), 2 deletions(-)
> > > > >> > > >
> > > > >> > > > diff --git a/doc/APIchanges b/doc/APIchanges
> > > > >> > > > index bbefc83..d06868e 100644
> > > > >> > > > --- a/doc/APIchanges
> > > > >> > > > +++ b/doc/APIchanges
> > > > >> > > > @@ -15,6 +15,9 @@ libavutil: 2017-10-21
> > > > >> > > >
> > > > >> > > > API changes, most recent first:
> > > > >> > > >
> > > > >> > > > +2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
> > > > >> > > > + Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
> > > > >> > > > +
> > > > >> > > > 2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
> > > > >> > > > Add AVCUDADeviceContext.stream.
> > > > >> > > >
> > > > >> > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > >> > > > index fb0c6fa..14811be 100644
> > > > >> > > > --- a/libavcodec/avcodec.h
> > > > >> > > > +++ b/libavcodec/avcodec.h
> > > > >> > > > @@ -1480,6 +1480,14 @@ typedef struct AVPacket {
> > > > >> > > > */
> > > > >> > > > #define AV_PKT_FLAG_DISPOSABLE 0x0010
> > > > >> > > >
> > > > >> > > > +/**
> > > > >> > > > + * The packet contains a top field.
> > > > >> > > > + */
> > > > >> > > > +#define AV_PKT_FLAG_TOP_FIELD 0x0020
> > > > >> > > > +/**
> > > > >> > > > + * The packet contains a bottom field.
> > > > >> > > > + */
> > > > >> > > > +#define AV_PKT_FLAG_BOTTOM_FIELD 0x0040
> > > > >> > > >
> > > > >> > > > enum AVSideDataParamChangeFlags {
> > > > >> > > > AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT = 0x0001,
> > > > >> > > > diff --git a/libavcodec/version.h b/libavcodec/version.h
> > > > >> > > > index 3fda743..b9752ce 100644
> > > > >> > > > --- a/libavcodec/version.h
> > > > >> > > > +++ b/libavcodec/version.h
> > > > >> > > > @@ -28,8 +28,8 @@
> > > > >> > > > #include "libavutil/version.h"
> > > > >> > > >
> > > > >> > > > #define LIBAVCODEC_VERSION_MAJOR 58
> > > > >> > > > -#define LIBAVCODEC_VERSION_MINOR 19
> > > > >> > > > -#define LIBAVCODEC_VERSION_MICRO 101
> > > > >> > > > +#define LIBAVCODEC_VERSION_MINOR 20
> > > > >> > > > +#define LIBAVCODEC_VERSION_MICRO 100
> > > > >> > > >
> > > > >> > > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_  
> > > > VERSION_MAJOR,  
> > > > >>  
> > > > >> > > \  
> > > > >> > > >  
> > > > >> > > LIBAVCODEC_VERSION_MINOR, \
> > > > >> > >
> > > > >> > > So far we could avoid codec-specific packet flags, and I think  
> > it  
> > > > >> > > should stay this way. Maybe make it side data, something with  
> > naming  
> > > > >> > > specific to the bitpacked codec. Or alternatively, if this codec
> > > > >> > > is 100% RTP specific and there's no such thing as standard  
> > bitpacked  
> > > > >> > > packets (e.g. muxed in other files etc.), add it to the packet
> > > > >> > > directly. The RTP code "repacks" it already on the libavformat  
> > side  
> > > > >> > > anyway.
> > > > >> > > _______________________________________________
> > > > >> > > ffmpeg-devel mailing list
> > > > >> > > ffmpeg-devel@ffmpeg.org
> > > > >> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > > >> > >  
> > > > >> >
> > > > >> > This codec isn't RTP specific, its the same as V210. There are  
> > no  
> > > > flags  
> > > > >> in  
> > > > >> > the bitstream, its just a sequence of packed pixels. And just  
> > like  
> > > > v210  
> > > > >> > there's a standard for what packets need to look like (rfc4175,  
> > and  
> > > > >> > unfortunately it does specify the fields need to be separate),  
> > so  
> > > > >> packing 2  
> > > > >> > fields in the muxer isn't really an option.  
> > > > >>
> > > > >> Then why are there separate bitpacked and v210 decoders/codec_ids?
> > > > >>  
> > > > >
> > > > > Its a different type of packing.
> > > > >
> > > > >
> > > > >  
> > > > >> > Side data seems a bit of an overkill for a flag. I'd say the  
> > field  
> > > > flags  
> > > > >> > are not codec specific as some raw codecs and containers can  
> > signal  
> > > > >> fields  
> > > > >> > per packet. So I don't really mind them.  
> > > > >>
> > > > >> You want codec specific flags to accumulate in AVPacket.flags?  
> > Can't way  
> > > > >> until we change the flags field to int128_t.
> > > > >>
> > > > >>  
> > > > > No, but I think 2 more bits won't hurt much, especially considering  
> > > > pretty  
> > > > > much all formats supporting interlaced content split each field into  
> > a  
> > > > > separate packet.  
> > > >
> > > > Recomposing a frame from fields on the demux side would make the  
> > bitpacked  
> > > > decoder
> > > > completely useless. Can we find a consensus? How about reusing
> > > > AVPictureStructure
> > > > as suggested by Derek?
> > > >  
> > > > > _______________________________________________
> > > > > ffmpeg-devel mailing list
> > > > > ffmpeg-devel@ffmpeg.org
> > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel  
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >  
> > >
> > > Reusing that structure would mean adding a field to AVPackets. I'd rather
> > > avoid that, so I'm okay with the packet flags.  
> >
> > We can't add fields to AVPacket (ABI issues). I'm against the flags
> > though. None of the current packet flags are needed for correct
> > decoding, why change that suddenly?
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >  
> 
> AV_PKT_FLAG_TRUSTED is needed to decode some packets, so it would not be an
> entirely new change.

That only indicates how the packet was created. It's necessary to
"decode" some packets, but it doesn't really say anything about the
contents. Besides, it could get removed when cleaning up the side data
API.

Other flags are also generic. For a long time, AV_PKT_FLAG_KEY was the
only flag. It applies to every codec, and is in theory not necessary
for decoding (although some decoders might read it anyway, but that's
a separate story).

If we add flags about fields now, this will cause only confusion,
because:

1. Unlike someone would expect, they're not always set, but only for
   the very obscure situation of RTP uncompressed data transport.
2. They're suddenly needed for correct decoding.

> On the other hand, using side data would mean having to use
> AV_CODEC_CAP_PARAM_CHANGE, adding a AV_SIDE_DATA_PARAM_CHANGE_FIELD and
> adding a new AVCodecContext field to indicate the current field of a
> packet. Or adding a new 1-byte large side data type to indicate packet
> field.

I don't know why it can't be written to the packet data itself (and I
don't understand the patch author's comments about this - why does
adding a new byte to the packet data require "recomposing" data?).

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.

There's no need to stuff that into packet flags instead.

> I think the packet flag solution is much saner than that.
Rostislav Pehlivanov May 18, 2018, 9:28 p.m. UTC | #12 
On 18 May 2018 at 22:17, wm4 <nfxjfg@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
Hendrik Leppkes May 18, 2018, 10:12 p.m. UTC | #13 
On Fri, May 18, 2018 at 11:17 PM, wm4 <nfxjfg@googlemail.com> wrote:
>
> Other flags are also generic. For a long time, AV_PKT_FLAG_KEY was the
> only flag. It applies to every codec, and is in theory not necessary
> for decoding (although some decoders might read it anyway, but that's
> a separate story).
>
> If we add flags about fields now, this will cause only confusion,
> because:
>
> [...]
> 2. They're suddenly needed for correct decoding.
>

That would be my main concern. I abstract the connection between
demuxer and decoder, and there is no field to transmit such mandatory
flags right now, while I do handle side-data.
So basically, this new flag would add additional work for everyone
doing something similar, while using side-data would likely "just
work".

- Hendrik
Patrick Keroulas May 22, 2018, 9:19 p.m. UTC | #14 
----- Original Message -----
> From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> To: "FFmpeg development discussions and patches" <ffmpeg-devel@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@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?

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 May 23, 2018, 3:18 p.m. UTC | #15 
On Tue, 22 May 2018 17:19:35 -0400 (EDT)
Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:

> ----- Original Message -----
> > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > To: "FFmpeg development discussions and patches" <ffmpeg-devel@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@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).
Rostislav Pehlivanov May 23, 2018, 3:46 p.m. UTC | #16 
On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:

> On Tue, 22 May 2018 17:19:35 -0400 (EDT)
> Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>
> > ----- Original Message -----
> > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > To: "FFmpeg development discussions and patches" <
> ffmpeg-devel@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@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@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.
wm4 May 23, 2018, 4:02 p.m. UTC | #17 
On Wed, 23 May 2018 16:46:17 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> >  
> > > ----- Original Message -----  
> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > > To: "FFmpeg development discussions and patches" <  
> > ffmpeg-devel@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@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@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.
Patrick Keroulas May 23, 2018, 6:29 p.m. UTC | #18 
----- Original Message -----
> From: "wm4" <nfxjfg@googlemail.com>
> To: ffmpeg-devel@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@gmail.com> wrote:
> 
>> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
>> 
>> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
>> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>> > 
>> > > ----- Original Message -----
>> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
>> > > > To: "FFmpeg development discussions and patches" <
>> > ffmpeg-devel@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@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@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?

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 May 23, 2018, 7:01 p.m. UTC | #19 
On Wed, 23 May 2018 14:29:38 -0400 (EDT)
Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:

> ----- Original Message -----
> > From: "wm4" <nfxjfg@googlemail.com>
> > To: ffmpeg-devel@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@gmail.com> wrote:
> >   
> >> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
> >>   
> >> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
> >> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> >> >   
> >> > > ----- Original Message -----  
> >> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> >> > > > To: "FFmpeg development discussions and patches" <  
> >> > ffmpeg-devel@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@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@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.
Rostislav Pehlivanov May 23, 2018, 7:25 p.m. UTC | #20 
On 23 May 2018 at 20:01, wm4 <nfxjfg@googlemail.com> wrote:

> On Wed, 23 May 2018 14:29:38 -0400 (EDT)
> Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>
> > ----- Original Message -----
> > > From: "wm4" <nfxjfg@googlemail.com>
> > > To: ffmpeg-devel@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@gmail.com> wrote:
> > >
> > >> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
> > >>
> > >> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
> > >> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > >> >
> > >> > > ----- Original Message -----
> > >> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > >> > > > To: "FFmpeg development discussions and patches" <
> > >> > ffmpeg-devel@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@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@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@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.
wm4 May 23, 2018, 7:49 p.m. UTC | #21 
On Wed, 23 May 2018 20:25:34 +0100
Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

> On 23 May 2018 at 20:01, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Wed, 23 May 2018 14:29:38 -0400 (EDT)
> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> >  
> > > ----- Original Message -----  
> > > > From: "wm4" <nfxjfg@googlemail.com>
> > > > To: ffmpeg-devel@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@gmail.com> wrote:
> > > >  
> > > >> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
> > > >>  
> > > >> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
> > > >> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > > >> >  
> > > >> > > ----- Original Message -----  
> > > >> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > >> > > > To: "FFmpeg development discussions and patches" <  
> > > >> > ffmpeg-devel@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@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@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@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.
Rostislav Pehlivanov May 23, 2018, 8:45 p.m. UTC | #22 
On 23 May 2018 at 20:49, wm4 <nfxjfg@googlemail.com> wrote:

> On Wed, 23 May 2018 20:25:34 +0100
> Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
>
> > On 23 May 2018 at 20:01, wm4 <nfxjfg@googlemail.com> wrote:
> >
> > > On Wed, 23 May 2018 14:29:38 -0400 (EDT)
> > > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > >
> > > > ----- Original Message -----
> > > > > From: "wm4" <nfxjfg@googlemail.com>
> > > > > To: ffmpeg-devel@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@gmail.com> wrote:
> > > > >
> > > > >> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
> > > > >>
> > > > >> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
> > > > >> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
> > > > >> >
> > > > >> > > ----- Original Message -----
> > > > >> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> > > > >> > > > To: "FFmpeg development discussions and patches" <
> > > > >> > ffmpeg-devel@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@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@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@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@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.
Patrick Keroulas May 23, 2018, 8:47 p.m. UTC | #23 
----- Original Message -----
> From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
> To: "FFmpeg development discussions and patches" <ffmpeg-devel@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@googlemail.com> wrote:
> 
>> On Wed, 23 May 2018 14:29:38 -0400 (EDT)
>> Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>> 
>> > ----- Original Message -----
>> > > From: "wm4" <nfxjfg@googlemail.com>
>> > > To: ffmpeg-devel@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@gmail.com> wrote:
>> > > 
>> > >> On 23 May 2018 at 16:18, wm4 <nfxjfg@googlemail.com> wrote:
>> > >> 
>> > >> > On Tue, 22 May 2018 17:19:35 -0400 (EDT)
>> > >> > Patrick Keroulas <patrick.keroulas@savoirfairelinux.com> wrote:
>> > >> > 
>> > >> > > ----- Original Message -----
>> > >> > > > From: "Rostislav Pehlivanov" <atomnuker@gmail.com>
>> > >> > > > To: "FFmpeg development discussions and patches" <
>> > >> > ffmpeg-devel@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@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@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@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.
diff mbox

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index bbefc83..d06868e 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2017-10-21
 
 API changes, most recent first:
 
+2018-05-xx - xxxxxxxxxx - lavc 58.20.100 - avcodec.h
+  Add AV_PKT_FLAG_TOP_FIELD and AV_PKT_FLAG_BOTTOM_FIELD.
+
 2018-05-xx - xxxxxxxxxx - lavu 56.18.101 - hwcontext_cuda.h
   Add AVCUDADeviceContext.stream.
 
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index fb0c6fa..14811be 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -1480,6 +1480,14 @@  typedef struct AVPacket {
  */
 #define AV_PKT_FLAG_DISPOSABLE 0x0010
 
+/**
+ * The packet contains a top field.
+ */
+#define AV_PKT_FLAG_TOP_FIELD  0x0020
+/**
+ * The packet contains a bottom field.
+ */
+#define AV_PKT_FLAG_BOTTOM_FIELD  0x0040
 
 enum AVSideDataParamChangeFlags {
     AV_SIDE_DATA_PARAM_CHANGE_CHANNEL_COUNT  = 0x0001,
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 3fda743..b9752ce 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -28,8 +28,8 @@ 
 #include "libavutil/version.h"
 
 #define LIBAVCODEC_VERSION_MAJOR  58
-#define LIBAVCODEC_VERSION_MINOR  19
-#define LIBAVCODEC_VERSION_MICRO 101
+#define LIBAVCODEC_VERSION_MINOR  20
+#define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \