[FFmpeg-devel] [PATCH 1/2] mxfenc: factorize samples per frame code

[Matthieu Bouron]

On Mon, Sep 17, 2012 at 11:14 AM, Tomas Härdin <tomas.hardin at codemill.se>wrote:

> On Thu, 2012-09-13 at 11:09 +0100, Kieran Kunhya wrote:
> > On Thu, Sep 13, 2012 at 12:52 AM, Ben Jackson <ben at ben.com> wrote:
> > > On Thu, Sep 13, 2012 at 01:22:02AM +0200, Matthieu Bouron wrote:
> > >> On Thu, Sep 13, 2012 at 1:09 AM, Kieran Kunhya <
> kierank at ob-encoder.com>wrote:
> > >>
> > >> > > +    { { 1001, 60000 }, { 801,  801,  801,  801,  800,  0 } }, //
> NTSC
> > >> > 59.94
> > >> >
> > >> > I thought the pattern was:
> > >> > {  801,  800,  801,  801,  801 }
> > >> >
> > >> > I've never found a spec for this though - found it in a document
> > >> > somewhere so you might be right.
> > >
> > > The particular cadence doesn't matter.  It just needs to average 800.8
> > > over 10 fields.  At a frame level it typically does not diverge beyond
> > > 1600 to 1602 (although obviously 1601 and 1602 are all that are
> strictly
> > > necessary).
> > >
> > > I might be able to dig up the relevant SMPTE spec but it's not so
> specific
> > > as to give a pattern.
> >
> > It's more of a question as to whether tools on the market will
> > complain/crash/cause the apocalypse unless you use a pattern that it
> > likes.
>
> The pattern is important for sample accurate seeking - see "[PATCH]
> mxfdec: set audio packet pts". This reminds me: this table needs to be
> shared between muxer and demuxer so lavf works with itself. That can be
> done in a later patch though.
>

Ok, i'll correct the pattern in another patch.

Yes, mxf_get_samples_per_frame function needs to be exported to lavf.
This will help setting the audio packets pts in mxfdec and hopefully fix
the ntsc playback of mxf files :)



>
> > +typedef struct {
> > +    struct AVRational time_base;
> > +    int samples_per_frame[6];
> > +} MXFSamplesPerFrame;
> > +
> > +static const MXFSamplesPerFrame mxf_samples_per_frames[] = {
> >
> Not very important, but you could save a few lines by not naming the
> struct:
>
> > +static const struct {
> > +    struct AVRational time_base;
> > +    int samples_per_frame[6];
> > +} mxf_samples_per_frames[] = {
>
> The patch looks OK though.
>

Thanks for the review.

Matthieu