diff mbox

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

Message ID DM3PR16MB10503642BAF794588D16874DE2030@DM3PR16MB1050.namprd16.prod.outlook.com
State New
Headers show

Commit Message

Markus Schumann Sept. 4, 2018, 11:25 p.m. UTC 
---
 libavformat/mxfdec.c | 67 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

--
2.17.1

Comments

Marton Balint Sept. 5, 2018, 9:06 p.m. UTC | #1 
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
Markus Schumann Sept. 5, 2018, 9:37 p.m. UTC | #2 
> On Sep 5, 2018, at 16:06, Marton Balint <cus@passwd.hu> wrote:
> 
> 
> 
>> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

I just built FFmpeg master with your changes and then VLC using FFmpeg master. 

My sample from ticket #7366 plays fine in VLC with your changes.

So your changes should be good enough.

Thanks Markus.
diff mbox

Patch

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;
 }