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

[Tomas Härdin]

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.

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

/Tomas