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

[Rostislav Pehlivanov]

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.

> Fate comparison targets don't all reflect improvements in
> quality, yet listening tests show substantially improved quality
> and stability.
That's okay, FATE isn't meant to replace human ears :)
Definitely a huge improvement at low bitrates though.

> 3. Account for lowpass cutoff during PSY analysis
This is fine and it's what the psy system should be doing instead of
always considering no lowpass is in effect.

> 1. Increase SF range utilization.
Great, this should increase the stability and decrease bug reports :)

> 2. PNS tweaks
> 4. Tweaks to RC lambda tracking loop in relation to PNS
> This tweak makes PNS much less aggressive, though it can still
> use some further tweaks.
That's fine, PNS was previously so aggressive because it needed to fill
up holes that twoloop used to make.

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

> + 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) and some random _BT
for non-zero bands. The band_type[] gets overriden from what I can see.

> +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.

But those 3 things are really just small nitpicks. It really improves
quality quite a lot.

So yeah, LGTM. Did test it extensively with quite a lot of tracks and
it doesn't seem to trigger any asserts anymore. Fix those 3 small
things if you have the time.