On Tue, Feb 28, 2012 at 04:37:00AM -0800, Mashiat Sarker Shakkhar wrote:
>
> --- /dev/null
> +++ b/libavcodec/wmalosslessdec.c
> @@ -0,0 +1,1314 @@
> + * Copyright (c) 2011 - 12 Mashiat Sarker Shakkhar
2012
> +/** current decoder limitations */
> +#define WMALL_MAX_CHANNELS 8 ///< max number
> of handled channels
> +#define MAX_SUBFRAMES 32 ///< max number
> of subframes per channel
> +#define MAX_BANDS 29 ///< max number
> of scale factor bands
> +#define MAX_FRAMESIZE 32768 ///< maximum
> compressed frame size
nit: align the numbers
> +/**
> + *@brief Initialize the decoder.
> + *@param avctx codec context
> + *@return 0 on success, -1 otherwise
> + */
> +static av_cold int decode_init(AVCodecContext *avctx)
Do all of these static functions need Doxygen documentation?
> + /** dump the extradata */
> + for (i = 0; i < avctx->extradata_size; i++)
Why is this a Doxygen comment?
> + av_dlog(avctx, AV_LOG_DEBUG, "[%x] ", avctx->extradata[i]);
> + av_dlog(avctx, AV_LOG_DEBUG, "\n");
> +
> + } else {
> + av_log_ask_for_sample(avctx, "Unknown extradata size\n");
> + return AVERROR_INVALIDDATA;
> + }
> +
> + /** generic init */
> + s->log2_frame_size = av_log2(avctx->block_align) + 4;
Why is this a Doxygen comment?
> +
> + /** frame info */
> + s->skip_frame = 1; /* skip first frame */
Why is this a Doxygen comment?
> + /** get frame len */
> + s->samples_per_frame = 1 << ff_wma_get_frame_len_bits(avctx->sample_rate,
> + 3,
> s->decode_flags);
Why is this a Doxygen comment?
> +
> + /** init previous block len */
> + for (i = 0; i < avctx->channels; i++)
> + s->channel[i].prev_block_len = s->samples_per_frame;
Why is this a Doxygen comment?
You get the point - more below...
> + if (channel_mask & 8) {
> + unsigned int mask;
> + for (mask = 1; mask < 16; mask <<= 1) {
> + if (channel_mask & mask)
> + ++s->lfe_channel;
> + }
nit: pointless {}
> + if (s->num_channels < 0) {
> + av_log(avctx, AV_LOG_ERROR, "invalid number of channels %d\n",
> s->num_channels);
nit: long line
> + /* Should never consume more than 3073 bits (256 iterations for the
> + * while loop when always the minimum amount of 128 samples is
> substracted
> + * from missing samples in the 8 channel case).
> + * 1 + BLOCK_MAX_SIZE * MAX_CHANNELS / BLOCK_MIN_SIZE * (MAX_CHANNELS +
> 4)
> + */
Either you can drop the "always" from the sentence or I don't understand
what you are trying to say.
> + /** reset tiling information */
> + for (c = 0; c < s->num_channels; c++)
> + s->channel[c].num_subframes = 0;
> +
> + memset(num_samples, 0, sizeof(num_samples));
Initialize num_samples to zero instead.
> +static void decode_ac_filter(WmallDecodeCtx *s)
> +{
> + int i;
> + s->acfilter_order = get_bits(&s->gb, 4) + 1;
> + s->acfilter_scaling = get_bits(&s->gb, 4);
nit: align
> + for(i = 0; i < s->acfilter_order; i++) {
> + s->acfilter_coeffs[i] = get_bits(&s->gb, s->acfilter_scaling) + 1;
> + }
nit: pointless {}
> +static void decode_mclms(WmallDecodeCtx *s)
> +{
> + s->mclms_order = (get_bits(&s->gb, 4) + 1) * 2;
> + s->mclms_scaling = get_bits(&s->gb, 4);
nit: align
.. more alignment opportunities below
> + for (i = 0; i < s->mclms_order * s->num_channels * s->num_channels;
> i++) {
> + s->mclms_coeffs[i] = get_bits(&s->gb, send_coef_bits);
> + }
nit: pointless {}
> +static void decode_cdlms(WmallDecodeCtx *s)
And what is cdlms anyway? You have so many Doxygen comments, but
this is not explained anywhere.
> + for(i = 0; i < s->cdlms_ttl[c]; i++)
> + s->cdlms[c][i].scaling = get_bits(&s->gb, 4);
for ( - more below
> + if(s->seekable_tile) {
if ( - more below
> + s->ave_sum[ch] = residue + s->ave_sum[ch] -
> + (s->ave_sum[ch] >> s->movave_scaling);
Indentation is off.
> +static void clear_codec_buffers(WmallDecodeCtx *s)
> +{
> + int ich, ilms;
> +
> + memset(s->acfilter_coeffs , 0, 16 * sizeof(int));
> + memset(s->acfilter_prevvalues, 0, 16 * 2 * sizeof(int));
> + memset(s->lpc_coefs , 0, 40 * 2 * sizeof(int));
> +
> + memset(s->mclms_coeffs , 0, 128 * sizeof(int16_t));
> + memset(s->mclms_coeffs_cur, 0, 4 * sizeof(int16_t));
> + memset(s->mclms_prevvalues, 0, 64 * sizeof(int));
> + memset(s->mclms_updates , 0, 64 * sizeof(int16_t));
> +
> + for (ich = 0; ich < s->num_channels; ich++) {
> + for (ilms = 0; ilms < s->cdlms_ttl[ich]; ilms++) {
> + memset(s->cdlms[ich][ilms].coefs , 0, 256 *
> sizeof(int16_t));
> + memset(s->cdlms[ich][ilms].lms_prevvalues, 0, 512 * sizeof(int));
> + memset(s->cdlms[ich][ilms].lms_updates , 0, 512 *
> sizeof(int16_t));
Lose the spaces before the commas.
sizeof(member) would be better than sizeof(type) I think.
> + /** first sample of a seekable subframe is considered as the
> starting of
> + a transient area which is samples_per_frame samples long */
> + s->channel[ich].transient_counter = s->samples_per_frame;
> + s->transient[ich] = 1;
> + s->transient_pos[ich] = 0;
more stray Doxygen, you could also align here.
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> + pred += s->cdlms[ich][ilms].coefs[icoef] *
> + s->cdlms[ich][ilms].lms_prevvalues[icoef + recent];
Indentation is off.
> +static void lms_update(WmallDecodeCtx *s, int ich, int ilms, int input, int
> residue)
nit: long line
> +static void use_normal_update_speed(WmallDecodeCtx *s, int ich)
> +{
> + int ilms, recent, icoef;
> + for (ilms = s->cdlms_ttl[ich] - 1; ilms >= 0; ilms--) {
> + if (s->update_speed[ich] == 8)
> + if (s->bV3RTM) {
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> + } else {
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> +static void revert_cdlms(WmallDecodeCtx *s, int ch, int coef_begin, int
> coef_end)
> +{
> + int icoef;
> +
> + for (ilms = num_lms - 1; ilms >= 0; ilms--) {
> + for (icoef = coef_begin; icoef < coef_end; icoef++) {
> +static void revert_inter_ch_decorr(WmallDecodeCtx *s, int tile_size)
> +{
> + int icoef;
> + else if (s->is_channel_coded[0] && s->is_channel_coded[1]) {
> + for (icoef = 0; icoef < tile_size; icoef++) {
Given that you seem to have made it a point to declare variables in the
innermost scope above, you could move icoef into the block where it it
required in these cases.
> + if(s->do_lpc) {
> + decode_lpc(s);
> + av_log_ask_for_sample(s->avctx, "Inverse LPC filter not "
> + "implemented. Expect wrong output.\n");
Indentation is totally off.
> + av_dlog(s->avctx, AV_LOG_DEBUG, "RAWPCM %d bits per sample. "
> + "total %d bits, remain=%d\n", bits,
> + bits * s->num_channels * subframe_len, get_bits_count(&s->gb));
Indentation is off.
> + if (s->skip_frame) {
> + s->skip_frame = 0;
> + }
pointless {}
> + } else {
> +/*
> + while (get_bits_count(gb) < s->num_saved_bits && get_bits1(gb) == 0)
> {
> + av_dlog(s->avctx, AV_LOG_DEBUG, "skip1\n");
> + }
> +*/
> + }
?
Also, indentation is off.
> + if (!append) {
> + avpriv_copy_bits(&s->pb, gb->buffer + (get_bits_count(gb) >> 3),
> + s->num_saved_bits);
indentation
> + if (s->packet_done && !s->packet_loss &&
> + remaining_bits(s, gb) > 0) {
> + /** save the rest of the data so that it can be decoded
> + with the next packet */
> + save_bits(s, gb, remaining_bits(s, gb), 0);
Why is this a Doxygen comment?
> + *(AVFrame *)data = s->frame;
> + *got_frame_ptr = 1;
> + s->packet_offset = get_bits_count(gb) & 7;
nit: align the '='
> +/**
> + *@brief wmall decoder
> + */
> +AVCodec ff_wmalossless_decoder = {
What is the Doxygen comment for?
> + .name = "wmalossless",
> + .type = AVMEDIA_TYPE_AUDIO,
> + .id = CODEC_ID_WMALOSSLESS,
> + .priv_data_size = sizeof(WmallDecodeCtx),
> + .init = decode_init,
> + .decode = decode_packet,
> + .capabilities = CODEC_CAP_SUBFRAMES | CODEC_CAP_DR1 |
> CODEC_CAP_EXPERIMENTAL,
> + .long_name = NULL_IF_CONFIG_SMALL("Windows Media Audio 9 Lossless"),
align the '='
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
