On Fri, Jun 13, 2014 at 11:06:55AM +0200, Niels Möller wrote:
> Message-Id:
> <89817380bf2af23bedb4e61fa5ebea556befe5e3.1402646664.git.ni...@southpole.se>
> In-Reply-To:
> <6849fe55de4f2b999e480ea35f72029bbc09014e.1402646664.git.ni...@southpole.se>
> References:
> <6849fe55de4f2b999e480ea35f72029bbc09014e.1402646664.git.ni...@southpole.se>
> Subject: [PATCH 4/4] dcadec: Support for xll (lossless extension)
>
> Builds on top of changes by Paul B Mahol <[email protected]>, copied
> from the ffmpeg branch at
> https://github.com/richardpl/FFmpeg/commits/xll. In particular, new
> xll-related state variables, and the current function
> dca_xll_decode_header is an expanded version of Paul's
> dca_xll_decode_frame.
no need for the github reference
I have a bunch of mostly small comments below. If you cannot be bothered
to address them, I will.
> --- a/libavcodec/dcadec.c
> +++ b/libavcodec/dcadec.c
> @@ -941,8 +1031,85 @@ static void qmf_32_subbands(DCAContext *s, int chans,
>
> -static void lfe_interpolation_fir(DCAContext *s, int decimation_select,
> - int num_deci_sample, float *samples_in,
> +static QMF64_table *qmf64_precompute(void)
> +{
> +
> + for (i = 0; i < 32; i++)
> + for (j = 0; j < 32; j++)
> + table->dct4_coeff[i][j] = cos((2*i + 1) * (2*j + 1) * M_PI /
> 128);
spaces around *, more below
> @@ -1241,29 +1411,58 @@ static int dca_subsubframe(DCAContext *s, int
> base_channel, int block_index)
>
> -static int dca_filter_channels(DCAContext *s, int block_index)
> +static int dca_filter_channels(DCAContext *s, int block_index, int upsample)
> {
> + }
> + }
> + else {
} and else on the same line, more below
> @@ -1461,6 +1660,730 @@ static void dca_exss_skip_mix_coeffs(GetBitContext
> *gb, int channels, int out_ch
> +
> +/* Returns -1 on error */
> +static int32_t dca_get_dmix_coeff(DCAContext *s)
> +{
> +}
> +
> +/* Returns -1 on error */
> +static int32_t dca_get_inv_dmix_coeff(DCAContext *s)
> +{
> +}
Returning a more descriptive error code would be better here, probably
AVERROR_INVALLIDDATA.
> +/* parse XLL header */
> +static int dca_xll_decode_header(DCAContext *s)
> +{
> + version = get_bits(&s->gb, 4) + 1;
> + hdr_size = get_bits(&s->gb, 8) + 1;
nit: align =
> + chset->channels = get_bits(&s->gb, 4) + 1;
> + chset->residual_encode = get_bits(&s->gb, chset->channels);
> + chset->bit_resolution = get_bits(&s->gb, 5) + 1;
> + chset->bit_width = get_bits(&s->gb, 5) + 1;
> + chset->sampling_frequency = dca_sampling_freqs[get_bits(&s->gb, 4)];
> + chset->fs_interpolate = get_bits(&s->gb, 2);
> + chset->replacement_set = get_bits(&s->gb, 2);
same
> + if (chset->primary_ch_set) {
> + for (i = 0; i < chset->downmix_ncoeffs; i++)
> + if ( (chset->downmix_coeffs[i] =
> dca_get_dmix_coeff(s)) == -1)
> + return AVERROR_INVALIDDATA;
Drop the space after (.
> + chset->downmix_coeffs[i+r]
> + = (chset->downmix_coeffs[i] * (int64_t)
> coeff + (1<<15)) >> 16;
Spaces around operators and keep the = on the previous line.
> +/* parse XLL navigation table */
> +static int dca_xll_decode_navi(DCAContext *s, int asset_end)
> +{
> + for (chset = 0; chset < s->xll_nch_sets; chset++)
> + if (band < s->xll_chsets[chset].num_freq_bands) {
> + s->xll_navi.chset_size[band][seg][chset]
> + = get_bits(&s->gb, s->xll_bits4seg_size) + 1;
> + s->xll_navi.segment_size[band][seg]
> + += s->xll_navi.chset_size[band][seg][chset];
Same, keep the = on the previous line.
> +static int dca_xll_decode_audio(DCAContext *s, AVFrame *frame)
This function is huge, is there a way to split it?
> + /* Layout: First the sample buffer for one segment per channel,
> + * followed by history buffers of DCA_XLL_AORDER_MAX samples for
> + * each channel. */
> + av_fast_malloc(&s->xll_sample_buf, &s->xll_sample_buf_size,
> + (s->xll_smpl_in_seg + DCA_XLL_AORDER_MAX) *
> + s->xll_channels * sizeof(*s->xll_sample_buf));
> + if (!s->xll_sample_buf)
> + return AVERROR(ENOMEM);
Where does this memory get freed?
> + av_log(s->avctx, AV_LOG_DEBUG, "aux count %d (bits
> %d)\n",
> + count, s->xll_log_smpl_in_seg);
Indentation on the second line is off.
> + if (chset->downmix_coeff_code_embedded
> + && !chset->primary_ch_set
> + && chset->hier_chset) {
Indentation is off, move the && to the end of the line(s).
> + next_chset:
> + in_channel += chset->channels;
Move the goto label to the first column.
> @@ -1720,8 +2685,15 @@ static int dca_decode_frame(AVCodecContext *avctx,
> void *data,
>
> + av_log(avctx, AV_LOG_DEBUG, "%s: stream_index %d, pos %ld, frame index
> %u, sample index %u\n",
> + __func__, avpkt->stream_index, avpkt->pos, s->frame_index,
> s->sample_index);
avpkt->pos is int64_t, so use PRId64 as printf format specifier.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
