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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
