[FFmpeg-devel] [PATCH 4/4] mxfenc: support smpte dv codecs

[Tomas Härdin]

On Tue, 2012-05-29 at 12:23 +0200, Matthieu Bouron wrote:
> > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > index fce8446..0c39acb 100644
> > --- a/libavformat/mxfenc.c
> > +++ b/libavformat/mxfenc.c
> > @@ -93,6 +93,7 @@ static const struct {
> >      { CODEC_ID_MPEG2VIDEO, 0 },
> >      { CODEC_ID_PCM_S24LE,  1 },
> >      { CODEC_ID_PCM_S16LE,  1 },
> > +    { CODEC_ID_DVVIDEO,   15 },
> >      { CODEC_ID_NONE }
> >  };
> >  
> > @@ -169,6 +170,51 @@ static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
> >        { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x06,0x01,0x10,0x00 },
> >        { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x02,0x02,0x01,0x00,0x00,0x00,0x00 },
> >        mxf_write_generic_sound_desc },
> > +    // DV Unknwon 15
> > +    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x02,0x7F,0x01 },
> > +      { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x18,0x01,0x01,0x00 },
> > +      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x02,0x00,0x00,0x00 },
> > +      mxf_write_cdci_desc },

Using this UL and the mxf_essence_mappings entry seems suspicious - we
should write proper ones instead of guessing so we don't output bad
files.
I suspect the intent is to support DVCPRO HD, but that has its own ULs.

> > +    // DV25 525/60
> > +    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x02,0x40,0x01 },
> > +      { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x18,0x01,0x01,0x00 },
> > +      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x02,0x02,0x01,0x00 },
> > +      mxf_write_cdci_desc },
> >-- snip --
> > +    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x02,0x63,0x01 },
> > +      { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x18,0x01,0x01,0x00 },
> > +      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x02,0x02,0x02,0x02,0x08,0x00 },
> > +      mxf_write_cdci_desc },

I'm noticing five ULs in S383m missing here (first value = byte 15 of
UL):
01 IEC DV, 25Mbps 525-I 
02 IEC DV, 25Mbps 625-I 
03 IEC DV, 25Mbps 525-lines, 
   Registered Quality: SMPTE 322M Compatible 
04 IEC DV, 25Mbps 625-lines, 
   Registered Quality: SMPTE 322M Compatible 
3F IEC DV, undefined 

I don't think it matters much - just noting it.

> > @@ -1292,6 +1338,70 @@ static void mxf_write_partition(AVFormatContext *s, int bodysid,
> >      avio_flush(pb);
> >  }
> >  
> > +static int mxf_parse_dv_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)

Shouldn't some kind of parser do what this function does? I see this
kind of stuff for MPEG-2 all over lavf and it bothers me.

> > +{
> > +    MXFContext *mxf = s->priv_data;
> > +    MXFStreamContext *sc = st->priv_data;
> > +    uint8_t *vs_pack = pkt->data + 80*5 + 48;
> > +    uint8_t *vsc_pack = pkt->data + 80*5 + 53;

Check pkt->size before doing anything with these.

The rest of the function looks OK.

> >  static const UID mxf_mpeg2_codec_uls[] = {
> >      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x03,0x04,0x01,0x02,0x02,0x01,0x01,0x10,0x00 }, // MP-ML I-Frame
> >      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x03,0x04,0x01,0x02,0x02,0x01,0x01,0x11,0x00 }, // MP-ML Long GOP
> > @@ -1536,7 +1646,10 @@ static int mxf_write_header(AVFormatContext *s)
> >          MXFStreamContext *sc = s->streams[i]->priv_data;
> >          // update element count
> >          sc->track_essence_element_key[13] = present[sc->index];
> > -        sc->order = AV_RB32(sc->track_essence_element_key+12);
> > +        if (sc->track_essence_element_key[12] == 0x18) // DV

This could be a problem if in the future if we add other non-DV essence
element keys with that byte == 0x18. Consider checking it against
{0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x18}.

The rest looks fine.

/Tomas