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

[Paul Kendall]

On Monday 25 May 2009 09:45:42 pm Reimar D?ffinger wrote:
> 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"?
unsigned, yes

>
> > +    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);
> ?
good idea

>
> > +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)
>
much nicer, agreed

> 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;
> ?
done

>
> > +        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.
new copy_bits function at top of file

>
> > +    // 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.
>
The method needs to return the actual number of bits read/copied
as it subtracts that amount from the count on return in one of the cases
that it is used.

Perhaps this would be better/safer?
+    // count the extradata
+    ret = put_bits_count(&o);
+
+    flush_put_bits(&o);
+    filter->extrasize = put_bits_count(&o);

> > +    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.
>
done

> > +    if (filter->audio_mux_version_A == 0) {
>
> if (!filter->audio_mux_version_A) {
> ?
>
done

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

> > +            // skip left over bits
> > +            while (ascLen > 16) {
> > +                skip_bits(b, 16);
> > +                ascLen -= 16;
> > +            }
> > +            skip_bits(b, ascLen);
>
> skip_bits_long(ascLen);
> ?
>
much better this way

> > +        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.
>
certainly does :-)

> > +                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?
Not needed for DVB-T, so I just tossed out the variable and the assignment.

> 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...
>
done

> > +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.
>
all function names changed to lower with _'s

> > +    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.
>
In the read_audio_sync_stream function the size is checked to make sure
it's big enough for the header, then once it's got the mux_length it checks
that it's bigger than that too, so no overflow should be possible.

> > +    /* parser only codecs */
>
> parser-only

Yes, the comment relates to the other patch I mentioned about separate
parser selection.

LATM is implemented a parser and a bit stream filter, there is no codec.
With DVB-T mpeg2-ts streams there is a stream type 0x11 which has LATM
with encapsulated AAC audio data.

So the other patch mentioned allows mpegts.c to set the parser separate
from the codec. It also needs Baptiste's patch for selecting the bit stream
filter automatically as well. I have all this hooked up and can get mpeg-ts
data from my DVB-T card in NZ and play it quite happily with fplay with all
these patches in place.

Thanks for all the feedback. I'm pretty sure I did all that you and Diego
asked, please let me know if theres other tweaks, formatting changes
required.

If all this goes through, and Baptiste's patch and the parser selection
patch, I'll have a patch to mpegts.c and mpeg.c which will select the LATM
parser and LATM bsf for stream type 0x11.

Anyway, new patch attached. Thanks again.

Cheers,
Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: latm-2.patch
Type: text/x-patch
Size: 16997 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090526/c797082b/attachment.bin>