[FFmpeg-devel] [PATCH] E-AC-3 spectral extension
Justin Ruggles
justin.ruggles
Sat Aug 1 00:23:23 CEST 2009
Michael Niedermayer wrote:
> On Sat, Jun 06, 2009 at 05:24:23PM -0400, Justin Ruggles wrote:
>> Justin Ruggles wrote:
>>> Michael Niedermayer wrote:
>>>> On Tue, Jun 02, 2009 at 09:19:23PM -0400, Justin Ruggles wrote:
>>>> [...]
>>>>> - /* TODO: spectral extension coordinates */
>>>>> + /* spectral extension coordinates */
>>>>> + if (s->spx_in_use) {
>>>>> + for (ch = 1; ch <= fbw_channels; ch++) {
>>>>> + if (s->channel_in_spx[ch]) {
>>>>> + if (s->first_spx_coords[ch] || get_bits1(gbc)) {
>>>>> + int bin;
>>>>> + float spx_blend;
>>>>> + int master_spx_coord;
>>>>> + s->first_spx_coords[ch] = 0;
>>>>> + spx_blend = get_bits(gbc, 5) * 0.03125f;
>>>>> + master_spx_coord = get_bits(gbc, 2) * 3;
>>>>> + bin = s->spx_start_freq;
>>>> an empty line in there somewhere would improve readability IMHO
>>> fixed.
>>>
>>>> [...]
>>>>> + /* decode spx coordinates */
>>>>> + spx_coord_exp = get_bits(gbc, 4);
>>>>> + spx_coord_mant = get_bits(gbc, 2);
>>>>> + if (spx_coord_exp == 15)
>>>>> + spx_coord = spx_coord_mant * 8.0f;
>>>>> + else
>>>>> + spx_coord = (spx_coord_mant + 4) * 4.0f;
>>>>> + spx_coord /= 1 << (spx_coord_exp + master_spx_coord);
>>>> something based on the following would avoid the /
>>>> spx_coord *= (1<<123) >> (spx_coord_exp + master_spx_coord)
>>>>
>>>> also *4 can be factored out of the if/else and into the factor above
>>> fixed (I think).
>>>
>>>> [...]
>>>>> @@ -66,6 +62,96 @@ typedef enum {
>>>>>
>>>>> #define EAC3_SR_CODE_REDUCED 3
>>>>>
>>>>> +void ff_eac3_apply_spectral_extension(AC3DecodeContext *s)
>>>>> +{
>>>>> + int bin, bnd, ch, i;
>>>>> + uint8_t wrapflag[SPX_MAX_BANDS]={0,}, num_copy_sections, copy_sizes[SPX_MAX_BANDS];
>>>>> + float rms_energy[SPX_MAX_BANDS];
>>>>> +
>>>>> + /* Set copy index mapping table. Set wrap flags to apply a notch filter at
>>>>> + wrap points later on. */
>>>>> + wrapflag[0] = 1;
>>>> double initialization
>>> fixed.
>>>
>>>>> + bin = s->spx_copy_start_freq;
>>>>> + num_copy_sections = 0;
>>>>> + for (bnd = 0; bnd < s->num_spx_bands; bnd++) {
>>>>> + int copysize;
>>>>> + int bandsize = s->spx_band_sizes[bnd];
>>>>> + if ((bin + bandsize) > s->spx_start_freq) {
>>>> redundant ()
>>> fixed.
>>>
>>> New patch attached.
>>>
>>> Thanks,
>>> Justin
>> oops. empty patch...
>>
>>
>> + int spx_end_subband;
>> +
>> + /* determine which channels use spx */
>> + if (s->channel_mode == AC3_CHMODE_MONO) {
>> + s->channel_in_spx[1] = 1;
>> + } else {
>> + for (ch = 1; ch <= fbw_channels; ch++)
>> + s->channel_in_spx[ch] = get_bits1(gbc);
>> + }
>> +
>> + s->spx_copy_start_freq = get_bits(gbc, 2) * 12 + 25;
>> + begf = get_bits(gbc, 3);
>> + endf = get_bits(gbc, 3);
>
>> + s->spx_start_subband = begf < 6 ? begf+2 : 2*begf-3;
>
> iam not sure, but maybe the following is easier to make sense of:
>
> begf + 2 + (begf >= 6 ? begf-5 : 0);
Yes, that's fine too.
>
>> + spx_end_subband = endf < 4 ? endf+5 : 2*endf+3;
>> + if (s->spx_start_subband >= spx_end_subband) {
>> + av_log(s->avctx, AV_LOG_ERROR, "invalid spectral extension range (%d >= %d)\n",
>> + s->spx_start_subband, spx_end_subband);
>> + return -1;
>> + }
>
> would it be possible not to write values in the context and validate them
> afterwards?
> doing this requires me to proof that the value is not used afterwards
> <- more time for doing a review, more chances for bugs ...
> als it would need to be documented so that noone mistakely uses invalid values
> through a change in the code beliving the values in the contex would
> be validted
> its really easier to move checks before the storing in the context
ok, I will fix this. I know they are not used, but I see your point.
> [...]
>> + float noise = nscale * (int)av_lfg_get(&s->dith_state);
>
> have you considered that int may be 32 and 64 bit on different platforms ?
No, I have not considered this. Since the code expects 32-bit, I'll
change it to int32_t.
Thanks for the review. I'll send a new patch this weekend.
-Justin
More information about the ffmpeg-devel
mailing list