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

[wm4]

On Fri, 18 May 2018 20:09:02 +0100
Rostislav Pehlivanov <atomnuker at gmail.com> wrote:

> On 18 May 2018 at 20:05, 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: 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 at googlemail.com> wrote:
> > >  
> > >> On Tue, 15 May 2018 17:15:05 +0100
> > >> Rostislav Pehlivanov <atomnuker at gmail.com> wrote:
> > >>  
> > >> > On 15 May 2018 at 15:55, wm4 <nfxjfg at googlemail.com> wrote:
> > >> >  
> > >> > > On Mon, 14 May 2018 18:26:35 -0400
> > >> > > Patrick Keroulas <patrick.keroulas at 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 at 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 at ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel  
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at 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?