----- Original Message -----
> From: Diego Biurrun <[email protected]>
> To: libav development <[email protected]>
> Cc:
> Sent: Wednesday, February 29, 2012 9:00 PM
> Subject: Re: [libav-devel] [PATCH] Windows Media Audio Lossless decoder
[...]
>> +typedef struct WmallDecodeCtx {
>> + /* generic decoder variables */
>> + AVCodecContext* avctx; ///< codec context
> for av_log
>
> You don't need a complete AVCodecContext just for logging.
That comment does not really describe the purpose of avctx, removed.
>
>> + uint8_t len_prefix; ///< frame is
> prefixed with its length
>> + uint8_t dynamic_range_compression; ///< frame contains
> DRC data
>
> So these two are flags?
Yes. Changed to int, more appropriate for flags.
[..]
>> + uint8_t packet_offset; ///< frame offset
> in the packet
>
> variable name and comment confuse me - what is offset where?
I think the name is fine, it's offset to the beginning of the frame
in the packet. Changed the comment to make it clearer.
>
>> + // WMA lossless
>
> The comment is slightly confusing me. This is a WMA lossless
> decoder, so ... ?
What it means is probably that the following members are WMALL-
specific. Generally, WMA Lossless shares bitstream syntax with
WMA Pro, so there are a few member which are common between WMA
Pro and WMA Lossless contexts. Then there are a few members
which are ASF-specific.
Changed the comment to "WMA Lossless-specific".
[...]
>> + /* 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 ...
That comment has been taken verbatim from wmaprodec.c by Andreas, I am not
sure if it even applies to WMA Lossless. Someone more experienced than me
should comment. (May be Andreas or Kostya?)
[..]
>> + 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;
LOL. OK.
[...]
>> + 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?
Don't think so. IIRC, there is an issue regarding zero-truncation
involved here. Please ask Mans to comment. I'm not an authority here.
[...]
>> +/**
>> + *@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.
I don't know, ask Andreas or whoever wrote the WMA Pro code. I would
rather not change it.
The rest has been taken care of.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
