[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Michael Niedermayer
michaelni
Sun Jan 6 23:17:00 CET 2008
On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
> Hi,
>
> New version attached, hopefully addressing most of your comments.
>
> On Dec 10, 2007 11:20 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> > On Tue, Dec 04, 2007 at 03:50:50PM +0000, Ian Caulfield wrote:
> > [...]
> > > +/** 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?
>
> Not really - I can fold it in if you'd like.
>
> > [...]
> > > + 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 ...
>
> I've deleted a lot of the dprintfs that weren't especially useful to
> me any more.
>
> >
> > [...]
> > > + 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?
>
> The main point is to detect if the restart header containing the
> needed decoding parameters is missing and to error out if so. I've
> reworked the check to be somewhat saner.
[...]
> +typedef struct MLPDecodeContext {
> + AVCodecContext *avctx;
> +
> + uint8_t params_valid;
> + uint8_t max_decoded_substream;
> + int access_unit_size;
> + int access_unit_size_pow2;
> + uint8_t restart_seen[MAX_SUBSTREAMS];
> +
> + uint8_t num_substreams;
> +
> + //@{
> + /** Restart header data */
> + uint16_t restart_sync[MAX_SUBSTREAMS];
shouldnt this rather be called sync_word ?
> + uint8_t min_channel[MAX_SUBSTREAMS];
> + uint8_t max_channel[MAX_SUBSTREAMS];
> + uint8_t max_matrix_channel[MAX_SUBSTREAMS];
> + uint8_t noise_shift[MAX_SUBSTREAMS];
> + uint32_t noisegen_seed[MAX_SUBSTREAMS];
> + uint8_t data_check_present[MAX_SUBSTREAMS];
> + uint8_t param_presence_flags[MAX_SUBSTREAMS];
> + //@}
> +
> + //@{
> + /** Matrix data */
> + uint8_t num_primitive_matrices[MAX_SUBSTREAMS];
> + uint8_t matrix_ch[MAX_SUBSTREAMS][MAX_MATRICES];
> + uint8_t lsb_bypass[MAX_SUBSTREAMS][MAX_MATRICES];
> + int32_t matrix_coeff[MAX_SUBSTREAMS][MAX_MATRICES][MAX_CHANNELS+2];
> + uint8_t matrix_noise_shift[MAX_SUBSTREAMS][MAX_MATRICES];
> + //@}
> +
> + uint16_t blocksize[MAX_SUBSTREAMS];
> + uint16_t blockpos[MAX_SUBSTREAMS];
> + int8_t output_shift[MAX_SUBSTREAMS][MAX_CHANNELS];
> + uint8_t quant_step_size[MAX_SUBSTREAMS][MAX_CHANNELS];
> +
> + //@{
> + /* Filter data. Filter 0 is an FIR filter, filter 1 IIR. */
> + uint8_t filter_order[MAX_CHANNELS][2];
> + uint8_t filter_coeff_q[MAX_CHANNELS][2];
> + int32_t filter_coeff[MAX_CHANNELS][2][MAX_FILTER_ORDER];
> + int32_t filter_state[MAX_CHANNELS][2][MAX_FILTER_ORDER];
> + //@}
> +
> + //@{
> + /** Sample data coding infomation */
> + int16_t huff_offset[MAX_CHANNELS];
> + int32_t sign_huff_offset[MAX_CHANNELS];
> + uint8_t codebook[MAX_CHANNELS];
> + uint8_t huff_lsbs[MAX_CHANNELS];
> + //@}
> +
> + int32_t lossless_check_data[MAX_SUBSTREAMS];
> +
> + int8_t noise_buffer[MAX_BLOCKSIZE_POW2];
> + int8_t bypassed_lsbs[MAX_BLOCKSIZE][MAX_CHANNELS];
> + int32_t sample_buffer[MAX_BLOCKSIZE][MAX_CHANNELS+2];
> +} MLPDecodeContext;
some comments explaing what each var is, would be nice
[...]
> +static inline void calculate_sign_huff(MLPDecodeContext *m, unsigned int substr,
> + unsigned int ch)
> +{
> + int lsb_bits = m->huff_lsbs[ch] - m->quant_step_size[substr][ch];
> + int sign_shift = lsb_bits + (m->codebook[ch] ? 2 - m->codebook[ch] : -1);
> +
> + m->sign_huff_offset[ch] = m->huff_offset[ch];
> +
> + if (sign_shift >= 0)
> + m->sign_huff_offset[ch] -= 1 << sign_shift;
> +}
> +
> +/** 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 result = 0;
> +
> + if (codebook > 0)
> + result = get_vlc2(gbp, huff_vlc[codebook-1].table,
> + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
the -7 can be merged into sign_huff_offset
[...]
> +static void generate_noise_1(MLPDecodeContext *m, unsigned int substr)
> +{
> + unsigned int i;
> + 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++) {
> + uint8_t tmp = seed >> 15;
please name this seed_shr15
[...]
> +static int read_access_unit(AVCodecContext *avctx, void* data, int *data_size,
> + uint8_t *buf, int buf_size)
> +{
> + MLPDecodeContext *m = avctx->priv_data;
> + GetBitContext gb;
> + unsigned int length, substr;
> + unsigned int substream_start;
> + unsigned int header_size;
> + uint8_t substream_parity_present[MAX_SUBSTREAMS];
> + uint16_t substream_data_len[MAX_SUBSTREAMS];
> +
> + if (buf_size < 2)
> + return 0;
> +
> + if (*data_size < m->avctx->channels * m->access_unit_size
> + * (m->avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2))
> + return -1;
its nice that you check that, it would be nicer i you did after setting
the variables, like that it is just exploitable
[...]
> + skip_bits(&gb, (-get_bits_count(&gb)) & 15);
> + if (show_bits_long(&gb, 32) == 0xd234d234 ||
> + show_bits_long(&gb, 18) == 0xd234e) {
> + skip_bits(&gb, 18);
> + av_log(m->avctx, AV_LOG_INFO, "End of stream indicated\n");
> + if (get_bits1(&gb))
> + m->blockpos[substr] -= get_bits(&gb, 13);
> + else
> + skip_bits(&gb, 13);
exploitable, blockpos overflows and is later used as array limit
(for exmple in rematrix_channels)
iam starting to be scared by your patches, can you _please_ take this more
serious, and check the variables used to decide where to write data to?
all of them, double check your code, if i find another such issue i will not
review this patch again and permanently reject it!
[...9
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20080106/756f763f/attachment.pgp>
More information about the ffmpeg-devel
mailing list