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

[Marton Balint]

On Tue, 3 Mar 2020, Baptiste Coudurier wrote:

>> On Mar 3, 2020, at 11:37 AM, Tomas Härdin <tjoppen at 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 at 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 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).
>>> 
>>> 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