[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Michael Niedermayer
michaelni
Sat Nov 3 00:49:21 CET 2007
On Wed, Oct 17, 2007 at 04:37:16PM +0100, Ian Caulfield wrote:
> Updated decoder patch attached.
[...]
> + int out_buf_remaining;
this variable is useless, its used only at the top of read_access_unit()
and only set before the call to read_access_unit(), check the buffer before
calling a functon filling it, not storing the buffer size in some context
and doing the check in the function!
[...]
> + uint8_t restart_header_present[MAX_SUBSTREAMS];
this one is used in various places but it looks like the code using this does
nothing besides printing useless debug messages
[...]
> +/** Calculate an 8-bit checksum over a restart header -- a non-multiple-of-8
> + * number of bits, starting two bits into the first byte of buf.
> + */
> +
> +static uint8_t mlp_restart_checksum(const uint8_t *buf, unsigned int bit_size)
> +{
> + int i;
> + int num_bytes = (bit_size + 2) / 8;
> +
> + uint8_t crc = crc_1D[buf[0] & 0x3f];
> + crc = av_crc(crc_1D, crc, buf + 1, num_bytes - 2);
> + crc ^= buf[num_bytes - 1];
> +
> + for (i = 0; i < (bit_size + 2) % 8; i++) {
> + if (crc & 0x80)
> + crc = (crc << 1) ^ 0x1D;
> + else
> + crc = crc << 1;
crc<<=1;
if(crc&0x100) crc ^= 0x11D;
and sizeof(crc) > 1
[...]
> +/** Read a sample, consisting of either, both or neither of entropy-coded MSBs
> + * and plain LSBs
> + */
> +
> +static inline int read_huff(MLPDecodeContext *m, GetBitContext *gbp,
> + unsigned int substr, unsigned int channel)
> +{
> + int codebook = m->codebook[channel];
> + int quant_step_size = m->quant_step_size[substr][channel];
> + int lsb_bits = m->huff_lsbs[channel] - quant_step_size;
> + int sign_shift;
> + int msbs = 0, lsbs = 0;
> + int result;
> +
> + if (codebook > 0)
> + msbs = get_vlc2(gbp, huff_vlc[codebook-1].table,
> + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
> +
> + if (lsb_bits > 0)
> + lsbs = get_bits(gbp, lsb_bits);
> +
> + result = (msbs << lsb_bits) + lsbs;
> + sign_shift = lsb_bits + (codebook ? 2 - codebook : -1);
> +
> + if (sign_shift >= 0)
> + result += (-1) << sign_shift;
> +
> + result += m->huff_offset[channel];
> +
> + return result << quant_step_size;
int result=0;
if (codebook > 0)
result = get_vlc2(gbp, huff_vlc[codebook-1].table,
VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
if(lsb_bits > 0)
result = (result<<lsb_bits) + get_bits(gbp, lsb_bits);
return (result + m->huff_offset[channel]) << quant_step_size;
note, yes you can merge the sign_shift stuff into huff_offset
[...]
> + m->stream_type = mh.stream_type;
> +
> + if (m->stream_type == 0xbb) {
as this is the only use of the variable, this doesnt need to be in the context
[...]
> + if (mh.group2_samplerate != 0 && mh.group2_samplerate != mh.group1_samplerate) {
the != 0 is redundant
[...]
> + if (m->num_substreams > MAX_SUBSTREAMS)
> + av_log(m->avctx, AV_LOG_INFO,
> + "Number of substreams %d is more than maximum supported by "
> + "decoder. Only the first %d will be decoded. Please provide a "
> + "sample of this file to the FFmpeg development list.\n",
> + m->num_substreams, MAX_SUBSTREAMS);
this should do a return -1 after the message, its much more effective at
convincing users to send us a sample ...
this would also avoid the FFMIN below and possibly the max_decoded_substream
variable as well?
> +
> + m->max_decoded_substream = FFMIN(m->num_substreams, MAX_SUBSTREAMS) - 1;
just asking (id have to check the other patch to know for sure) ...
num_substreams cannot be 0 right? because if it can then this is not
good at all
[...]
> + for (substr = 0; substr < m->num_substreams; substr++)
> + m->restart_seen[substr] = 0;
memset()
[...]
> + if (m->max_channel[substr] >= MAX_CHANNELS
> + || m->max_matrix_channel[substr] >= MAX_CHANNELS)
> + {
> + if (substr > 0) {
> + av_log(m->avctx, AV_LOG_INFO,
> + "Substream %d contains more channels than the maximum "
> + "supported by this decoder (%d). Only substreams up to %d "
> + "will be decoded. Please provide a sample of this file "
> + "to the FFmpeg development list.\n",
> + substr, MAX_CHANNELS, substr - 1);
> + m->max_decoded_substream = substr - 1;
> + m->avctx->channels = m->max_channel[substr - 1] + 1;
> + } else {
> + av_log(m->avctx, AV_LOG_INFO,
> + "This stream contains more channels than the maximum "
> + "supported by this decoder (%d). Please provide a sample "
> + "of this file to the FFmpeg development list.\n",
> + MAX_CHANNELS);
> + return -1;
> + }
> + }
so after this max_channel and max_matrix_channel are invalid, is there
anything that stops them from being used? if no then this
check is as good as no check at all
you could as well print
"please reformat your harddisk and reinstall everything your system has
been hacked into"
also if i really didnt miss something and this can lead to writing file
data over the end of arrays then please recheck your code, yes all of it
for similar issues
[...]
> + m->max_lsbs[substr] = get_bits(gbp, 5);
> + m->max_bits1[substr] = get_bits(gbp, 5);
> + m->max_bits2[substr] = get_bits(gbp, 5);
> + av_log(m->avctx, AV_LOG_DEBUG, "max_lsbs %d max_bits1 %d max_bits2 %d\n",
> + m->max_lsbs[substr], m->max_bits1[substr], m->max_bits2[substr]);
these 3 variables are not used anywhere else, they dont belong into
the context!
[...]
> + for (ch = 0; ch <= m->max_matrix_channel[substr]; ch++) {
> + m->ch_assign[substr][ch] = get_bits(gbp, 6);
> + av_log(m->avctx, AV_LOG_DEBUG, "ch_assign[%d][%d] = %d\n",
> + substr, ch, m->ch_assign[substr][ch]);
> + }
this is not used anywhere
[...]
> + for (i = 0; i < order; i++) {
> + m->filter_coeff[channel][filter][i] =
> + get_sbits(gbp, coeff_bits) << coeff_shift;
> + av_log(m->avctx, AV_LOG_DEBUG, " coefficient %f\n",
> + ((float)m->filter_coeff[channel][filter][i])
> + / (1 << m->filter_coeff_q[channel][filter]));
i think dprintf() would be better for these as it would not waste
cpu time or memory if debuging is not wanted
the same applies to many other av_log()
[...]
> + uint32_t tmp;
> + uint32_t seed = m->noisegen_seed[substr];
> +
> + for (i = 0; i < m->access_unit_size_pow2; i++) {
> + tmp = seed >> 15;
uint32_t tmp= seed >> 15;
[...]
> +/** Write the audio data into the output buffer.
> + */
> +
> +static int output_data(MLPDecodeContext *m, unsigned int substr)
> +{
> + unsigned int i, ch;
> + int32_t sample;
> + int16_t *sample_buf16 = (int16_t*)(m->out_buf + m->bytes_output);
> +
> +#ifdef CONFIG_AUDIO_NONSHORT
> + int32_t *sample_buf32 = (int32_t*)(m->out_buf + m->bytes_output);
> +
> + if (m->avctx->sample_fmt == SAMPLE_FMT_S32) {
> + for (i = 0; i < m->blockpos[substr]; i++) {
> + for (ch = 0; ch <= m->max_channel[substr]; ch++) {
> + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch];
> + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch;
> + *sample_buf32++ = sample << 8;
> + m->bytes_output += 4;
> + }
> + }
> + } else
> +#endif
> + {
> + for (i = 0; i < m->blockpos[substr]; i++) {
> + for (ch = 0; ch <= m->max_channel[substr]; ch++) {
> + sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch];
> + m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch;
> + *sample_buf16++ = (sample) >> 8;
> + m->bytes_output += 2;
> + }
> + }
> + }
> +
> + return 0;
> +}
static inline void output_data_internal(MLPDecodeContext *m, unsigned int substr, int is32)
{
unsigned int i, ch;
for (i = 0; i < m->blockpos[substr]; i++) {
for (ch = 0; ch <= m->max_channel[substr]; ch++) {
int sample = m->sample_buffer[i][ch] << m->output_shift[substr][ch];
m->lossless_check_data[substr] ^= (sample & 0xffffff) << ch;
if(is32) *((int32_t*)m->out_ptr)++ = sample << 8;
else *((int16_t*)m->out_ptr)++ = sample >> 8;
}
}
}
static void output_data_internal(MLPDecodeContext *m, unsigned int substr)
{
if(m->avctx->sample_fmt == SAMPLE_FMT_S32) output_data_internal(m, substr, 1);
else output_data_internal(m, substr, 0);
}
[...]
> + while (buf_size >= 4) {
> + scratch ^= *((const uint32_t*)buf);
> + buf += 4;
> + buf_size -= 4;
> + }
for(; buf_size >= 4; buf_size -= 4){
}
or
for(i=0; i < buf_size/4; i++)
scratch ^= ((const uint32_t*)buf)[i];
[...]
> + if ((show_bits_long(&gb, 31) << 1) == 0xf8726fba) {
hmmmm
the <<1 is useless, shift your constant!
[...]
> + if (!m->in_sync)
> + return length * 2;
please explain this, didnt your parser already drop all "not in sync" parts?
[...]
> + extraword_present = get_bits1(&gb);
> + restart_absent = get_bits1(&gb);
> + checkdata_present = get_bits1(&gb);
the get_bits1 can be vertically aligned
[...]
> + } while ((get_bits_count(&gb) < m->substream_data_len[substr] * 16)
> + && get_bits1(&gb) == 0);
substream_data_len seems to be only used in this function so it doesnt belong
into the context
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071103/02961692/attachment.pgp>
More information about the ffmpeg-devel
mailing list