[FFmpeg-devel] [PATCH 2/4] avformat/mxf: get rid of samples per frame array usage

[Tomas Härdin]

mån 2020-03-02 klockan 21:35 +0100 skrev Marton Balint:
> 
> On Mon, 2 Mar 2020, Tomas Härdin wrote:
> 
> > fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> > > Signed-off-by: Marton Balint <cus at passwd.hu>
> > > ---
> > >  libavformat/mxf.c    | 44 ++++----------------------------------------
> > >  libavformat/mxf.h    |  6 ------
> > >  libavformat/mxfdec.c | 23 +++--------------------
> > >  libavformat/mxfenc.c | 24 ++++++------------------
> > >  4 files changed, 13 insertions(+), 84 deletions(-)
> > >  int ff_mxf_get_content_package_rate(AVRational time_base)
> > >  {
> > > -    int idx = av_find_nearest_q_idx(time_base, mxf_time_base);
> > > -    AVRational diff = av_sub_q(time_base, mxf_time_base[idx]);
> > > -
> > > -    diff.num = FFABS(diff.num);
> > > -
> > > -    if (av_cmp_q(diff, (AVRational){1, 1000}) >= 0)
> > > -        return -1;
> > > -
> > > -    return mxf_content_package_rates[idx];
> > > +    for (int i = 0; mxf_time_base[i].num; i++)
> > > +        if (!av_cmp_q(time_base, mxf_time_base[i]))
> > 
> > I see this gets rid of the inexact check for an exact one. Approve!
> > 
> > > @@ -3307,20 +3307,17 @@ static int mxf_get_next_track_edit_unit(MXFContext *mxf, MXFTrack *track, int64_
> > >  static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
> > >                                          int64_t edit_unit)
> > >  {
> > > -    int i, total = 0, size = 0;
> > >      MXFTrack *track = st->priv_data;
> > >      AVRational time_base = av_inv_q(track->edit_rate);
> > >      AVRational sample_rate = av_inv_q(st->time_base);
> > > -    const MXFSamplesPerFrame *spf = NULL;
> > > -    int64_t sample_count;
> > > 
> > >      // For non-audio sample_count equals current edit unit
> > >      if (st->codecpar->codec_type != AVMEDIA_TYPE_AUDIO)
> > >          return edit_unit;
> > > 
> > > -    if ((sample_rate.num / sample_rate.den) == 48000)
> > > -        spf = ff_mxf_get_samples_per_frame(mxf->fc, time_base);
> > > -    if (!spf) {
> > > +    if ((sample_rate.num / sample_rate.den) == 48000) {
> > > +        return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
> > 
> > Should be OK, per previous rounding argument
> > 
> > >                  }
> > >                  sc->index = INDEX_D10_AUDIO;
> > >                  sc->container_ul = ((MXFStreamContext*)s->streams[0]->priv_data)->container_ul;
> > > -                sc->frame_size = 4 + 8 * spf[0].samples_per_frame[0] * 4;
> > > +                sc->frame_size = 4 + 8 * av_rescale_rnd(st-
> > > >codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den,
> > > AV_ROUND_UP) * 4;
> > 
> > I was going to say this is only used for CBR video, but closer
> > inspection reveals it's used to prevent 1/1.001 rate audio packets from
> > making their way into CBR files. This is a bit surprising to me, since
> > D-10 supports NTSC (without audio?)
> 
> I thought D10 can only be CBR and and it can only use a constant edit unit 
> size, 1/1.001 audio packet size difference is handled using KLV 
> padding. So what we compute here is a _maximum_ frame size.

Now I remember: D-10 is indeed CBR, but at the MXF/CP level. To make it
CBR you must pad the content packages with fill KLVs as necessary. This
filling was actually removed by Baptiste a while back, and we had a set
of patches attempting to fix support for NTSC but sadly the ratecontrol
in lavc is not up to snuff to support that for files longer than about
an hour. This also means the video essence needs to be coded at a
bitrate slightly lower than the D-10 rate, say 49.9 Mbps for 50 Mbps D-
10 (+-audio).

I believe at some point there was code for padding the MPEG-2 essence
with zeroes to make CBR, but that obviously doesn't work with audio due
to 1601.6 samples per EditUnit

/Tomas