[Ffmpeg-devel] [PATCH] MXF demuxer

[Michael Niedermayer]

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


[...]

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


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

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


> +            mxf->descriptors = av_realloc(mxf->descriptors, mxf->tracks_count * sizeof(*mxf->descriptors));
> +            url_fskip(pb, 4); /* useless size of objects, always 16 according to specs */
> +            for (i = mxf->tracks_count - tracks_count; i < mxf->tracks_count; i++) {
> +                mxf->tracks[i].stream = av_new_stream(mxf->fc, 0);
> +                get_buffer(pb, mxf->tracks[i].track_uid, 16);

and data from the file is blindly copied over the end of the array


[...]

> +/* SMPTE RP224 http://www.smpte-ra.org/mdd/index.html */
> +static const CodecTag mxf_sound_essence_labels[] = {
> +    { CODEC_ID_PCM_S16LE, 0x01000000 },/* Uncompressed Sound Coding */
> +    { CODEC_ID_PCM_S16LE, 0x017F0000 },/* Uncompressed Sound Coding */
> +    { CODEC_ID_PCM_S16BE, 0x017E0000 },/* Uncompressed Sound Coding Big Endian*/
> +    { CODEC_ID_PCM_ALAW,  0x02030101 },
> +    { CODEC_ID_AC3,       0x02030201 },
> +  //{ CODEC_ID_MP1,       0x02030104 },
> +    { CODEC_ID_MP2,       0x02030105 },/* MP2 or MP3 */
> +  //{ CODEC_ID_MP2,       0x02030106 },/* MPEG-2 Layer 1 */
> +  //{ CODEC_ID_???,       0x0203010C },/* Dolby E */
> +  //{ CODEC_ID_???,       0x02030301 },/* MPEG-2 AAC */
> +    { 0, 0 },
> +};
> +
> +static const CodecTag mxf_picture_essence_labels[] = {
> +    { CODEC_ID_RAWVIDEO,   0x0100 },
> +    { CODEC_ID_MPEG2VIDEO, 0x0201 },
> +    { CODEC_ID_DVVIDEO,    0x0202 },
> +  //{ CODEC_ID_???,        0x0207 },/* D-11 HDCAM */
> +    { 0, 0 },
> +};
> +
> +static const CodecTag mxf_container_picture_labels[] = {
> +    { CODEC_ID_MPEG2VIDEO, 0x0201 }, /* D-10 Mapping */
> +    { CODEC_ID_DVVIDEO,    0x0202 }, /* DV Mapping */
> +  //{ CODEC_ID_???,        0x0203 }, /* HDCAM D-11 Mapping */
> +    { CODEC_ID_MPEG2VIDEO, 0x0204 }, /* MPEG ES Mapping */
> +};
> +
> +static const CodecTag mxf_container_sound_labels[] = {
> +  //{ CODEC_ID_PCM_S16??,  0x0201 }, /* D-10 Mapping */
> +    { CODEC_ID_MP2,        0x0204 }, /* MPEG ES Mapping */
> +    { CODEC_ID_PCM_S16LE,  0x0206 }, /* AES BWF Mapping */
> +    { CODEC_ID_PCM_ALAW,   0x020A },
> +    { 0, 0 },
> +};

why are there so many tables? isnt one enough?


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

[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is