[FFmpeg-devel] [libav-devel] [PATCH] Add ATRAC3+ decoder

Vitor Sessak vitor1001 at gmail.com
Fri Oct 11 21:18:44 CEST 2013


On Thu, Oct 10, 2013 at 9:14 PM, Maxim Polijakowski <max_pole at gmx.de> 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


More information about the ffmpeg-devel mailing list