[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Michael Niedermayer
michaelni
Tue Dec 11 00:20:07 CET 2007
On Tue, Dec 04, 2007 at 03:50:50PM +0000, Ian Caulfield wrote:
> On Dec 4, 2007 3:03 PM, Ian Caulfield <ian.caulfield at gmail.com> wrote:
> > Hi
> >
> > Another revision of the patch attached. Hopefully creeping towards
> > acceptability :)
>
> Updated version attached - fixes a typo and some warnings that crept in.
[...]
> +#define SAMPLE_MESSAGE \
> + "Please file a bug report following the instructions at " \
> + "http://ffmpeg.mplayerhq.hu/bugreports.html and include " \
> + "a sample of this file."
very ugly use of preprocessor and wasting memory by duplication of slight
variations of this
please use:
static const char *sample_message= ...
[...]
> + uint8_t data_check_present[MAX_SUBSTREAMS];
> +// uint8_t ch_assign[MAX_SUBSTREAMS][MAX_CHANNELS];
why is this commented out? if its unused it should be removed
[...]
> +static const uint8_t huffman_tables[3][18][2] = {
> + { /* Huffman table 0, -7 - +10 */
> + {0x01, 9}, {0x01, 8}, {0x01, 7}, {0x01, 6}, {0x01, 5}, {0x01, 4}, {0x01, 3},
> + {0x04, 3}, {0x05, 3}, {0x06, 3}, {0x07, 3},
> + {0x03, 3}, {0x05, 4}, {0x09, 5}, {0x11, 6}, {0x21, 7}, {0x41, 8}, {0x81, 9},
> + }, { /* Huffman table 1, -7 - +8 */
> + {0x01, 9}, {0x01, 8}, {0x01, 7}, {0x01, 6}, {0x01, 5}, {0x01, 4}, {0x01, 3},
> + {0x02, 2}, {0x03, 2},
> + {0x03, 3}, {0x05, 4}, {0x09, 5}, {0x11, 6}, {0x21, 7}, {0x41, 8}, {0x81, 9},
> + }, { /* Huffman table 1, -7 - +7 */
Huffman table 2 i would guess ...
[...]
> +/** Initialize static data, constant between all invocations of the codec. */
> +
> +static void init_static()
> +{
> + static int done = 0;
> +
> + if (!done) {
> + init_vlc(&huff_vlc[0], VLC_BITS, 18,
> + &huffman_tables[0][0][1], 2, 1,
> + &huffman_tables[0][0][0], 2, 1, 1);
> + init_vlc(&huff_vlc[1], VLC_BITS, 16,
> + &huffman_tables[1][0][1], 2, 1,
> + &huffman_tables[1][0][0], 2, 1, 1);
> + init_vlc(&huff_vlc[2], VLC_BITS, 15,
> + &huffman_tables[2][0][1], 2, 1,
> + &huffman_tables[2][0][0], 2, 1, 1);
> +
> + av_crc_init(crc_63, 0, 8, 0x63, sizeof(crc_63));
> + av_crc_init(crc_1D, 0, 8, 0x1D, sizeof(crc_1D));
> +
> + done = 1;
> + }
static int done is unneeded you can check some entry from one of the
tables
[...]
> + for (i = 0; i < (bit_size + 2) % 8; i++) {
&7 is faster than %8 depending on how stupid the compiler is
[...]
> + if (sign_shift >= 0)
> + m->sign_huff_offset[ch] += (-1) << sign_shift;
-= 1 << sign_shift
[...]
> +/** Initialize the decoder. */
> +
> +static int mlp_decode_init(AVCodecContext *avctx)
> +{
> + MLPDecodeContext *m = avctx->priv_data;
> +
> + init_static();
> + m->avctx = avctx;
> + memset(m->lossless_check_data, 0xff, sizeof(m->lossless_check_data));
> + return 0;
> +}
is there a special reason why init_static() is a seperate function?
> +
> +/** Read a major sync info header - contains high level information about
> + * the stream - sample rate, channel arrangement etc. Most of this
> + * information is not actually necessary for decoding, only for playback.
> + */
> +
> +static int read_major_sync(MLPDecodeContext *m, const uint8_t *buf,
> + unsigned int buf_size)
> +{
> + MLPHeaderInfo mh;
> +
> + if (ff_mlp_read_major_sync(m->avctx, &mh, buf, buf_size) != 0)
> + return -1;
> +
> + if (mh.stream_type == 0xbb) {
> + if (mh.group1_bits == 0) {
> + av_log(m->avctx, AV_LOG_ERROR, "Invalid/unknown bits per sample\n");
> + return -1;
> + }
> + if (mh.group2_bits > mh.group1_bits) {
> + av_log(m->avctx, AV_LOG_ERROR,
> + "Channel group 2 cannot have more bits per sample than group 1\n");
> + return -1;
> + }
> +
> + if (mh.group2_samplerate && mh.group2_samplerate != mh.group1_samplerate) {
> + av_log(m->avctx, AV_LOG_ERROR,
> + "Channel groups with differing sample rates not currently supported\n");
> + return -1;
> + }
> + }
there is no reason to skip
these checks if stream_type differs from 0xbb, maybe they are guranteed to be
set to safe values if stream_type is not 0xBB and or not used if
its 0xbb currently but what if somone discovers a new stream type which uses
them and changes the parser to set them? it cant be expected that every
change to the code is accompanied by a 10hour review of the whole code
i repeat the checks should be where the variables are read ideally
there are 2 kinds of values the ones which are valid, your decoder should
support them, and the ones which are not valid, they should be rejected
during reading the header
also you can easily just pass a check_stuff flag as argument to
ff_mlp_read_major_sync() if you dont want it to check stuff in the parser
but that makes no sense
[...]
> + dprintf(m->avctx, "Filter %d, order %d\n", filter, order);
please merge these hundreads of dprintf() in few proper
if(debug & FF_DEBUG_*)
av_log()
or dprintf() instead of av_log() for the more obscure/less usefull ones
having this debug stuff interleaved with normal code makes reading it
harder ...
[...]
> + "%s filter coeff_bits must be between 1 and 16\n",
> + filter ? "IIR" : "FIR");
%cIR filter coeff_bits must be between 1 and 16\n",
filter ? 'I' : 'F'
[...]
> + for (i = 0; i < m->filter_order[channel][0]; i++)
> + accum += (int64_t)m->filter_state[channel][0][i] *
> + m->filter_coeff[channel][0][i];
> +
> + for (i = 0; i < m->filter_order[channel][1]; i++)
> + accum += (int64_t) m->filter_state[channel][1][i] *
> + m->filter_coeff[channel][1][i];
for(j=0; j<2; j++)
for (i = 0; i < m->filter_order[channel][j]; i++)
accum += (int64_t)m->filter_state[channel][j][i] *
m->filter_coeff[channel][j][i];
[...]
> + uint32_t tmp;
> + uint32_t seed = m->noisegen_seed[substr];
> + unsigned int maxchan = m->max_matrix_channel[substr];
> +
> + for (i = 0; i < m->blockpos[substr]; i++) {
> + m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << m->noise_shift[substr];
> + m->sample_buffer[i][maxchan+2] = ((int8_t)(seed >> 7)) << m->noise_shift[substr];
> +
> + tmp = seed >> 7;
> + seed = (seed << 16) ^ tmp ^ (tmp << 5);
> + seed &= 0x7fffff;
> + }
uint32_t seed = m->noisegen_seed[substr];
unsigned int maxchan = m->max_matrix_channel[substr];
for (i = 0; i < m->blockpos[substr]; i++) {
uint16_t seed_shr7 = seed >> 7;
m->sample_buffer[i][maxchan+1] = ((int8_t)(seed >> 15)) << m->noise_shift[substr];
m->sample_buffer[i][maxchan+2] = ((int8_t) seed_shr7 ) << m->noise_shift[substr];
seed = (seed << 16) ^ seed_shr7 ^ (seed_shr7 << 5);
}
> +
> + m->noisegen_seed[substr] = seed;
> +}
> +
> +/** Generate a block of noise, used when restart_sync == 0x31eb. */
> +
> +static void generate_noise_2(MLPDecodeContext *m, unsigned int substr)
> +{
> + unsigned int i;
> + uint32_t seed = m->noisegen_seed[substr];
> +
> + for (i = 0; i < m->access_unit_size_pow2; i++) {
> + uint32_t tmp = seed >> 15;
> + m->noise_buffer[i] = noise_table[tmp];
> + seed = (seed << 8) ^ tmp ^ (tmp << 5);
> + seed &= 0x7fffff;
> + }
for (i = 0; i < m->access_unit_size_pow2; i++) {
uint8_t tmp = seed >> 15;
m->noise_buffer[i] = noise_table[tmp];
seed = (seed << 8) ^ tmp ^ (tmp << 5);
}
[...]
> +static void output_data_internal(MLPDecodeContext *m, unsigned int substr,
> + uint8_t *data, unsigned int *data_size, int is32)
> +{
> + unsigned int i, ch = 0;
> + int32_t sample;
> + int32_t *data_32 = (int32_t*) data;
> + int16_t *data_16 = (int16_t*) data;
> +
> + 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];
int sample = ...
theres no need to preserve the value outside the loop ...
[...]
> +static uint8_t calculate_parity(const uint8_t *buf, unsigned int buf_size)
> +{
> + uint32_t scratch = 0;
> +
> + for (; buf_size >= 4; buf_size -= 4) {
> + scratch ^= *((const uint32_t*)buf);
> + buf += 4;
> + }
for(;buf < buf_end-3; buf+=4)
scratch ^= *((const uint32_t*)buf);
1 add less
> + scratch ^= scratch >> 16;
> + scratch ^= scratch >> 8;
> + scratch &= 0xff;
> +
> + while (buf_size > 0) {
> + scratch ^= *buf;
> + buf++;
> + buf_size--;
> + }
for(;buf < buf_end; buf++)
scratch ^= *buf;
[...]
> + if (restart_absent && !m->restart_seen[substr]) {
> + av_log(m->avctx, AV_LOG_ERROR,
> + "No restart header indicated for substream %d", substr);
> + goto error;
> + }
what is this check and the whole complicated restart_seen logic good
for?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker, user
questions for the command line tools ffmpeg, ffplay, ... as well as questions
about how to use libav* should be sent to the ffmpeg-user mailinglist.
-------------- 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/20071211/bc60b7fe/attachment.pgp>
More information about the ffmpeg-devel
mailing list