[FFmpeg-devel] [PATCH] ACT demuxer

[Reimar Döffinger]

Hello,
On Sun, Feb 24, 2008 at 01:27:01PM +0600, Vladimir Voroshilov wrote:
> >  > +    pkt_buf=(uint16_t*)pkt->data;
> >  > +
> >  > +    pkt_buf[1]=bFrame[0];
> >  > +    pkt_buf[3]=bFrame[1];
> >  > +    pkt_buf[5]=bFrame[2];
> >  > +    pkt_buf[7]=bFrame[3];
> >  > +    pkt_buf[9]=bFrame[4];
> >  > +    pkt_buf[0]=bFrame[5];
> >  > +    pkt_buf[2]=bFrame[6];
> >  > +    pkt_buf[4]=bFrame[7];
> >  > +    pkt_buf[6]=bFrame[8];
> >  > +    pkt_buf[8]=bFrame[9];
> >
> >  Why do you think the demuxer should do this conversion and not the
> >  decoder?
> >  It also has endianness issues, files created with -ac copy will only
> >  work on a machine with the same endianness or something evil like that...
> 
> Because proper order of byte is described in G.729 spec and ACT does
> not follow it.
> Thus demuxer just provides those order of bytes which expected by decoder.
> Decoder uses get_bits for reading bits in MSB first order.
> 
> I can't see here endianness or -ac copy issue.

Note that in the original code you had:
> pkt_buf=(uint16_t*)pkt->data;
So I had assumed that pkt_buf would have been uint16_t*, in which case
1) there would have been an endianness issue
2) the decoder could have easily distinguished between the formats by
the packet size.

> +static int act_probe(AVProbeData *p)
> +{
> +    if ((AV_RL32(&p->buf[0]) != RIFF_TAG) ||
> +        (AV_RL32(&p->buf[8]) != WAVE_TAG) ||
> +        (AV_RL32(&p->buf[16]) != 16))
> +    return 0;
> +
> +    if(p->buf[256]!=0x84)
> +        return 0;

IIRC the buffer is only guaranteed to be 32 bytes at least, so here you
must check buf_size.

> +static int act_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    ACTContext* ctx = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    int size;
> +    AVStream* st;

* is placed inconsistently.

> +    ctx->hdr.tag=get_byte(pb);
> +    if(ctx->hdr.tag!=0x84)
> +        return AVERROR_INVALIDDATA;

Does it have any advantage to check here as well?
If there ever comes up some new variant that for no good reason uses
0x85 instead of 0x84 for tag but leaves everything the same,
this check means you cannot even force this demuxer manually.

> +    st->codec->codec_tag = 0;

There is no need to set it to 0, it is initialized to that.

> +    st->duration=(ctx->hdr.minutes*60+ctx->hdr.sec)*100+ctx->hdr.msec/10;
> +
> +    av_set_pts_info(st, 64, 1, 800);

Duration is in time_base units, your time_base is 1/800s but you set
duration in 1/100s units, so your duration value should be off by a
factor 8?
Also, since it is only used here in this function, is it not fairly
pointless and a (tiny) waste of memory to have the header in the context?

> +static int act_read_packet(AVFormatContext *s,
> +                          AVPacket *pkt)
> +{
> +    ACTContext* ctx = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    uint8_t bFrame[22];
> +    uint8_t *pkt_buf;
> +    int bytes_read;
> +    int frame_size=s->streams[0]->codec->frame_size;
> +    
> +    bytes_read = get_buffer(pb, bFrame, frame_size);

There are all kinds of weird assumptions here.
Since bFrame is 22 byte large (why anyway?), you assume that frame_size is <= 22,
without checking.
Maybe there is no risk currently, but it certainly makes it excessively
easy to accidentally introduce an exploit later.
Also since the code below and in general assumes that frame_size is 10
anyway, why not just #define FRAME_SIZE 10 and use that?

> +    if(bytes_read != frame_size || av_new_packet(pkt, frame_size))
> +        return AVERROR(EIO);

AVERROR(EIO) is wrong when av_new_packet fails. Actually you should just
return the error av_new_packet gave.

> +    pkt_buf=(uint8_t*)pkt->data;

The cast should not be necessary at all.
Actually, since pkt->data is uint8_t * already IMO just use it directly.

> +    pkt_buf[1]=bFrame[0];
> +    pkt_buf[3]=bFrame[1];
> +    pkt_buf[5]=bFrame[2];
> +    pkt_buf[7]=bFrame[3];
> +    pkt_buf[9]=bFrame[4];
> +    pkt_buf[0]=bFrame[5];
> +    pkt_buf[2]=bFrame[6];
> +    pkt_buf[4]=bFrame[7];
> +    pkt_buf[6]=bFrame[8];
> +    pkt_buf[8]=bFrame[9];

Hmm.. could by done in-place, has the advantage that
av_get_packet could be used, but it is rather obfuscated...

tmp = pkt->data[0];
pkt->data[0] = pkt->data[5];
pkt->data[5] = pkt->data[2];
pkt->data[2] = pkt->data[6];
pkt->data[6] = pkt->data[8];
pkt->data[8] = pkt->data[9];
pkt->data[9] = pkt->data[4];
pkt->data[4] = pkt->data[7];
pkt->data[7] = pkt->data[3];
pkt->data[3] = pkt->data[1];
pkt->data[1] = tmp;

Greetings,
Reimar D?ffinger