[FFmpeg-devel] Bink audio decoder

[Eli Friedman]

On Wed, Apr 9, 2008 at 6:47 AM,  <pross at xvid.org> wrote:
> Hi,
>
>  Enclosed is a working demuxer and audio decoder for the Bink multimedia
>  format. The patch has been tested on PPC hardware only, but should run
>  elsewhere.

A quick look at the demuxer:

+static int bink_probe(AVProbeData *p)
+{
+    const uint8_t *b = p->buf;
+
+    if (b[0] == 'B' && b[1] == 'I' && b[2] == 'K')
+        return AVPROBE_SCORE_MAX;
+    return 0;
+}

Hmm, this looks a bit sensitive (overlikely to trigger)... you should
probably also check for a known version number in b[3], and possibly
also that the video width and height are sane.

+        case 'b' :
+        case 'f' :
+        case 'i' :
+            break;
+        default:
+            av_log(s, AV_LOG_INFO, "unsupported bink container
version 0x%x\n", version);
+            return -1;

http://wiki.multimedia.cx/index.php?title=Bink_Container claims that
there are also versions 'g' and 'h'; have you not tested any of these
samples yet?

+        url_fseek(pb, bink->index_pos + (bink->video_pts+1)*4, SEEK_SET);

It would probably be cleaner to read the entire index while you're
reading the header.  (See av_add_index_entry.)

+    st->codec->codec_id = CODEC_ID_BINKVIDEO;

If you're not actually adding the codec, you should comment out this
line and add "st->codec->codec_id = 0;"

+  unsigned int file_size;
+  unsigned int total_frames;
+  int64_t index_pos;

How exactly can index_pos not fit into an unsigned int?

+        AVStream **st;
+        int i;
+
+        st = av_malloc(bink->num_tracks*sizeof(AVStream*));

Don't allocate your own memory; just use s->streams to iterate over
the streams the second time.  (Plus, as written, it's a buffer
overflow vulnerability.)

+    if (av_get_packet(s->pb, pkt, bink->remain_packet_size) !=
bink->remain_packet_size)
+        return AVERROR(EIO);
+    pkt->stream_index = 0;
+    pkt->pts = bink->video_pts++;

If Bink video is guaranteed to have one frame per packet, add a
comment to that effect.

+    bink->packet_pos = get_le32(pb) - 1;

What the heck is the -1 there for?  Needs a comment.

+            pkt->pts = bink->audio_pts++;

This isn't going to work with multiple audio tracks.

+        if (audio_size > 0) {

Can there be a zero-size audio packet?  If it's impossible, you should
return an error.

You should set PKT_FLAG_KEY at least for the first packet.

-Eli