You are missing a version bump now that the tree is declared stable again.
On Tue, Feb 28, 2012 at 06:20:27PM -0800, Mashiat Sarker Shakkhar wrote:
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -770,6 +770,7 @@ following image formats are supported:
> @item Windows Media Audio 2 @tab X @tab X
> @item Windows Media Audio Pro @tab @tab X
> @item Windows Media Audio Voice @tab @tab X
> +@item Windows Media Audio Lossless @tab @tab X
> @end multitable
order
> --- /dev/null
> +++ b/libavcodec/wmalosslessdec.c
> @@ -0,0 +1,1294 @@
> +
> +/**
> + * @brief frame specific decoder context for a single channel
frame-specific
> + uint16_t subframe_offset[MAX_SUBFRAMES]; ///< subframe
> positions in the current frame
I'd call this subframe_offsetS (plural) to make it clearer what it is.
> +typedef struct WmallDecodeCtx {
> + /* generic decoder variables */
> + AVCodecContext* avctx; ///< codec context for
> av_log
You don't need a complete AVCodecContext just for logging.
> + uint8_t len_prefix; ///< frame is prefixed
> with its length
> + uint8_t dynamic_range_compression; ///< frame contains DRC
> data
So these two are flags?
> + int next_packet_start; ///< start offset of the
> next wma packet in the demuxer packet
WMA
> + uint8_t packet_offset; ///< frame offset in the
> packet
variable name and comment confuse me - what is offset where?
> + // WMA lossless
The comment is slightly confusing me. This is a WMA lossless
decoder, so ... ?
> +static av_cold int decode_init(AVCodecContext *avctx)
> +{
> + s->dynamic_range_compression = (s->decode_flags & 0x80);
pointless ()
> + if (s->num_channels < 0) {
> + av_log(avctx, AV_LOG_ERROR, "invalid number of channels %d\n",
> + s->num_channels);
Indentation is off.
> + avcodec_get_frame_defaults(&s->frame);
> + avctx->coded_frame = &s->frame;
> +
> + avctx->channel_layout = channel_mask;
nit:
avctx->coded_frame = &s->frame;
avctx->channel_layout = channel_mask;
> +/**
> + *@brief Decode the subframe length.
> + *@param s context
> + *@param offset sample offset in the frame
> + *@return decoded subframe length on success, < 0 in case of an error
> + */
Leave a space between '*' and '@', same everywhere.
> +static int decode_tilehdr(WmallDecodeCtx *s)
> +{
> + uint16_t num_samples[WMALL_MAX_CHANNELS] = {0}; /**< sum of samples
> for all currently known subframes of a channel */
> + uint8_t contains_subframe[WMALL_MAX_CHANNELS]; /**< flag indicating
> if a channel contains the current subframe */
> + int channels_for_cur_subframe = s->num_channels; /**< number of
> channels that contain the current subframe */
> + int fixed_channel_layout = 0; /**< flag indicating
> that all channels use the same subfra2me offsets and sizes */
> + int min_channel_len = 0; /**< smallest sum of
> samples (channels with this length will be processed first) */
Doxygen comments for function-local variables?
> + /* Should never consume more than 3073 bits (256 iterations for the
> + * while loop when the minimum amount of 128 samples is subtracted
> + * from missing samples in the 8 channel case).
> + * 1 + BLOCK_MAX_SIZE * MAX_CHANNELS / BLOCK_MIN_SIZE * (MAX_CHANNELS +
> 4)
> + */
This comment is missing a subject, so it's not clear if it refers to the
whole function, the next block, or ...
> + if (fixed_channel_layout || channels_for_cur_subframe == 1 ||
> + (min_channel_len == s->samples_per_frame -
> s->min_samples_per_subframe)) {
indentation
> + shift_l = 32 - s->cdlms[c][i].bitsend;
> + shift_r = 32 - 2 - s->cdlms[c][i].scaling;
possibly more readable:
shift_l = 32 - s->cdlms[c][i].bitsend;
shift_r = 32 - s->cdlms[c][i].scaling -2;
> +static int decode_channel_residues(WmallDecodeCtx *s, int ch, int tile_size)
> +{
> + int i = 0;
> + for (; i < tile_size; i++) {
That looks weird, initialize it inside of the for loop.
> + else {
> + rem_bits = av_ceil_log2(ave_mean);
> + rem = rem_bits ? get_bits(&s->gb, rem_bits) : 0;
> + residue = (quo << rem_bits) + rem;
align
> + s->ave_sum[ch] = residue + s->ave_sum[ch] -
> + (s->ave_sum[ch] >> s->movave_scaling);
align after the '='
> +static void decode_lpc(WmallDecodeCtx *s)
> +{
> + int ch, i, cbits;
> + s->lpc_order = get_bits(&s->gb, 5) + 1;
> + s->lpc_scaling = get_bits(&s->gb, 4);
> + s->lpc_intbits = get_bits(&s->gb, 3) + 1;
align
> +/**
> + *@brief Resets filter parameters and transient area at new seekable tile
Reset, end in a period.
> + s->channel[ich].transient_counter = s->samples_per_frame;
> + s->transient[ich] = 1;
> + s->transient_pos[ich] = 0;
align
> + int order = s->mclms_order;
> + int num_channels = s->num_channels;
align
> +static void revert_mclms(WmallDecodeCtx *s, int tile_size)
> +{
> + int icoef, pred[WMALL_MAX_CHANNELS] = {0};
{ 0 }
> + 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];
align after the '='
> +static void lms_update(WmallDecodeCtx *s, int ich, int ilms, int input, int
> residue)
long line
> + if (s->bV3RTM) {
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> + s->cdlms[ich][ilms].lms_updates[icoef + recent] *= 2;
> + } else {
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> + s->cdlms[ich][ilms].lms_updates[icoef] *= 2;
> + if (s->bV3RTM)
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> + s->cdlms[ich][ilms].lms_updates[icoef + recent] /= 2;
> + else
> + for (icoef = 0; icoef < s->cdlms[ich][ilms].order; icoef++)
> + s->cdlms[ich][ilms].lms_updates[icoef] /= 2;
shift instead?
> +static void revert_cdlms(WmallDecodeCtx *s, int ch, int coef_begin, int
> coef_end)
long line
> + int64_t *filter_coeffs = s->acfilter_coeffs;
> + int scaling = s->acfilter_scaling;
> + int order = s->acfilter_order;
align
> + /* reset channel context and find the next block offset and size
> + == the next block of the channel with the smallest number of
> + decoded samples
> + */
nit: star this comment, as you did above
> + if (s->do_lpc) {
> + decode_lpc(s);
> + av_log_ask_for_sample(s->avctx, "Inverse LPC filter not "
> + "implemented. Expect wrong output.\n");
indentation
> + if (rawpcm_tile) {
> + int bits = s->bits_per_sample - padding_zeroes;
> + 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
> + } else {
> + for (i = 0; i < s->num_channels; i++)
> + if (s->is_channel_coded[i]) {
indentation
> + } else {
> +/*
> + while (get_bits_count(gb) < s->num_saved_bits && get_bits1(gb) == 0)
> + av_dlog(s->avctx, AV_LOG_DEBUG, "skip1\n");
> +*/
> + }
This is still commented-out cruft that should at the very least be explained.
You could also add an assert or so instead.
> +/**
> + *@brief Fill the bit reservoir with a (partial) frame.
> + *@param s codec context
> + *@param gb bitstream reader context
> + *@param len length of the partial frame
> + *@param append decides wether to reset the buffer or not
whether
I'm left wondering why a parameter named "append" makes such a decision.
> + /* when the frame data does not need to be concatenated, the input buffer
> + is resetted and additional bits from the previous frame are copyed
reset, copied
> + if (!append) {
> + s->frame_offset = get_bits_count(gb) & 7;
> + s->num_saved_bits = s->frame_offset;
align
> + s->num_saved_bits += len;
> + if (!append) {
> + avpriv_copy_bits(&s->pb, gb->buffer + (get_bits_count(gb) >> 3),
> + s->num_saved_bits);
indentation
> + s->next_packet_start = buf_size - avctx->block_align;
> + buf_size = avctx->block_align;
> + s->buf_bit_size = buf_size << 3;
align
> + /* parse packet header */
> + init_get_bits(gb, buf, s->buf_bit_size);
> + packet_sequence_number = get_bits(gb, 4);
> + seekable_frame_in_packet = get_bits1(gb);
> + spliced_packet = get_bits1(gb);
align
> + if (s->packet_loss) {
> + /* reset number of saved bits so that the decoder
> + does not start to decode incomplete frames in the
> + s->len_prefix == 0 case */
> + s->num_saved_bits = 0;
> + s->packet_loss = 0;
align
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
