[FFmpeg-devel] [PATCH] ALS decoder

[Michael Niedermayer]

On Sat, Aug 29, 2009 at 01:04:27AM +0200, Thilo Borgmann wrote:
> Michael Niedermayer schrieb:
> > On Wed, Aug 26, 2009 at 08:28:43PM +0200, Thilo Borgmann wrote:
> >> Revision 11 attached.
> > [...]
> >> +/** Reads an ALSSpecificConfig from a buffer into the output struct.
> >> + */
> >> +static av_cold int read_specific_config(ALSDecContext *ctx)
> >> +{
> >> +    GetBitContext gb;
> >> +    uint64_t ht_size;
> >> +    int i, config_offset, crc_enabled;
> >> +    MPEG4AudioConfig m4ac;
> >> +    ALSSpecificConfig *sconf = &ctx->sconf;
> >> +    AVCodecContext *avctx    = ctx->avctx;
> >> +    const uint8_t *buffer    = avctx->extradata;
> >> +    uint32_t samples, als_id;
> >> +
> >> +    init_get_bits(&gb, buffer, avctx->extradata_size * 8);
> >> +
> >> +    config_offset = ff_mpeg4audio_get_config(&m4ac, buffer, avctx->extradata_size);
> >> +
> >> +    if (config_offset < 0)
> >> +        return -1;
> >> +
> >> +    skip_bits_long(&gb, config_offset);
> >> +
> >> +    if (get_bits_left(&gb) < (22 << 3))
> >> +        return -1;
> > 
> > is that the smallest size a valid frame can have? i would have suspect it
> > to be larger
> I don't know. Is the AudioSpecificConfig embedded in something like a
> frame? I don't think there is a minimum size for AudioSpecificConfig and
> determining this by the smallest possible *SpecificConfig would be
> error-prone and would have to be reconsidered whenever 14496-3 changes.
> 

> > Now if it where larger, several of the following checks could be removed
> These also prevent from reading beyond a broken stream and are
> one-timers for decoding a whole stream and therefore speed is not
> relevant, I think.

my concern was mainly with source code complexity here, there are quite a
few checks, ill look at this one again in the next iteration, i must have
somehow mixed extradata and frames up in my logic :)


> 
> 
> > 
> > 
> > [...]
> >> +/** Reads and decodes a Rice codeword.
> >> + */
> >> +static int32_t decode_rice(GetBitContext *gb, unsigned int k)
> >> +{
> >> +    int max   = gb->size_in_bits - get_bits_count(gb) - k;
> >> +    int32_t q = get_unary(gb, 0, max);
> > 
> > i suspect int is fine for these 2 instead of int32_t ?
> Is int at least 32 bits long? k <= 32 and therefore q might have to hold
> up to 32 bits.

int in POSIX is at least 32bits long, yes


> 
> 
> >> +{
> >> +    int i;
> >> +
> >> +    for (i = 0; i < (k + 1) >> 1; i++) {
> > 
> >> +        int64_t tmp1 = cof[    i    ] + ((MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20);
> >> +        int64_t tmp2 = cof[k - i - 1] + ((MUL64(par[k], cof[    i    ]) + (1 << 19)) >> 20);
> >> +
> >> +        if (tmp1 < -INT32_MAX || tmp1 > INT32_MAX ||
> >> +            tmp2 < -INT32_MAX || tmp2 > INT32_MAX)
> >> +            return -1;
> >> +
> >> +        cof[k - i - 1] = tmp2;
> >> +        cof[    i    ] = tmp1;
> > 
> > if we didnt do overflow checks ...
> > 
> > int tmp1        = (MUL64(par[k], cof[k - i - 1]) + (1 << 19)) >> 20;
> > cof[k - i - 1] += (MUL64(par[k], cof[    i    ]) + (1 << 19)) >> 20;
> > cof[    i    ] += tmp1;
> I agree mathematically, but this doesn't work - most likely because of
> the += operator. Used the previous no-check version (no tmp2).

the only failure i can see quickyl is with odd k and the middle value being
transformed twice, that is k-i-1 == i
it might make sense to handle the middle value specially outside the loop


[...]
> > 
> > [...]
> >> +/** Computes the bytes left to decode for the current frame
> >> + */
> >> +static void zero_remaining(unsigned int b, unsigned int b_max,
> >> +                           unsigned int *div_blocks, int32_t *buf)
> >> +{
> > 
> > div_blocks should be const, also this likel applies to other arguments of
> > other functions
> Are there some common FFmpeg-wide rules to declare something const?
> I've done what I think might be useful...

pointers in function arguments should be const when possible as it often
makes understanding the code easier also it might help some compilers
optimizing the code better


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible (turings halting problem).
On any real computer, always possible as a real computer has a finite number
of states N, and will either halt in less than N cycles or never halt.
-------------- 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/20090830/daba04c7/attachment.pgp>