[FFmpeg-devel] [PATCH] Bink file demuxer and audio decoder

Peter Ross pross
Sat Jun 20 15:49:05 CEST 2009


On Mon, Jan 19, 2009 at 02:34:07PM +0100, Michael Niedermayer wrote:
> On Wed, Jan 14, 2009 at 02:39:28AM -0600, Daniel Verkamp wrote:
> > Hi,
> > 
> > Attached are patches to add a Bink file demuxer and Bink audio
> > decoder, plus support for Bink audio in Smacker files.
> > 

Revised patch enclosed.

This now uses Alex Converse's RDFT implementation, and includes a
preliminary FFT-based DCT.

> [...]
> 
> > +// from wmadata.h
> > +static const uint16_t wma_critical_freqs[25] = {
> 
> if its "from" that means its copy & pasted, but instead it should be
> shared

Fixed.

> > +#define MAX_CHANNELS 2
> > +
> 
> > +#define BLOCK_MAX_SIZE ((1 << 11) * MAX_CHANNELS)
> 
> MAX_CHANNELS << 11

Fixed.

> > +    DECLARE_ALIGNED_16(short, window[BLOCK_MAX_SIZE / 16]);
> 
> this is no window, this are the samples of the previous frame

Yeah, okay.

> [...]
> > +    // windowing
> > +    if (!s->first) {
> > +        int count = s->overlap_len * channels;
> > +        for (i = 0; i < count; i++) {
> > +            out[i] = (s->window[i] * (count - i) + out[i] * i) / count;
> > +        }
> 
> / is slow, use >> or * >>

Done.

> > +    }
> > +    memcpy(s->window,
> > +           out + (s->frame_len - s->overlap_len) * channels,
> > +           s->overlap_len * channels * sizeof(short));
> 
> sizeof(*out) is more robust

Agree.

> > +    init_get_bits(gb, buf, buf_size * 8);
> > +
> 
> > +    frame_size = get_bits(gb, 32);
> > +    if (frame_size > *outdata_size) {
> > +        av_log(avctx, AV_LOG_ERROR, "insufficient output buffer size\n");
> > +        return -1;
> > +    }
> 
> this check is broken
>
> > +
> > +    *outdata_size = frame_size;
> > +    while (get_bits_count(gb) / 8 < buf_size) {
> > +        samples += decode_block(s, samples);
> > +        get_bits_align32(gb);
> > +    }
> 
> exploitable

Fixed and fixed.

> [...]
> > +static int bink_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > +{
> > +    BinkDemuxContext *bink = s->priv_data;
> > +    ByteIOContext *pb = s->pb;
> > +    uint32_t fps_num, fps_den;
> > +    AVStream *vst, *ast;
> > +    unsigned int i;
> > +    uint32_t pos, prev_pos;
> > +    uint16_t audio_flags;
> > +    int keyframe;
> > +
> > +    vst = av_new_stream(s, 0);
> > +    if (!vst)
> > +        return AVERROR(ENOMEM);
> > +
> > +    vst->codec->codec_tag = get_le32(pb);
> > +
> > +    bink->file_size = get_le32(pb) + 8;
> > +    bink->total_frames = get_le32(pb);
> > +
> 
> > +    if (bink->total_frames > 1000000) {
> > +        av_log(s, AV_LOG_ERROR, "invalid header: more than 1000000 frames\n");
> > +        return AVERROR(EIO);
> > +    }
> 
> in what way is a file invalid that has more frames?

David Verkamp gave a justification somewhere else in the thread.

> > +
> > +    if (get_le32(pb) > bink->file_size) {
> > +        av_log(s, AV_LOG_ERROR,
> > +               "invalid header: largest frame size greater than file size\n");
> > +        return AVERROR(EIO);
> > +    }
> > +
> > +    url_fskip(pb, 4);
> > +
> 
> > +    vst->codec->width  = get_le32(pb);
> > +    if (vst->codec->width > 32767) {
> > +        av_log(s, AV_LOG_ERROR,
> > +               "invalid header: width %d greater than 32767\n",
> > +               vst->codec->width);
> > +        return AVERROR(EIO);
> > +    }
> > +
> > +    vst->codec->height = get_le32(pb);
> > +    if (vst->codec->height > 32767) {
> > +        av_log(s, AV_LOG_ERROR,
> > +        "invalid header: height %d greater than 32767\n",
> > +        vst->codec->height);
> > +        return AVERROR(EIO);
> > +    }
> 
> similarly, how is a file invalid when it has a larger w/h?

Removed.

> > +
> > +    fps_num = get_le32(pb);
> > +    fps_den = get_le32(pb);
> > +
> > +    if (fps_num == 0 || fps_den == 0) {
> > +        av_log(s, AV_LOG_ERROR,
> > +               "invalid header: invalid fps (%d/%d)\n",
> > +               fps_num, fps_den);
> > +        return AVERROR(EIO);
> > +    }
> > +
> > +    url_fskip(pb, 4);
> > +
> > +    vst->codec->codec_type = CODEC_TYPE_VIDEO;
> > +    vst->codec->codec_id   = 0; /* FIXME: CODEC_ID_BINKVIDEO */
> > +    av_set_pts_info(vst, 64, fps_den, fps_num);
> > +
> > +    bink->num_audio_tracks = get_le32(pb);
> > +
> 
> > +    if (bink->num_audio_tracks > BINK_MAX_AUDIO_TRACKS) {
> > +        av_log(s, AV_LOG_ERROR,
> > +               "invalid header: more than 256 audio tracks (%d)\n",
>                                              ^^^
> should be BINK_MAX_AUDIO_TRACKS

Fixed.

> 
> > +               bink->num_audio_tracks);
> > +        return AVERROR(EIO);
> > +    }
> > +
> 
> > +    if (bink->num_audio_tracks) {
> 
> the if() seems useless

This is crucial as the audio header only exists when num_audio_tracks > 0.

> > +        url_fskip(pb, 4 * bink->num_audio_tracks);
> > +
> > +        for (i = 0; i < bink->num_audio_tracks; i++) {
> > +            ast = av_new_stream(s, 1);
> > +            if (!ast)
> > +                return AVERROR(ENOMEM);
> > +            ast->codec->codec_type  = CODEC_TYPE_AUDIO;
> > +            ast->codec->codec_id    = CODEC_ID_BINKAUDIO;
> > +            ast->codec->codec_tag   = 0;
> > +            ast->codec->sample_rate = get_le16(pb);
> > +            av_set_pts_info(ast, 64, 1, ast->codec->sample_rate);
> 
> > +            audio_flags = get_le16(pb);
> > +            ast->codec->channels = audio_flags & BINK_AUD_STEREO ? 2 : 1;
> > +            if (audio_flags & BINK_AUD_USEDCT) {
> > +                ast->codec->extradata = av_malloc(BINK_EXTRADATA_SIZE);
> > +                ast->codec->extradata_size = BINK_EXTRADATA_SIZE;
> > +                *ast->codec->extradata = 1;
> > +            }
> 
> why dont you read the flags into extradata ?

The flags bitmasks are unfortunately specific to Smacker and Bink container
formats. I have changed this such that the RDFT and DCT based codecs are
now identified using seperate CODEC_IDs.

> [...]
> > +static int bink_read_packet(AVFormatContext *s, AVPacket *pkt)
> > +{
> > +    BinkDemuxContext *bink = s->priv_data;
> > +    ByteIOContext *pb = s->pb;
> > +    int ret;
> > +
> > +    if (bink->current_track < 0) {
> > +        int index_entry;
> > +        AVStream *st = s->streams[0]; // stream 0 is video stream with index
> > +
> > +        if (bink->video_pts >= bink->total_frames)
> > +            return AVERROR(EIO);
> > +
> > +        index_entry = av_index_search_timestamp(st, bink->video_pts,
> > +                                                AVSEEK_FLAG_ANY);
> > +        if (index_entry < 0) {
> > +            av_log(s, AV_LOG_ERROR,
> > +                   "could not find index entry for frame %d\n",
> > +                   bink->video_pts);
> > +            return AVERROR(EIO);
> > +        }
> > +
> > +        bink->remain_packet_size = st->index_entries[index_entry].size;
> > +        bink->current_track = 0;
> > +    }
> > +
> > +    if (bink->current_track < bink->num_audio_tracks) {
> 
> > +        uint32_t audio_size = get_le32(pb);
> > +        if (4 + audio_size > bink->remain_packet_size) {
> 
> 4 + audio_size can overflow

Well spotted. Fixed.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: export-wma-critial-freqs.diff
Type: text/x-diff
Size: 1428 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/8f621af6/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dct-r201.diff
Type: text/x-diff
Size: 5009 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/8f621af6/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: binkaudio-decoder-r301.diff
Type: text/x-diff
Size: 11966 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/8f621af6/attachment-0002.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bink-demuxer-r301.diff
Type: text/x-diff
Size: 9880 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/8f621af6/attachment-0003.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smacker-decoder-r300.diff
Type: text/x-diff
Size: 1565 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/8f621af6/attachment-0004.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090620/8f621af6/attachment.pgp>



More information about the ffmpeg-devel mailing list