Thank you for your suggestions!
Please find the updated patch in my next email
[PATCH] ATRAC3+ decoder, 2nd try
Below a couple of comments...
On Thu, Oct 10, 2013 at 9:14 PM, Maxim Polijakowski<max_p...@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>
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).
This part has been significantly reworked. The VLC initialization uses
more loops now...
+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.
Done.
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.
*/
Ok, comments will be added/improved later...
+/**
+ * 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.
Done.
+
+ /* 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.
Indeed. The table has been replaced with direct computation.
+ /* 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.
Yes, replaced with vector_fmul()...
+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?
imdct_half = IDCT-IV in this case because there is no overlap...
Best regards
Maxim
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
