On Tue, Jul 30, 2013 at 01:25:03PM +0200, Diego Biurrun wrote:
>
> > --- a/libavcodec/twinvq.c
> > +++ b/libavcodec/twinvq.c
> > @@ -1088,7 +542,7 @@ static av_cold void init_bitstream_params(TwinContext
> > *tctx)
> > -static av_cold int twin_decode_close(AVCodecContext *avctx)
> > +av_cold int ff_tvq_decode_close(AVCodecContext *avctx)
>
> Previously the name prefix was "twin", now it's "tvq". Both seem less
> than ideal to me; the natural prefix appears to be "twinvq".
The proper name for the coding method is TwinVQ - that's why I've shortened it
to tvq (the names are usually long enough even without prefix) and left twin_
for the original decoder. And it has a nice property of not changing length
when removing "static".
> > --- /dev/null
> > +++ b/libavcodec/twinvq.h
> > @@ -0,0 +1,162 @@
> > +
> > +enum FrameType {
> > + FT_SHORT = 0, ///< Short frame (divided in n sub-blocks)
> > + FT_MEDIUM, ///< Medium frame (divided in m<n sub-blocks)
> > + FT_LONG, ///< Long frame (single sub-block + PPC)
> > + FT_PPC, ///< Periodic Peak Component (part of the long frame)
> > +};
>
> Other enums by that name exist, please make the name more specific.
>
> > +struct FrameMode {
> > +};
>
> same
>
> > +typedef struct {
> > +} ModeTab;
>
> same
>
> No anonymous structs in headers please, we had issues with that
> before and I just eliminated all anonymous structs in headers.
> Maybe I will need to eliminate all anonymous structs altogether.
Maybe.
> > +#define PPC_SHAPE_CB_SIZE 64
> > +#define PPC_SHAPE_LEN_MAX 60
> > +#define SUB_AMP_MAX 4500.0
> > +#define MULAW_MU 100.0
> > +#define GAIN_BITS 8
> > +#define AMP_MAX 13000.0
> > +#define SUB_GAIN_BITS 5
> > +#define WINDOW_TYPE_BITS 4
> > +#define PGAIN_MU 200
> > +#define LSP_COEFS_MAX 20
> > +#define LSP_SPLIT_MAX 4
> > +#define CHANNELS_MAX 2
> > +#define SUBBLOCKS_MAX 16
> > +#define BARK_N_COEF_MAX 4
>
> All of this looks like it should be prefixed as well.
>
> > +/** @note not speed critical, hence not optimized */
> > +static inline void memset_float(float *buf, float val, int size)
> > +{
> > + while (size--)
> > + *buf++ = val;
> > +}
> > +
> > +static inline float mulawinv(float y, float clip, float mu)
> > +{
> > + y = av_clipf(y / clip, -1, 1);
> > + return clip * FFSIGN(y) * (exp(log(1 + mu) * fabs(y)) - 1) / mu;
> > +}
>
> name prefixes?
maybe
> > +void ff_tvq_decode_lsp(TwinContext *tctx, int lpc_idx1, uint8_t *lpc_idx2,
> > + int lpc_hist_idx, float *lsp, float *hist);
> > +void ff_tvq_dec_lpc_spectrum_inv(TwinContext *tctx, float *lsp,
> > + enum FrameType ftype, float *lpc);
> > +void ff_tvq_imdct_output(TwinContext *tctx, enum FrameType ftype, int
> > wtype,
> > + float **out);
> > +av_cold int ff_tvq_init_mdct_win(TwinContext *tctx);
> > +av_cold void ff_tvq_init_bitstream_params(TwinContext *tctx);
> > +av_cold int ff_tvq_decode_close(AVCodecContext *avctx);
>
> The naming is an inconsistent and non-obvious mess IMO. The context
> struct uses "Twin", here it's "tvq". I suggest "twinvq" everywhere.
Why should it be consistent anyway? Though I agree that twinvq everywhere
would be logical and consistent.
> This seems to be missing an attributes.h #include, does it pass
> "make check"?
it does
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
