[FFmpeg-devel] [PATCH] fix MXF audio PTS calculation for compressed audio (ADTS/AAC)

[Marton Balint]

On Tue, 4 Sep 2018, Markus Schumann wrote:

> ---
> libavformat/mxfdec.c | 67 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 66 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 8e1089620f..adfffd954a
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -3266,9 +3266,63 @@ static int64_t mxf_set_current_edit_unit(MXFContext *mxf, AVStream *st, int64_t
>     return mxf_set_current_edit_unit(mxf, st, current_offset, 0);
> }
>
> +static int mxf_aac_adts_count_frames(MXFContext *mxf, AVCodecParameters *par,
> +                                     AVPacket *pkt)
> +{
> +    const uint8_t *end_ptr;
> +    uint8_t *data_ptr;
> +    int32_t aac_frame_length, adts_frame_count;
> +    uint8_t sample_rate_index;
> +    uint8_t channel_configuration;
> +
> +    data_ptr = pkt->data;
> +    end_ptr = pkt->data + pkt->size;
> +       adts_frame_count = 0;
> +
> +       /* ADTS header is 7 bytes */
> +       if (pkt->size < 7)
> +               return 0;
> +
> +       /* check for sync bits 0xfff */
> +       if (data_ptr[0] != 0xff || (data_ptr[1] & 0xf0) != 0xf0) return AVERROR(EINVAL);
> +
> +       sample_rate_index = (data_ptr[2] & 0x3c) >> 2;
> +
> +       channel_configuration = ((data_ptr[2] & 0x01) << 2) | ((data_ptr[3] & 0xc0) >> 6);
> +
> +       adts_frame_count = 1;
> +
> +       data_ptr += 7;
> +
> +       for (; data_ptr + 7 < end_ptr; ++data_ptr)
> +       {
> +               /* check for sync bits 0xfff */
> +               if (data_ptr[0] != 0xff || (data_ptr[1] & 0xf0) != 0xf0) continue;
> +
> +               /* make sure sample rate is identical */
> +               if (sample_rate_index != (data_ptr[2] & 0x3c) >> 2) continue;
> +
> +               /* make sure channel configuration is identical */
> +               if (channel_configuration != (((data_ptr[2] & 0x01) << 2) | ((data_ptr[3] & 0xc0) >> 6))) continue;
> +
> +               aac_frame_length = ((int32_t)(data_ptr[3] & 0x03) << 11) | ((int32_t)data_ptr[4] << 3) | ((int32_t)(data_ptr[5] & 0xe0) >> 5);
> +
> +               /* sanity check on the frame length */
> +               if (aac_frame_length < 1 || aac_frame_length > 8 * 768 + 7 + 2) continue;
> +
> +               ++adts_frame_count;
> +
> +               data_ptr += 7;
> +       }
> +
> +       return adts_frame_count;
> +}
> +
> static int mxf_set_audio_pts(MXFContext *mxf, AVCodecParameters *par,
>                              AVPacket *pkt)
> {
> +    int frame_count;
> +
>     MXFTrack *track = mxf->fc->streams[pkt->stream_index]->priv_data;
>     int64_t bits_per_sample = par->bits_per_coded_sample;
>
> @@ -3281,7 +3335,18 @@ static int mxf_set_audio_pts(MXFContext *mxf, AVCodecParameters *par,
>         || bits_per_sample <= 0
>         || par->channels * (int64_t)bits_per_sample < 8)
>         return AVERROR(EINVAL);
> -    track->sample_count += pkt->size / (par->channels * (int64_t)bits_per_sample / 8);
> +
> +    switch (par->codec_id) {
> +    case AV_CODEC_ID_AAC:
> +        frame_count =  mxf_aac_adts_count_frames(mxf, par, pkt);
> +        if (frame_count < 0) return AVERROR(EINVAL);
> +        track->sample_count += 1024 * frame_count;
> +       break;
> +    default:
> +       /* here we assume PCM */
> +       track->sample_count += pkt->size / (par->channels * (int64_t)bits_per_sample / 8);
> +       break;
> +    }
>     return 0;

I just pushed a patch which fixes the error messages decoding 
frame wrapped (as in 25fps frame wrapped) AAC packets. Obviously audio 
packet pts-es will not be sample correct, but they should be accurate 
enough for average use cases. (and it is not uncommon that demuxers are 
unable to provide sample accurate timestamps).

In general it is not a good idea to put codec parsing code in a demuxer, 
we have parsers for that. So an alternative approach might be to return 
AV_NOPTS_VALUE from the mxf demuxer and let the aac parser fill in the 
timestamps, but in this case, since we can determine a rough timestamp 
based on the format, I think it is better if we return that, because that 
is what the other demuxers are doing as well.

Is there a use case which does not work for you using the current git 
master?

Thanks,
Marton