[FFmpeg-devel] [PATCH 11/17] avformat/mxfenc: Store locally whether DNXHD profile is interlaced

[Tomas Härdin]

tis 2021-11-09 klockan 22:48 +0100 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > tis 2021-11-09 klockan 19:01 +0100 skrev Andreas Rheinhardt:
> > > It is just a flag per supported CID. So there is no reason to use
> > > an avpriv function for this purpose.
> > 
> > This is data duplication. Honestly these ULs should probably live
> > in
> > dnxhddata.c.
> > 
> 
> But aren't these ULs mxf-specific? So the right place for them is
> here
> in libavformat.
> And the amount of data duplicated is trivial; furthermore adding a
> new
> DNXHD profile then and now requires modifications in two tables, so I
> don't see a maintenance burden either.

Right, this improves lavc/lavf separation. I suppose that's OK. Still
not the biggest fan of having this kind of data in more than one place,
but on the other hand it's constants..

> > > 
> > > Signed-off-by: Andreas Rheinhardt
> > > <andreas.rheinhardt at outlook.com>
> > > ---
> > >  libavformat/mxfenc.c | 47 ++++++++++++++++++++++----------------
> > > ----
> > > --
> > >  1 file changed, 23 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index aa9857fcff..326ec6a7d6 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -2036,29 +2036,30 @@ static int
> > > mxf_parse_prores_frame(AVFormatContext *s, AVStream *st, AVPacket
> > > *pk
> > >  }
> > >  
> > >  static const struct {
> > > -    int cid;
> > > +    uint16_t cid;
> > > +    uint8_t  interlaced;
> > >      UID codec_ul;
> > >  } mxf_dnxhd_codec_uls[] = {
> > 
> > Not sure if the narrowing of types here does any good. You might
> > need
> > to put cid and interlaced after codec_ul. On the other hand UID is
> > uint8_t[] so perhaps it works out.
> > 
> The narrowing is irrelevant, as all cid values in use fit into an
> uint16_t.
> Why would I need to put it at the end? Do you worry about padding
> between interlaced and codec_ul? In this case it doesn't matter: It
> would just be a matter of whether the padding is in the middle or at
> the
> end of the structure.

I don't actually care. I just thought it was curious given patch 04

/Tomas