[FFmpeg-devel] [PATCH] avformat/mxfenc: support XAVC long gop

[Tomas Härdin]

fre 2019-03-29 klockan 08:30 -0700 skrev Baptiste Coudurier:
> Hey Tomas, I hope you are doing well
> 
> > > > On Mar 29, 2019, at 2:41 AM, Tomas Härdin <tjoppen at acc.umu.se> wrote:
> > 
> > tor 2019-03-28 klockan 08:50 -0700 skrev Baptiste Coudurier:
> > > ---
> > >  libavformat/mxf.h    |   1 +
> > >  libavformat/mxfenc.c | 194 ++++++++++++++++++++++++++++++++-----------
> > >  2 files changed, 145 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/libavformat/mxf.h b/libavformat/mxf.h
> > > index 4394450dea..f32124f772 100644
> > > --- a/libavformat/mxf.h
> > > +++ b/libavformat/mxf.h
> > > @@ -1317,6 +1330,13 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
> > >          avio_w8(pb, sc->field_dominance);
> > >      }
> > > 
> > > +    if (st->codecpar->codec_id == AV_CODEC_ID_H264 && !sc->avc_intra) {
> > > +        // write avc sub descriptor ref
> > > +        mxf_write_local_tag(pb, 8 + 16, 0x8100);
> > > +        mxf_write_refs_count(pb, 1);
> > > +        mxf_write_uuid(pb, AVCSubDescriptor, 0);
> > > +    }
> > 
> > Should this always be written for long gop AVC? Not just XAVC? Same
> > goes for mxf_write_avc_subdesc() I think, unless I'm reading this incorrectly..
> 
> The code will write always write it for AV_CODEC_ID_H264 except if it is avc_intra (it uses the legacy mpegvideo desc),
> so it will always be written for long gop as well. There is no check for XAVC technically.
> 
> > >  static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st)
> > > @@ -2136,30 +2196,30 @@ static const struct {
> > >      int frame_size;
> > >      int profile;
> > >      uint8_t interlaced;
> > > -    int long_gop; // 1 or 0 when there are separate UIDs for Long GOP and Intra, -1 when Intra/LGOP detection can be ignored
> > > +    int intra_only; // 1 or 0 when there are separate UIDs for Long GOP and Intra, -1 when Intra/LGOP detection can be ignored
> > 
> > Any particular reason for inverting this logic? Leaving it as is would
> > make the patch smaller..
> > 
> > /Tomas
> 
> Checking long gop looks weird to me since the specs don’t make that distinction, they do however make the explicit distinction for intra only
> profile, so it feels more intuitive to mark them intra only. No strong opinion though.

I guess that's fair. If I were feeling super-nitpicky I'd suggest
putting the flip in a separate patch

/Tomas