diff mbox series

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

Message ID 20200228003750.22536-2-cus@passwd.hu
State Superseded
Headers show
Series [FFmpeg-devel,1/4] avformat/audiointerleave: disallow using a samples_per_frame array
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Marton Balint Feb. 28, 2020, 12:37 a.m. UTC
Signed-off-by: Marton Balint <cus@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(-)

Comments

Tomas Härdin March 2, 2020, 5:49 p.m. UTC | #1
fre 2020-02-28 klockan 01:37 +0100 skrev Marton Balint:
> Signed-off-by: Marton Balint <cus@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?)

>                  sc->index = INDEX_WAV;
>              } else {
>                  mxf->slice_count = 1;
> -                sc->frame_size = (st->codecpar->channels * spf[0].samples_per_frame[0] *
> -                                  av_get_bits_per_sample(st->codecpar->codec_id)) / 8;
> +                sc->frame_size = st->codecpar->channels *
> +                                 av_rescale_rnd(st->codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den, AV_ROUND_UP) *
> +                                 av_get_bits_per_sample(st->codecpar->codec_id) / 8;

Looks similarly OK

/Tomas
Marton Balint March 2, 2020, 8:35 p.m. UTC | #2
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@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.

Regards,
Marton

>
>>                  sc->index = INDEX_WAV;
>>              } else {
>>                  mxf->slice_count = 1;
>> -                sc->frame_size = (st->codecpar->channels * spf[0].samples_per_frame[0] *
>> -                                  av_get_bits_per_sample(st->codecpar->codec_id)) / 8;
>> +                sc->frame_size = st->codecpar->channels *
>> +                                 av_rescale_rnd(st->codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den, AV_ROUND_UP) *
>> +                                 av_get_bits_per_sample(st->codecpar->codec_id) / 8;
>
> Looks similarly OK
>
> /Tomas
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Tomas Härdin March 3, 2020, 6:54 p.m. UTC | #3
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@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
Baptiste Coudurier March 3, 2020, 7:22 p.m. UTC | #4
Hey guys,


> On Mar 3, 2020, at 10:54 AM, Tomas Härdin <tjoppen@acc.umu.se> 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@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).

IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed, 
so the essence needs to be CBR at the essence (mpeg-2) level.
Anything else is just broken IMHO.

> 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

Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames, 
Audio and video durations match perfectly.

— 
Baptiste
Tomas Härdin March 3, 2020, 7:37 p.m. UTC | #5
tis 2020-03-03 klockan 11:22 -0800 skrev Baptiste Coudurier:
> Hey guys,
> 
> 
> > On Mar 3, 2020, at 10:54 AM, Tomas Härdin <tjoppen@acc.umu.se> 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@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).
> 
> IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed, 
> so the essence needs to be CBR at the essence (mpeg-2) level.
> Anything else is just broken IMHO.

For practical D-10 decoders probably, but I think the spec is a bit
more lax. You only need to set up the DeltaEntry array in the
IndexTableSegment correctly

> > 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
> 
> Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames, 
> Audio and video durations match perfectly.

Yep, which luckily this patchset does. For audio there definitely needs
to be KLV fill since the size of the audio packets will vary

/Tomas
Baptiste Coudurier March 3, 2020, 10:24 p.m. UTC | #6
> On Mar 3, 2020, at 11:37 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
> tis 2020-03-03 klockan 11:22 -0800 skrev Baptiste Coudurier:
>> Hey guys,
>> 
>> 
>>> On Mar 3, 2020, at 10:54 AM, Tomas Härdin <tjoppen@acc.umu.se> 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@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).
>> 
>> IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed, 
>> so the essence needs to be CBR at the essence (mpeg-2) level.
>> Anything else is just broken IMHO.
> 
> For practical D-10 decoders probably, but I think the spec is a bit
> more lax. You only need to set up the DeltaEntry array in the
> IndexTableSegment correctly
> 
>>> 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
>> 
>> Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames, 
>> Audio and video durations match perfectly.
> 
> Yep, which luckily this patchset does. For audio there definitely needs
> to be KLV fill since the size of the audio packets will vary

Wait, is it what it does ? The pattern is very strict and ordered.
For 30000/1001 and 60000/1001, it seems to me that it would not be correct.

— 
Baptiste
Marton Balint March 4, 2020, 8:14 a.m. UTC | #7
On Tue, 3 Mar 2020, Baptiste Coudurier wrote:

>> On Mar 3, 2020, at 11:37 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
>> 
>> tis 2020-03-03 klockan 11:22 -0800 skrev Baptiste Coudurier:
>>> Hey guys,
>>> 
>>> 
>>>> On Mar 3, 2020, at 10:54 AM, Tomas Härdin <tjoppen@acc.umu.se> 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@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).
>>> 
>>> IIRC, and correct me if I’m wrong, the VBV delay in D-10 is actually fixed, 
>>> so the essence needs to be CBR at the essence (mpeg-2) level.
>>> Anything else is just broken IMHO.
>> 
>> For practical D-10 decoders probably, but I think the spec is a bit
>> more lax. You only need to set up the DeltaEntry array in the
>> IndexTableSegment correctly
>> 
>>>> 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
>>> 
>>> Yeah, the pattern of audio samples is very strict in MXF, and every 5 frames, 
>>> Audio and video durations match perfectly.
>> 
>> Yep, which luckily this patchset does. For audio there definitely needs
>> to be KLV fill since the size of the audio packets will vary
>
> Wait, is it what it does ? The pattern is very strict and ordered.
> For 30000/1001 and 60000/1001, it seems to me that it would not be correct.

The patchset replaces the hard coded sample tables with mathematically 
computed values, because

samples_in_nth_frame = av_rescale_q(n+1, {48000,1}, frame_rate) -
                        av_rescale_q(n,   {48000,1}, frame_rate);

And this is also true for 30000/1001 and 60000/1001 frame rates.

Regards,
Marton
Michael Niedermayer March 4, 2020, 6:58 p.m. UTC | #8
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@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

[...]
Tomas Härdin March 7, 2020, 11:22 a.m. UTC | #9
ons 2020-03-04 klockan 19:58 +0100 skrev 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@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 ?

See the thread "[FFmpeg-devel] [PATCH] libavformat/mxfenc: Allow more
bitrates for NTSC IMX50" in the archives. The basic problem is that it
is not possible to ask the ratecontrol system to produce packets with a
specified size. It is only possible to use bitrate, which never cleanly
divides by 30/1.001 for IMX

/Tomas
diff mbox series

Patch

diff --git a/libavformat/mxf.c b/libavformat/mxf.c
index 451cbcfb2c..10ccd770e3 100644
--- a/libavformat/mxf.c
+++ b/libavformat/mxf.c
@@ -131,16 +131,6 @@  int ff_mxf_decode_pixel_layout(const char pixel_layout[16], enum AVPixelFormat *
     return -1;
 }
 
-static const MXFSamplesPerFrame mxf_spf[] = {
-    { { 1001, 24000 }, { 2002, 0,    0,    0,    0,    0 } }, // FILM 23.976
-    { { 1, 24},        { 2000, 0,    0,    0,    0,    0 } }, // FILM 24
-    { { 1001, 30000 }, { 1602, 1601, 1602, 1601, 1602, 0 } }, // NTSC 29.97
-    { { 1001, 60000 }, { 801,  801,  800,  801,  801,  0 } }, // NTSC 59.94
-    { { 1, 25 },       { 1920, 0,    0,    0,    0,    0 } }, // PAL 25
-    { { 1, 50 },       { 960,  0,    0,    0,    0,    0 } }, // PAL 50
-    { { 1, 60 },       { 800,  0,    0,    0,    0,    0 } },
-};
-
 static const AVRational mxf_time_base[] = {
     { 1001, 24000 },
     { 1, 24},
@@ -152,40 +142,14 @@  static const AVRational mxf_time_base[] = {
     { 0, 0}
 };
 
-const MXFSamplesPerFrame *ff_mxf_get_samples_per_frame(AVFormatContext *s,
-                                                       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 NULL;
-
-    if (av_cmp_q(time_base, mxf_time_base[idx]))
-        av_log(s, AV_LOG_WARNING,
-               "%d/%d input time base matched %d/%d container time base\n",
-               time_base.num, time_base.den,
-               mxf_spf[idx].time_base.num,
-               mxf_spf[idx].time_base.den);
-
-    return &mxf_spf[idx];
-}
-
 static const int mxf_content_package_rates[] = {
     3, 2, 7, 13, 4, 10, 12,
 };
 
 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]))
+            return mxf_content_package_rates[i];
+    return 0;
 }
diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index f32124f772..2669269830 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -82,18 +82,12 @@  typedef struct MXFCodecUL {
     MXFWrappingIndicatorType wrapping_indicator_type;
 } MXFCodecUL;
 
-typedef struct {
-    struct AVRational time_base;
-    int samples_per_frame[6];
-} MXFSamplesPerFrame;
-
 extern const MXFCodecUL ff_mxf_data_definition_uls[];
 extern const MXFCodecUL ff_mxf_codec_uls[];
 extern const MXFCodecUL ff_mxf_pixel_format_uls[];
 extern const MXFCodecUL ff_mxf_codec_tag_uls[];
 
 int ff_mxf_decode_pixel_layout(const char pixel_layout[16], enum AVPixelFormat *pix_fmt);
-const MXFSamplesPerFrame *ff_mxf_get_samples_per_frame(AVFormatContext *s, AVRational time_base);
 int ff_mxf_get_content_package_rate(AVRational time_base);
 
 
diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 9a48e2d2d1..9113e2a09c 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -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);
+    } else {
         int remainder = (sample_rate.num * time_base.num) %
                         (time_base.den * sample_rate.den);
         if (remainder)
@@ -3331,20 +3328,6 @@  static int64_t mxf_compute_sample_count(MXFContext *mxf, AVStream *st,
                    sample_rate.num, sample_rate.den);
         return av_rescale_q(edit_unit, sample_rate, track->edit_rate);
     }
-
-    while (spf->samples_per_frame[size]) {
-        total += spf->samples_per_frame[size];
-        size++;
-    }
-
-    av_assert2(size);
-
-    sample_count = (edit_unit / size) * (uint64_t)total;
-    for (i = 0; i < edit_unit % size; i++) {
-        sample_count += spf->samples_per_frame[i];
-    }
-
-    return sample_count;
 }
 
 /**
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index d77f993947..1d8ad57415 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2425,7 +2425,6 @@  static int mxf_write_header(AVFormatContext *s)
     MXFContext *mxf = s->priv_data;
     int i, ret;
     uint8_t present[FF_ARRAY_ELEMS(mxf_essence_container_uls)] = {0};
-    const MXFSamplesPerFrame *spf = NULL;
     int64_t timestamp = 0;
 
     if (!s->nb_streams)
@@ -2480,14 +2479,8 @@  static int mxf_write_header(AVFormatContext *s)
             }
 
             mxf->timecode_base = (tbc.den + tbc.num/2) / tbc.num;
-            spf = ff_mxf_get_samples_per_frame(s, tbc);
-            if (!spf) {
-                av_log(s, AV_LOG_ERROR, "Unsupported video frame rate %d/%d\n",
-                       tbc.den, tbc.num);
-                return AVERROR(EINVAL);
-            }
             mxf->content_package_rate = ff_mxf_get_content_package_rate(tbc);
-            mxf->time_base = spf->time_base;
+            mxf->time_base = tbc;
             rate = av_inv_q(mxf->time_base);
             avpriv_set_pts_info(st, 64, mxf->time_base.num, mxf->time_base.den);
             if((ret = mxf_init_timecode(s, st, rate)) < 0)
@@ -2552,7 +2545,7 @@  static int mxf_write_header(AVFormatContext *s)
                 }
                 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;
             } else if (s->oformat == &ff_mxf_opatom_muxer) {
                 AVRational tbc = av_inv_q(mxf->audio_edit_rate);
 
@@ -2566,14 +2559,8 @@  static int mxf_write_header(AVFormatContext *s)
                     return AVERROR(EINVAL);
                 }
 
-                spf = ff_mxf_get_samples_per_frame(s, tbc);
-                if (!spf) {
-                    av_log(s, AV_LOG_ERROR, "Unsupported timecode frame rate %d/%d\n", tbc.den, tbc.num);
-                    return AVERROR(EINVAL);
-                }
-
                 mxf->time_base = st->time_base;
-                if((ret = mxf_init_timecode(s, st, av_inv_q(spf->time_base))) < 0)
+                if((ret = mxf_init_timecode(s, st, av_inv_q(tbc))) < 0)
                     return ret;
 
                 mxf->timecode_base = (tbc.den + tbc.num/2) / tbc.num;
@@ -2581,8 +2568,9 @@  static int mxf_write_header(AVFormatContext *s)
                 sc->index = INDEX_WAV;
             } else {
                 mxf->slice_count = 1;
-                sc->frame_size = (st->codecpar->channels * spf[0].samples_per_frame[0] *
-                                  av_get_bits_per_sample(st->codecpar->codec_id)) / 8;
+                sc->frame_size = st->codecpar->channels *
+                                 av_rescale_rnd(st->codecpar->sample_rate, mxf->time_base.num, mxf->time_base.den, AV_ROUND_UP) *
+                                 av_get_bits_per_sample(st->codecpar->codec_id) / 8;
             }
         } else if (st->codecpar->codec_type == AVMEDIA_TYPE_DATA) {
             AVDictionaryEntry *e = av_dict_get(st->metadata, "data_type", NULL, 0);