On Mon, Nov 30, 2015 at 4:50 PM, Claudio Freire <klaussfre...@gmail.com> wrote: > On Mon, Nov 30, 2015 at 12:27 PM, Rostislav Pehlivanov > <atomnu...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel