[FFmpeg-devel] [PATCH] check sampling index validity when parsing adts frame header

[Robert Swain]

2009/2/10 Robert Swain <robert.swain at gmail.com>:
> 2009/2/10 Alex Converse <alex.converse at gmail.com>:
>> On Tue, Feb 10, 2009 at 8:48 AM, Robert Swain <robert.swain at gmail.com> wrote:
>>> 2009/2/9 Robert Swain <robert.swain at gmail.com>:
>>>> 2009/2/9 Jai Menon <jmenon86 at gmail.com>:
>>>>> On Mon, Feb 9, 2009 at 1:51 PM, Baptiste Coudurier
>>>>> <baptiste.coudurier at gmail.com> wrote:
>>>>>> Jai Menon wrote:
>>>>>>> On Mon, Feb 9, 2009 at 12:41 PM, Alex Converse <alex.converse at gmail.com> wrote:
>>>>>>>>> Index: libavcodec/aac.c
>>>>>>>>> ===================================================================
>>>>>>>>> --- libavcodec/aac.c  (revision 16998)
>>>>>>>>> +++ libavcodec/aac.c  (working copy)
>>>>>>>>> @@ -1529,6 +1529,11 @@
>>>>>>>>>          if (hdr_info.chan_config)
>>>>>>>>>              ac->m4ac.chan_config = hdr_info.chan_config;
>>>>>>>>>          ac->m4ac.sample_rate     = hdr_info.sample_rate;
>>>>>>>>> +
>>>>>>>>> +        if(hdr_info.sampling_index > 11) {
>>>>>>>> 12 is a valid sampling index. It corresponds to 7350 Hz.
>>>>>>>
>>>>>>> Shouldn't it be in the table then?
>>>>>>
>>>>>> Isn't it ? check mpeg4audio.c
>>>>>
>>>>> the table I referred to is swb_offset_1024 in aacdectab.h
>>>>
>>>> I'll have to check the spec for this. There are other tables which are
>>>> indexed by sampling_index ( swb_offset, ff_aac_num_swb, tns_max_bands,
>>>> ff_aac_pred_sfb_max as well as ff_mpeg4audio_sample_rates ). If these
>>>> other tables only support up to index 11 in the spec then that's what
>>>> we shall have to check. If they should be extended and for whatever
>>>> reason they were truncated, we'll extend them.
>>>
>>> From the spec, it seems that a sample rate of 7350Hz (sampling_index
>>> == 12) is not supported in AAC. At least, none of the above mentioned
>>> tables contain entries for 7350Hz, only for 8kHz and above. In which
>>> case, I consider the checks valid.
>>>
>>
>> "Table 4.68 - Sampling frequency mapping" (1496-3:2005) says that for
>> f<9391 the tables for 8000 Hz should be used.
>
> Hmm. Indeed, this may need a bit more work.
>
> Looking at 13818-7, the ADTS headers do not specify a sample rate,
> only a sampling_frequency_index. "Table 35 ? Sampling frequency
> dependent on sampling_frequency_index" only covers indices mapping
> down to 8kHz, the rest of the values are reserved. When using ADTS, it
> seems the limitation of indices >11 being 'reserved' (and hence should
> cause an error if used) should be imposed.
>
> Looking at 14496-3, libavcodec/mpeg4audio.c gets it right in
> get_sample_rate() as it parses the sample rate in the case of the
> sampling_frequency_index == 15. However, indices 13 and 14 are still
> reserved so we should deal with this.
>
> If sampling_frequency_index == 15 and a sample rate has been
> specified, we should map this sample rate according to "Table 4.68 -
> Sampling frequency mapping" as you noted. I'm inclined to suggest
> reusing the m4ac.sampling_index variable for the result of the mapping
> but we should check if this would have any adverse side effects.

ADTS also being informatively defined in 14496-3 states the following
for sampling_frequency_index:

"Indicates the sampling frequency used according to the table defined in
subclause 1.6.3.4. The escape value is not permitted."

Which points to the mapping table containing 7350Hz for index 12.

As such, I suspect the following should be true:
- Sampling indices <= 12 are fine across ADIF, ADTS and MP4 (and others).
- A sampling index of 15 is not usable as an escape in ADTS as the
ADTS headers do not contain an option for an explicit sampling rate.
- A sampling index of 12 (7350Hz) should use the tables for 8kHz.

Alex has queried this on mp4-tech. I intend to alter the code
accordingly now and have a patch waiting. If we do not receive a
response before mid-week next week (I doubt we will...), I will apply
it anyway.

Best regards,
Rob