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

[Matthieu Bouron]

On Thu, May 31, 2012 at 02:08:10PM +0200, Tomas Härdin wrote:
> 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.
> 

This ul is only used when mxfenc try to determine sc->index. It will
overrided when mxf_parse_dv_frame is called.

> > > +    // 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.

I didn't include those since i don't know much about IEC DV.

> 
> > > @@ -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.

Done.

> 
> 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}.
> 

Patch updated

> The rest looks fine.
> 
> /Tomas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-mxfenc-support-smpte-dv-codecs.patch
Type: text/x-diff
Size: 8657 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120531/99ae3f40/attachment.bin>