[FFmpeg-devel] [PATCH] AAC encoder: improve SF range utilization

[Claudio Freire]

On Mon, Nov 30, 2015 at 12:27 PM, Rostislav Pehlivanov
<atomnuker at gmail.com> wrote:
> On Sun, 2015-11-29 at 16:54 -0300, Claudio Freire wrote:
>> Before pushing this, I'd like some feedback,
>> especially about
>> the implementation of point 3. I'm not sure the AAC encoder
>> setting the cutoff in the encoder context like this is legal or
>> desirable.
> I think setting the cutoff is necessary. You can't avoid holes and yet
> keep a full bandwidth as you decrease bits unless you like the sound of massive quantization errors.

My point was more about whether a codec should write into the context
struct like that or not. Basically an API technicality that's unclear
to me.

>> +    unsigned char nextband[128];
> Code wise, could you rename unsigned chars to uint8_t for consistency
> with the rest of the code?

Sure

>> + if (!ff_sfdelta_can_remove_band(sce, nextband, prev, w*16+g)) {
>> +     sce-->band_type[w*16+g] = 1;
>> + } else {
>> +     sce->zeroes[w*16+g] = 1;
>> +     sce->band_type[w*16+g] = 0;
>> + }
> I'd recommend putting sce->zeroes[w*16+g] = 0 in the first part for
> consistency and also band_type[] = ZERO_BT (for 0)

Yeah, not sure why I didn't do that from the start.

> and some random _BT
> for non-zero bands. The band_type[] gets overriden from what I can see.

That I cannot. The value there is taken as the minimum allowed band
type by the codebook_trellis_rate function, which picks the optimum
codebook selection. So it cannot be set to a random number, it really
needs to be set to 1 (first nonzero codebook), and it has no constant
or name other than "1".

>> +static inline int ff_sfdelta_can_replace(const SingleChannelElement
>> *sce, const unsigned char *nextband, int prev_sf, int new_sf, int
>> band)
>> +{
>> +    return new_sf >= (prev_sf - SCALE_MAX_DIFF) && new_sf
>> <= (prev_sf + SCALE_MAX_DIFF)
>> +        && sce->sf_idx[nextband[band]] >= (new_sf -
>> SCALE_MAX_DIFF) && sce->sf_idx[nextband[band]] <=
>> (new_sf + SCALE_MAX_DIFF);
>> +}
> Consider splitting up the function definition and using some static
> uint8_t to split up the ternary operator because it's really long.

I'll see how to make it smaller and easier to read, but this really
needs to be inlineable (it's called in a tight loop and is very small,
perhaps amounting to at most a dozen assembler instructions).

Also I don't see how a static var would help or even be correct here.
Perhaps you meant something else?