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

[Michael Niedermayer]

On Tue, Mar 03, 2020 at 07:54:55PM +0100, Tomas Härdin wrote:
> 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.

Do you have a testcase for this so the ratecontrol can be fixed by fixing
the testcase ?

Thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200304/1e4c973b/attachment.sig>