Thank you for your suggestions!
Please find the updated patch in my next email
[PATCH] ATRAC3+ decoder, 2nd try
Below a couple of comments...
+/**
+ * Generate canonical VLC table from given descriptor.
+ *
+ * @param[in] cb ptr to codebook descriptor
+ * @param[in] xlat ptr to translation table or NULL
+ * @param[out] out_vlc ptr to vlc table to be generated
+ */
+static av_cold void build_canonical_huff(const uint8_t *cb, const uint8_t
*xlat,
+ VLC *out_vlc)
+{
+ int i, b;
+ uint16_t codes[256];
+ uint8_t bits[256];
+ unsigned code = 0;
+ int index = 0;
+ int min_len = *cb++; // get shortest codeword length
+ int max_len = *cb++; // get longest codeword length
+
+ for (b = min_len; b <= max_len; b++) {
+ for (i = *cb++; i > 0; i--) {
+ bits[index] = b;
+ codes[index] = code++;
+ index++;
+ }
+ code <<= 1;
+ }
+
+ ff_init_vlc_sparse(out_vlc, max_len, index, bits, 1, 1, codes, 2, 2,
+ xlat, 1, 1, 0);
IIRC building VLC might fail (and not only because of wrong codes but also
because of allocation error), so it should be checked.
All VLC tables have been made static, thus there will be no allocation
failure in this context anymore...
[...]
useless checks - ff_free_vlc() will work on empty VLCs fine too
Static VLC tables don't require ff_free_vlc() anymore...
[...]
+
+/**
+ * Decode word length for each quantization unit of a channel.
+ *
+ * @param[in,out] ctx ptr to the decoder context
+ * @param[in] num_channels number of channels to process
+ * @param[in] avctx ptr to the AVCodecContext
+ * @return result code: 0 = OK, -1 = error
+ */
+static int decode_channel_wordlen(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
+ int ch_num, ATRAC3PVlcTabs *vlc_tabs,
+ AVCodecContext *avctx)
+{
+ int i, weight_idx = 0, delta, diff, pos, delta_bits, min_val, flag, ret;
+ VLC *vlc_tab;
+ Atrac3pChanParams *chan = &ctx->channels[ch_num];
+ Atrac3pChanParams *ref_chan = &ctx->channels[0];
+
+ chan->fill_mode = 0;
+
+ switch (get_bits(gb, 2)) { /* switch according to coding mode */
+ case 0: /* coded using constant number of bits */
+ for (i = 0; i < ctx->num_quant_units; i++)
+ chan->qu_wordlen[i] = get_bits(gb, 3);
+ break;
+ case 1:
+ if (ch_num) {
+ if ((ret = num_coded_units(gb, chan, ctx, avctx)) < 0)
+ return ret;
+
+ if (chan->num_coded_vals) {
+ vlc_tab = &vlc_tabs->wl_vlc_tabs[get_bits(gb, 2)];
+
+ for (i = 0; i < chan->num_coded_vals; i++) {
+ delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
probably we can test for delta < 0 here (i.e. a bitstream error)
It looks like I need to check each call to get_vlc2() for errors and
that will mess up the code.
Is there really any case where it may return a negative value?
[...]
+ }
+
+ if (chan->fill_mode == 2) {
+ for (i = chan->num_coded_vals; i < ctx->num_quant_units; i++)
+ chan->qu_wordlen[i] = ch_num ? get_bits1(gb) : 1;
I'd simply memset it and then chan->qu_wordlen[ch_num] = get_bits1(gb);
(if it's in range of course)
Memset would require to convert chan->qu_wordlen (currently int) to
uint8_t and that would be neither significantly faster (because we'are
processing just few elements) nor shorter. Fix me if I'm wrong...
+
+/**
+ * Decode scale factor indexes for each quant unit of a channel.
+ *
+ * @param[in,out] ctx ptr to the decoder context
+ * @param[in] num_channels number of channels to process
+ * @param[in] avctx ptr to the AVCodecContext
+ * @return result code: 0 = OK, -1 = error
+ */
+static int decode_channel_sf_idx(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
+ int ch_num, ATRAC3PVlcTabs *vlc_tabs,
+ AVCodecContext *avctx)
+{
+ int i, weight_idx, delta, diff, num_long_vals,
+ delta_bits, min_val, vlc_sel;
+ VLC *vlc_tab;
+ Atrac3pChanParams *chan = &ctx->channels[ch_num];
+ Atrac3pChanParams *ref_chan = &ctx->channels[0];
+
+ switch (get_bits(gb, 2)) { /* switch according to coding mode */
+ case 0: /* coded using constant number of bits */
+ for (i = 0; i < ctx->used_quant_units; i++)
+ chan->qu_sf_idx[i] = get_bits(gb, 6);
+ break;
+ case 1:
+ if (ch_num) {
+ vlc_tab = &vlc_tabs->sf_vlc_tabs[get_bits(gb, 2)];
+
+ for (i = 0; i < ctx->used_quant_units; i++) {
+ delta = get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1);
+ chan->qu_sf_idx[i] = (ref_chan->qu_sf_idx[i] + delta) & 0x3F;
+ }
+ } else {
+ weight_idx = get_bits(gb, 2);
+ if (weight_idx == 3) {
+ unpack_scalefactor_shape(gb, ctx, chan);
+
+ num_long_vals = get_bits(gb, 5);
+ delta_bits = get_bits(gb, 2);
+ min_val = get_bits(gb, 4) - 7;
+
+ for (i = 0; i < num_long_vals; i++)
+ chan->qu_sf_idx[i] = (chan->qu_sf_idx[i] +
+ get_bits(gb, 4) - 7) & 0x3F;
+
+ /* all others are: min_val + delta */
+ for (i = num_long_vals; i < ctx->used_quant_units; i++)
+ chan->qu_sf_idx[i] = (chan->qu_sf_idx[i] + min_val +
+ (delta_bits ? get_bits(gb, delta_bits) :
0)) & 0x3F;
Maybe define a macro for get_bits0() instead of constant checks?
Maybe even in get_bits.h
This one and several others have been replaced with locally defined
GET_DELTA macro...
[...]
+
+#define CODING_DIRECT get_bits(gb, num_bits)
+
+#define CODING_VLC get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1)
+
+#define CODING_VLC_DELTA \
+ (!i) ? CODING_VLC \
+ : (pred + get_vlc2(gb, delta_vlc->table, \
+ delta_vlc->bits, 1)) & mask; \
+ pred = chan->qu_tab_idx[i]
+
+#define CODING_VLC_DIFF \
+ (ref_chan->qu_tab_idx[i] + \
+ get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1)) & mask
Looks like it could've been used in the previous functions. Or at least
similar macros, it's full of X = (X + Y + get_vlc2()) & mask for instance.
I've just defined a macro for this common operation and replaced all
similar statements with that. Below an example:
#define ADD_VLC_DELTA(gb, vlc_tab, x, mask) \
(x + get_vlc2(gb, vlc_tab->table, vlc_tab->bits, 1)) & mask
chan->qu_wordlen[i] = ADD_VLC_DELTA(gb, vlc_tab, chan->qu_wordlen[i-1], 7);
The resulted code isn't really nice IMHO, so I reverted the changes and
left it as is...
I would gladly change my views if someone would suggest a more elegant
solution...
[...]
+static int decode_gainc_levels(GetBitContext *gb, Atrac3pChanUnitCtx *ctx,
+ int ch_num, int coded_subbands,
+ ATRAC3PVlcTabs *vlc_tabs, AVCodecContext *avctx)
+{
+ int sb, i, delta, delta_bits, min_val, pred;
+ Atrac3pChanParams *chan = &ctx->channels[ch_num];
+ Atrac3pChanParams *ref_chan = &ctx->channels[0];
+
+ switch (get_bits(gb, 2)) { /* switch according to coding mode */
+ case 0: /* fixed-length coding */
+ for (sb = 0; sb < coded_subbands; sb++)
+ for (i = 0; i < chan->gain_data[sb].num_points; i++)
+ chan->gain_data[sb].lev_code[i] = get_bits(gb, 4);
+ break;
+ case 1:
+ if (ch_num) { /* VLC modulo delta to master channel */
+ for (sb = 0; sb < coded_subbands; sb++)
+ for (i = 0; i < chan->gain_data[sb].num_points; i++) {
+ delta = get_vlc2(gb, vlc_tabs->gain_vlc_tabs[5].table,
+ vlc_tabs->gain_vlc_tabs[5].bits, 1);
+ pred = (i >= ref_chan->gain_data[sb].num_points)
+ ? 7 : ref_chan->gain_data[sb].lev_code[i];
+ chan->gain_data[sb].lev_code[i] = (pred + delta) & 0xF;
+ }
+ } else { /* VLC modulo delta to previous */
+ for (sb = 0; sb < coded_subbands; sb++)
+ gainc_level_mode1m(gb, ctx, &chan->gain_data[sb], vlc_tabs);
+ }
+ break;
+ case 2:
+ if (ch_num) { /* VLC modulo delta to previous or clone master */
+ for (sb = 0; sb < coded_subbands; sb++)
+ if (chan->gain_data[sb].num_points > 0) {
+ if (get_bits1(gb))
+ gainc_level_mode1m(gb, ctx, &chan->gain_data[sb],
+ vlc_tabs);
+ else
+ gainc_level_mode3s(&chan->gain_data[sb],
+ &ref_chan->gain_data[sb]);
+ }
+ } else { /* VLC modulo delta to lev_codes of previous subband */
+ if (chan->gain_data[0].num_points > 0)
+ gainc_level_mode1m(gb, ctx, &chan->gain_data[0], vlc_tabs);
+
+ for (sb = 1; sb < coded_subbands; sb++)
+ for (i = 0; i < chan->gain_data[sb].num_points; i++) {
+ delta = get_vlc2(gb, vlc_tabs->gain_vlc_tabs[4].table,
+ vlc_tabs->gain_vlc_tabs[4].bits, 1);
+ pred = (i >= chan->gain_data[sb - 1].num_points)
+ ? 7 : chan->gain_data[sb - 1].lev_code[i];
+ chan->gain_data[sb].lev_code[i] = (pred + delta) & 0xF;
+ }
+ }
+ break;
+ case 3:
+ if (ch_num) { /* clone master */
+ for (sb = 0; sb < coded_subbands; sb++)
+ gainc_level_mode3s(&chan->gain_data[sb],
+ &ref_chan->gain_data[sb]);
+ } else { /* shorter delta to min */
+ delta_bits = get_bits(gb, 2);
+ min_val = get_bits(gb, 4);
+
+ for (sb = 0; sb < coded_subbands; sb++)
+ for (i = 0; i < chan->gain_data[sb].num_points; i++) {
+ delta = delta_bits ? get_bits(gb, delta_bits) : 0;
+ chan->gain_data[sb].lev_code[i] = min_val + delta;
+ if (chan->gain_data[sb].lev_code[i] > 15)
+ return AVERROR_INVALIDDATA;
+ }
This internal operation looks like it can be made a macro or a standalone
function.
You're right - there are several internal operations used again and again:
- decode a fixed-length codeword
- take master signal and add some delta to it
- take previous value and add some delta to it
- take some initial (common) value and add some delta to it
- copy (clone) master signal
The next obvious step would be to write macros for all of these
operations and finally to remove all this code duplication.
Unfortunately, it isn't that easy because each mode adds its own
variation in the usage of these operations.
Incorporating these variations makes the resulting code full of
additional parameters and therefore messy and perhaps even slower.
This doesn't mean that we shouldn't try to split out small common things...
[...]
+
+static int decode_frame(AVCodecContext *avctx, void *data,
+ int *got_frame_ptr, AVPacket *avpkt)
+{
+ const uint8_t *buf = avpkt->data;
+ int buf_size = avpkt->size;
+ ATRAC3PContext *ctx = avctx->priv_data;
+ AVFrame *frame = data;
+ int i, ret, ch_unit_id, ch_block = 0, out_ch_index = 0,
channels_to_process;
+ float **samples_p = (float **)frame->extended_data;
+
+ frame->nb_samples = ATRAC3P_FRAME_SAMPLES;
+ if ((ret = ff_get_buffer(avctx, frame, 0)) < 0) {
+ av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
+ return ret;
+ }
+
+ init_get_bits(&ctx->gb, buf, buf_size * 8);
+
+ if (get_bits1(&ctx->gb)) {
+ av_log(avctx, AV_LOG_ERROR, "Invalid start bit!\n");
+ return AVERROR_INVALIDDATA;
+ }
+
+ while ((ch_unit_id = get_bits(&ctx->gb, 2)) != CH_UNIT_TERMINATOR) {
I'd also check get_bits_left() > 2
Good catch, check added...
Thanks!
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
