[FFmpeg-devel] [PATCH] ACT demuxer

[Vladimir Voroshilov]

Hi, Reimar
This is answer, related to ACT demuxer issues only.

On Sun, Feb 24, 2008 at 12:39 AM, Reimar D?ffinger
<Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:

> [...]

>  > +#define CHUNK_SIZE 512
>  > +#define RIFF_TAG MKTAG('R','I','F','F')
>  > +#define WAVE_TAG MKTAG('W','A','V','E')
>
>  If this is a RIFF/WAVE format, maybe the demuxer for that should be
>  extended instead?

First 44 bytes of file is RIFF format of unpacked WAVE stream.

Here is format description:
http://wiki.multimedia.cx/index.php?title=ACT

IMHO, this is NOT RIFF format.

>  > +typedef  struct{
>  > +    uint8_t tag;        //??? 0x84
>  > +    uint16_t msec;      ///< duration msec
>  > +    uint8_t  sec;       ///< duration sec
>  > +    uint32_t minutes;   ///< Duration min
>  > +} ACTHeader;
>
>  > +    ACTHeader* hdr=(ACTHeader*)&p->buf[256];
>
>  No!!! You can not read binary data into a struct with a simple case,
>  esp. due to endianness and alignment issues.

Removed

>  > +    if(hdr->tag!=0x84)
>
>  Not to mention that it's overkill for a single byte to compare.

Fixed as one-byte check.

>  > +    if (get_buffer(pb, (char*)&ctx->hdr, sizeof(ctx->hdr))!=sizeof(ctx->hdr))
>
>  Same problem.

Removed in favor of get_le16 and so on..

>  > +    if(st->codec->sample_rate!=8000 && st->codec->sample_rate!=4400)
>  > +    {
>  > +        av_log(s, AV_LOG_ERROR, "Sample rate %d is not supported\n", st->codec->sample_rate);
>  > +        return -1;
>  > +    }
>
>  Why do you think this check belongs in the demuxer and not only in the
>  decoder?

I was not sure where to put check, thus put it in both places.
Removed.

>  > +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 i, bytes_read;
>  > +    int frame_size=s->streams[0]->codec->frame_size;
>  > +
>  > +    bytes_read = get_buffer(pb, bFrame, frame_size);
>  > +
>  > +    if(bytes_read != frame_size || av_new_packet(pkt, frame_size))
>  > +        return AVERROR(EIO);
>  > +
>  > +    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.
Muxer will do bytes swapping again (in opposite order) and there will
be no problem.

Code was not touched yet for this issue.

Updated patch is attached.

P.S. also fixed two compilation warnings.

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: act_02.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080224/76f8c8fe/attachment.txt>