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

[Hendrik Leppkes]

On Mon, Nov 30, 2015 at 4:50 PM, Claudio Freire <klaussfreire at gmail.com> wrote:
> 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.
>

It seems slightly odd to write into that variable, since thats the
cutoff the user requests.
Maybe you can use/write into a private variable instead? ac3enc seems
to use a private variable for similar purposes.

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