On Thu, Oct 10, 2013 at 9:14 PM, Maxim Polijakowski <[email protected]> wrote:
> Hi crews, > > the attached patch adds an open-source decoder for Sony's ATRAC3+ format. > There is a partial description of its internals located here: > http://wiki.multimedia.cx/**index.php?title=ATRAC3plus<http://wiki.multimedia.cx/index.php?title=ATRAC3plus> > > Nice! I have a few comments: +av_cold void ff_atrac3p_init_vlcs(ATRAC3PVlcTabs *vlc_tabs) +{ + int i; + + ff_init_vlc_sparse(&vlc_tabs->wl_vlc_tabs[0], 2, 3, + ff_atrac3p_wl_huff_bits1, 1, 1, + ff_atrac3p_wl_huff_code1, 1, 1, + ff_atrac3p_wl_huff_xlat1, 1, 1, 0); + + ff_init_vlc_sparse(&vlc_tabs->wl_vlc_tabs[1], 3, 5, + ff_atrac3p_wl_huff_bits2, 1, 1, + ff_atrac3p_wl_huff_code2, 1, 1, + ff_atrac3p_wl_huff_xlat2, 1, 1, 0); + + init_vlc(&vlc_tabs->wl_vlc_tabs[2], 5, 8, + ff_atrac3p_wl_huff_bits3, 1, 1, + ff_atrac3p_wl_huff_code3, 1, 1, 0); + + init_vlc(&vlc_tabs->wl_vlc_tabs[3], 5, 8, + ff_atrac3p_wl_huff_bits4, 1, 1, + ff_atrac3p_wl_huff_code4, 1, 1, 0); + + ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[0], 9, 64, + ff_atrac3p_sf_huff_bits1, 1, 1, + ff_atrac3p_sf_huff_code1, 2, 2, + ff_atrac3p_sf_huff_xlat1, 1, 1, 0); + + ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[1], 9, 64, + ff_atrac3p_sf_huff_bits1, 1, 1, + ff_atrac3p_sf_huff_code1, 2, 2, + ff_atrac3p_sf_huff_xlat2, 1, 1, 0); + + init_vlc(&vlc_tabs->sf_vlc_tabs[2], 9, 64, + ff_atrac3p_sf_huff_bits2, 1, 1, + ff_atrac3p_sf_huff_code2, 2, 2, 0); + + init_vlc(&vlc_tabs->sf_vlc_tabs[3], 9, 64, + ff_atrac3p_sf_huff_bits3, 1, 1, + ff_atrac3p_sf_huff_code3, 2, 2, 0); + + ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[4], 6, 16, + ff_atrac3p_sf_huff_bits4, 1, 1, + ff_atrac3p_sf_huff_code4, 1, 1, + ff_atrac3p_sf_huff_xlat4, 1, 1, 0); + + ff_init_vlc_sparse(&vlc_tabs->sf_vlc_tabs[5], 6, 16, + ff_atrac3p_sf_huff_bits4, 1, 1, + ff_atrac3p_sf_huff_code4, 1, 1, + ff_atrac3p_sf_huff_xlat5, 1, 1, 0); + + init_vlc(&vlc_tabs->sf_vlc_tabs[6], 7, 16, + ff_atrac3p_sf_huff_bits5, 1, 1, + ff_atrac3p_sf_huff_code5, 1, 1, 0); + + init_vlc(&vlc_tabs->sf_vlc_tabs[7], 7, 16, + ff_atrac3p_sf_huff_bits6, 1, 1, + ff_atrac3p_sf_huff_code6, 1, 1, 0); + + init_vlc(&vlc_tabs->ct_vlc_tabs[0], 3, 4, + ff_atrac3p_ct_huff_bits1, 1, 1, + ff_atrac3p_ct_huff_code1, 1, 1, 0); + + init_vlc(&vlc_tabs->ct_vlc_tabs[1], 4, 8, + ff_atrac3p_ct_huff_bits2, 1, 1, + ff_atrac3p_ct_huff_code2, 1, 1, 0); + + ff_init_vlc_sparse(&vlc_tabs->ct_vlc_tabs[2], 4, 8, + ff_atrac3p_ct_huff_bits2, 1, 1, + ff_atrac3p_ct_huff_code2, 1, 1, + ff_atrac3p_ct_huff_xlat1, 1, 1, 0); + + init_vlc(&vlc_tabs->ct_vlc_tabs[3], 4, 8, + ff_atrac3p_ct_huff_bits3, 1, 1, + ff_atrac3p_ct_huff_code3, 1, 1, 0); You could define: const uint8_t *ff_atrac3p_sf_huff_bits[] = {atrac3p_sf_huff_bits1, atrac3p_sf_huff_bits2, atrac3p_sf_huff_bits1, ...}; And use one (or three) loops to init it (also valid for the ones initialized with build_canonical_huff). +static void subtract_sf_weights(Atrac3pChanUnitCtx *ctx, + Atrac3pChanParams *chan, int wtab_idx) +{ + int i; + const int8_t *weights_tab; + + weights_tab = &ff_atrac3p_sf_weights[wtab_idx - 1][0]; + + for (i = 0; i < ctx->used_quant_units; i++) + chan->qu_sf_idx[i] -= weights_tab[i]; +} Please move this function right after add_wordlen_weights(). Even if I understand the reason for the code duplication, I think grouping them together improves readability. There is a pretty large number of decode_* functions with four different coding modes, some more or less similar. In the cases where applicable, It would be nice to have some comments like: /** * This function almost identical to decode_something_else(), but VLC delta coding use * different coefficients. */ +/** + * ATRAC3+ uses two different MDCT windows: + * - The first one is just the plain sine window of size 256. + * - The 2nd one is the plain sine window of size 128 + * wrapped into zero (at the start) and one (at the end) regions. + * Both regions are 32 samples long. */ +static float mdct_wind_steep[128]; ///< second MDCT window + +av_cold void ff_atrac3p_init_imdct(AVCodecContext *avctx, FFTContext *mdct_ctx) +{ + int i; + + avpriv_float_dsp_init(&atrac3p_dsp, avctx->flags & CODEC_FLAG_BITEXACT); + + ff_init_ff_sine_windows(7); + ff_init_ff_sine_windows(6); + + /* Copy the 2nd sine window and place it between one/zero regions. */ + memcpy(&mdct_wind_steep[32], ff_sine_64, sizeof(ff_sine_64)); + + for (i = 0; i < 32; i++) { + mdct_wind_steep[i] = 0.0f; + mdct_wind_steep[127 - i] = 1.0f; + } I think you could use ff_sine_64 directly if you modify ff_atrac3p_imdct() doing something like atrac3p_dsp.vector_fmul(pOut+64, pOut+64, ff_sine_64, 64); for the steep window case. + + /* generate amplitude mantissas table */ + for (i = 0; i < 16; i++) + amp_mant_tab[i] = (1.0f / 15.13f) * (i + 1); +} + I think this can be done efficiently without a table. + /* Hann windowing for non-faded wave signals */ + if (tones_now->num_wavs && tones_next->num_wavs && + reg1_env_nonzero && reg2_env_nonzero) { + for (i = 0; i < 128; i++) { + wavreg1[i] *= hann_window[128 - i]; + wavreg2[i] *= hann_window[i]; + } + } else { + if (tones_now->num_wavs && !tones_now->curr_env.has_stop_point) + for (i = 0; i < 128; i++) + wavreg1[i] *= hann_window[128 - i]; + + if (tones_next->num_wavs && !tones_next->curr_env.has_start_point) + for (i = 0; i < 128; i++) + wavreg2[i] *= hann_window[i]; + } + + /* Overlap and add to residual */ + for (i = 0; i < 128; i++) + out[i] += wavreg1[i] + wavreg2[i]; +} I think we have DSP functions for multiplying and adding floats. +void ff_atrac3p_ipqf(FFTContext *dct_ctx, Atrac3pIPQFChannelCtx *hist, + float *in, float *out) +{ + int i, s, sb, t, pos_now, pos_next; + DECLARE_ALIGNED(32, float, idct_in)[ATRAC3P_SUBBANDS]; + DECLARE_ALIGNED(32, float, idct_out)[ATRAC3P_SUBBANDS]; + + memset(out, 0, ATRAC3P_FRAME_SAMPLES * sizeof(*out)); + + for (s = 0; s < ATRAC3P_SUBBAND_SAMPLES; s++) { + /* pick up one sample from each subband */ + for (sb = 0; sb < ATRAC3P_SUBBANDS; sb++) + idct_in[sb] = in[sb * ATRAC3P_SUBBAND_SAMPLES + s]; + + /* Calculate the sine and cosine part of the PQF using IDCT-IV */ + dct_ctx->imdct_half(dct_ctx, idct_out, idct_in); IDCT-IV or IMDCT? Finally, I'll try to find time to look at the rest of the patch better. -Vitor _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
