On Thu, Mar 01, 2012 at 05:43:00AM -0800, Mashiat Sarker Shakkhar wrote:
>
> --- /dev/null
> +++ b/libavcodec/wmalosslessdec.c
> @@ -0,0 +1,1288 @@
> +
> +static av_cold int decode_init(AVCodecContext *avctx)
> +{
> + WmallDecodeCtx *s = avctx->priv_data;
> + uint8_t *edata_ptr = avctx->extradata;
nit:align
> + if (avctx->extradata_size >= 18) {
> + s->decode_flags = AV_RL16(edata_ptr+14);
> + channel_mask = AV_RL32(edata_ptr+2);
spaces around +
> +static int decode_tilehdr(WmallDecodeCtx *s)
> +{
> + /* 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)
> + */
Is the mystery of this comment resolved now?
> +static void clear_codec_buffers(WmallDecodeCtx *s)
> +{
> + int ich, ilms;
> +
> + memset(s->acfilter_coeffs, 0, 16 * sizeof(s->acfilter_coeffs[0]));
> + memset(s->acfilter_prevvalues, 0, 16 * 2 *
> sizeof(s->acfilter_prevvalues[0]));
> + memset(s->lpc_coefs, 0, 40 * 2 * sizeof(s->lpc_coefs[0]));
> +
> + memset(s->mclms_coeffs, 0, 128 * sizeof(s->mclms_coeffs[0]));
> + memset(s->mclms_coeffs_cur, 0, 4 * sizeof(s->mclms_coeffs_cur[0]));
> + memset(s->mclms_prevvalues, 0, 64 * sizeof(s->mclms_prevvalues[0]));
> + memset(s->mclms_updates, 0, 64 * sizeof(s->mclms_updates[0]));
> +
> + 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(s->cdlms[ich][ilms].coefs[0]));
> + memset(s->cdlms[ich][ilms].lms_prevvalues, 0,
> + 512 * sizeof(s->cdlms[ich][ilms].lms_prevvalues[0]));
> + memset(s->cdlms[ich][ilms].lms_updates, 0,
> + 512 * sizeof(s->cdlms[ich][ilms].lms_updates[0]));
> + }
Using [0] instead of * here is equivalent but uncommon. So uncommon
that grep cannot find a single instance of it in Libav. This should
therefore be adapted to the more usual style.
> +/**
> + * @brief Decode one WMA frame.
> + * @param s codec context
> + * @return 0 if the trailer bit indicates that this is the last frame,
> + * 1 if there are additional frames
> + */
> +static int decode_frame(WmallDecodeCtx *s)
I remember Justin making me remove such Doxygen comments for "standard"
functions in the review for the escape130 decoder. And indeed, it
should not be:
00:45 <@DonDiego> ruggles: i remember you considering doxygen comments for
"standard"
functions in decoders unnecessary, like decode_frame() etc
00:45 <@DonDiego> so which ones should be documented, which should not be?
00:52 <@peloverde> anything that implements a standard interface should not be
documented
00:52 <@peloverde> the interface itself should be documented
00:53 <@DonDiego> what is "standard interface" in this case? the functions that
all
decoders share, i.e. avcodec callbacks?
00:54 <@peloverde> If it is being called from a function pointer where there
are other
functions that are assigned to the pointer
00:55 <@peloverde> basically ask yourself is this a vtable?
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
