[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