[Ffmpeg-devel] Re: [PATCH] MXF demuxer

[Baptiste Coudurier]

Michael Niedermayer wrote:
> Hi
> 
> On Mon, Jul 17, 2006 at 04:29:44PM +0200, Baptiste Coudurier wrote:
> [...]
>> +static int64_t klv_decode_ber_length(ByteIOContext *pb)
>> +{
>> +    int64_t size = 0;
>> +    uint8_t length = get_byte(pb);
>> +    int type = length >> 7;
>> +
>> +    if (type) { /* long form */
>> +        int bytes_num = length & 0x7f;
>> +        /* SMPTE 379M 5.3.4 guarantee that bytes_num must not exceed 8 bytes */
>> +        assert(bytes_num <= 8);
> 
> using assert() to check stuff from a stream for validity is unacceptable
> its like printf() aborting your application due to a spelling error

I assume bytes_num is <= 8. then It will overflow size. It's a
"security" issue. I'll use av_assert.

> [...]
> 
>> +}
>> +
>> +static void klv_read_packet(KLVPacket *klv, ByteIOContext *pb)
>> +{
>> +    klv->offset = url_ftell(pb);
>> +    get_buffer(pb, klv->key, 16);
>> +    klv->length = klv_decode_ber_length(pb);
>> +}
>> +
>> +static int mxf_get_stream_index(AVFormatContext *s, KLVPacket *klv)
>> +{
>> +    int id = BE_32(klv->key + 12); /* SMPTE 379M 7.3 */
> 
> cant this be solved cleaner? (= be read with the normal get_* functions)
> if no, then some DECLARE_ALIGNED must be added

Thanks, I will DECLARD_ALIGNED.

> [...]
>> +static int mxf_read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    KLVPacket klv;
>> +
>> +    while (!url_feof(&s->pb)) {
>> +        klv_read_packet(&klv, &s->pb);
>> +        PRINT_KEY(klv.key);
> 
> unconditional debuging code ...

Codecs use that too ? What should I use if I want to debug my demuxer ?
Without defining DEBUG of course.

> [...]
>> +static void mxf_read_metadata_source_package(MXFContext *mxf, KLVPacket *klv)
>> +{
>> +    ByteIOContext *pb = &mxf->fc->pb;
>> +    int tracks_count;
>> +    int bytes_read = 0;
>> +    int i;
>> +
>> +    while (bytes_read < klv->length) {
>> +        int tag = get_be16(pb);
>> +        int size = get_be16(pb); /* KLV specified by 0x53 */
>> +        
>> +        switch (tag) {
>> +        case 0x4403:
>> +            tracks_count = get_be32(pb);
>> +            mxf->tracks_count += tracks_count; /* op2a contains multiple source packages */
>> +            mxf->tracks = av_realloc(mxf->tracks, mxf->tracks_count * sizeof(*mxf->tracks));
> 
> the multiply overflows ...

Right, right, I miss basics, sorry.


> [...]
> 
> why are there so many tables? isnt one enough?
> 

Hell, no. MXF is a bloated mess.

>> +
>> +static void mxf_resolve_track_descriptor(MXFContext *mxf)
>> +{
>> +    uint32_t container_label;
>> +    uint32_t essence_label;
>> +    int i, j;
>> +    
>> +    for (i = 0; i < mxf->descriptors_count; i++) {
>> +        for (j = 0; j < mxf->tracks_count; j++) {
>> +            AVStream *st = mxf->tracks[j].stream;
>> +            MXFDescriptor *desc = &mxf->descriptors[i];
>> +            
>> +            if ((desc->linked_track_id == -1 && st->codec->codec_type == desc->codec_type)
>> +                || desc->linked_track_id == mxf->tracks[j].track_id) {
>> +                if (st->codec->codec_type == CODEC_TYPE_AUDIO) {
>> +                    st->codec->channels = desc->channels;
>> +                    st->codec->bits_per_sample = desc->bits_per_sample;
>> +                    st->codec->block_align = desc->block_align;
>> +                    assert(desc->sample_rate.num % desc->sample_rate.den == 0);
> 
> another assert which checks the input i think

There are some from you in mov.c, sorry, I was just inspired :(
I don't understand the rationale here.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312