[FFmpeg-devel] [PATCH] MLP/TrueHD decoder

Michael Niedermayer michaelni
Sun Dec 2 03:28:33 CET 2007


On Sat, Dec 01, 2007 at 11:49:17PM +0000, Ian Caulfield wrote:
> It appears I attached an older version of the patch in my last email,
> sorry. I've attached an up-to-date patch (hopefully) addressing
> Diego's comments.
> 
> On Dec 1, 2007 10:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
> > > On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > >
> > > As well as an error being returned, the "have parameters" flag in the
> > > context is cleared, causing all packets to be rejected until we get a
> > > (valid) sync packet.
> >
> > there is no variable, constant or otherwise called "have parameters" or have*
> 
> The variable "has_params" is used (previously called in_sync).

rather call it params_valid please
because if its 0 they are not, also this design is fragile, every future
change to the codec would have to be checked that it doesnt use anything
from the context if has_params != 1

a memset(0) of the whole context is minimum if you want to keep this design
and want to see it applied to svn, but it would be much better not to set
critical variables to invalid values to begin with or overwrite them
individually with harmless values if they have been filled with something
out of range


[...]

> +    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;

could you please, remove this fraile hack and just return -1 if the file
contains more channels than the decoder supports


[...]
> +            av_log(m->avctx, AV_LOG_ERROR,
> +                   "Non 1:1 channel assignments are used in this stream. "
> +                   "Please send a sample of this file to the FFmpeg "
> +                   "development list.\n");

no! upload to the ftp and send a mail pointing to the file to the list


[...]
> +    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;
> +    }

buffer overflow, access_unit_size_pow2 is not checked for validity
and the buffer is smaller than the max this variable can be

please check the source of the WHOLE decoder for such buffer overflows!
and add proper checks


[...]
> +    if (show_bits_long(&gb, 31) == (0xf8726fba >> 1)) {
> +        dprintf(m->avctx, "Found major sync\n");
> +        if (read_major_sync(m, buf + 4, buf_size - 4) < 0)
> +            goto error;
> +        skip_bits_long(&gb, 28 * 8);
> +    }
> +
> +    if (!m->has_params)
> +    {

{ placement is inconsistant

[...]


> +static int mlp_decode_frame(AVCodecContext *avctx,
> +                            void *data, int *data_size,
> +                            uint8_t *buf, int buf_size)
> +{
> +    MLPDecodeContext *m = avctx->priv_data;
> +    return read_access_unit(m, data, data_size, buf, buf_size);
> +}

read_access_unit is senseless, you can just put its code in here

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20071202/deff9210/attachment.pgp>



More information about the ffmpeg-devel mailing list