[FFmpeg-devel] [PATCH] LATM Bitstream Filter & Parser

[Reimar Döffinger]

On Mon, May 25, 2009 at 09:12:46PM +1200, Paul Kendall wrote:
> +static inline int32_t latm_get_value(GetBitContext *b)

Hm. Are you sure int32_t is the right type, not uint32_t?
Or just "unsigned"?

> +    uint8_t num_bytes = get_bits(b, 2);
> +    int32_t value = 0;
> +    int i;
> +    for (i = 0; i <= num_bytes; i++) {
> +        value <<= 8;
> +        value |= get_bits(b, 8);
> +    }
> +    return value;

return get_bits_long(num_bytes * 8);
?

> +static void readGASpecificConfig(int audio_object_type, GetBitContext *b,
> +                                 PutBitContext *o)

Standard names are gb and pb for Get/PutBitContext. o and b are not
really helpful names anyway, so consistency seems like a good
argument...

> +    put_bits(o, 1, get_bits(b, 1));             // framelen_flag
> +    put_bits(o, 1, depends_on_coder = get_bits(b, 1));
> +    if (depends_on_coder)
> +        put_bits(o, 14, get_bits(b, 14));       // delay
> +    put_bits(o, 1, ext_flag = get_bits(b, 1));

IMO please put the assignment on a different line.

> +    if (audio_object_type == AOT_AAC_SCALABLE
> +            || audio_object_type == AOT_ER_AAC_SCALABLE)

either of

if (audio_object_type == AOT_AAC_SCALABLE ||
    audio_object_type == AOT_ER_AAC_SCALABLE)

or

if (audio_object_type == AOT_AAC_SCALABLE
 || audio_object_type == AOT_ER_AAC_SCALABLE)

looks better IMO.

> +    if (ext_flag) {

Maybe
if (!ext_flag)
    return;
?

> +        if (audio_object_type == AOT_ER_BSAC) {
> +            put_bits(o, 5, get_bits(b, 5));     // numOfSubFrame
> +            put_bits(o, 11, get_bits(b, 11));   // layer_length

And an extra function to copy from GetBitContext to PutBitContext might
make this quite a bit more readable I think.
I also think that the DV encoder uses such stuff, too, though I don't
know if it makes sense to write such a generic function that it
covers both cases.

> +    // count the extradata
> +    ret = put_bits_count(&o);
> +    filter->extrasize = (ret + 7) / 8;
> +
> +    flush_put_bits(&o);

I think you might want to do the flush_put_bits first, since that IIRC
calls align_put_bits (or you can do that manually) and thus you don't
need to do the rounding of extrasize separately.

> +    int audio_mux_version = get_bits(b, 1);
> +    filter->audio_mux_version_A = 0;
> +    if (audio_mux_version == 1)                 // audioMuxVersion

The " == 1" part seems useless.

> +    if (filter->audio_mux_version_A == 0) {

if (!filter->audio_mux_version_A) {
?

> +        if (audio_mux_version == 1) {
> +            // taraFullness
> +            latm_get_value(b);
> +        }

Pointless == 1 and {}

> +            // skip left over bits
> +            while (ascLen > 16) {
> +                skip_bits(b, 16);
> +                ascLen -= 16;
> +            }
> +            skip_bits(b, ascLen);

skip_bits_long(ascLen);
?

> +        if (frame_length_type == 0)
> +            get_bits(b, 8);
> +        else if (frame_length_type == 1)
> +            get_bits(b, 9);
> +        else if (frame_length_type == 3 || frame_length_type == 4
> +                || frame_length_type == 5)
> +            get_bits(b, 6);                     // celp_table_index
> +        else if (frame_length_type == 6 || frame_length_type == 7)
> +            get_bits(b, 1);                     // hvxc_table_index

switch (frame_length_type)
would probably look better.

> +                int esc, tmp;
> +                // other data bits
> +                int64_t other_data_bits = 0;
> +                do {
> +                    esc = get_bits(b, 1);
> +                    tmp = get_bits(b, 8);
> +                    other_data_bits = other_data_bits << 8 | tmp;
> +                } while (esc);

You don't seem to use other_data_bits?
do {
  tmp = get_bits(b, 9);
  other_data_bits = other_data_bits << 8 | (tmp & 0xff);
} while (tmp & 0x100);

still might be better even if.

> +        return mux_slot_length;
> +    } else {
> +        if (filter->frame_length_type == 3
> +                || filter->frame_length_type == 5
> +                || filter->frame_length_type == 7)
> +            get_bits(b, 2);
> +        return 0;
> +    }

No need for an else if the if path always has a return...

> +static int readAudioSyncStream(LATMFilter *filter, GetBitContext *b,
> +                               int size, uint8_t **payload, int *payloadsize)
> +    readAudioMuxElement(filter, b, *payload, payloadsize);

FFmpeg does not usually use camelCase for functions, so e.g. read_audio_sync_stream
would be a more consistent name.

> +    init_get_bits(&b, data, size * 8);
> +    if (!readAudioSyncStream(filter, &b, size, poutbuf, poutbuf_size))
> +        return 0;

Maybe I missed it, but do you check against the input size anywhere?
Note that passing the size to init_get_bits does _not at all_ protect
you against reading beyond the buffer and causing a crash.

> +    /* parser only codecs */

parser-only