[FFmpeg-devel] [PATCH] ISS-Funcom Demuxer

[Reimar Döffinger]

Hello,
On Sun, Apr 06, 2008 at 12:44:33AM +0530, Jai Menon wrote:
> +typedef struct {
> +    uint16_t packet_size;
> +    uint32_t audio_frame_count;
> +} IssDemuxContext;

I think they can both be just "int" (or actually, for audio_frame_count
see my comment further down).

> +static int iss_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)
> +{
> +    IssDemuxContext *iss = s->priv_data;
> +    ByteIOContext *pb = s->pb;
> +    AVStream *st;
> +    uint8_t header[MAX_HEADER_SIZE];
> +    uint32_t out_size, stream_size;
> +    uint16_t stereo, rate_divisor;
> +    uint8_t temp, tempstr[5];
> +
> +    get_strz(pb, header, MAX_HEADER_SIZE);
> +    sscanf(&header[ISS_SIG_LEN + 1], "%d", &iss->packet_size);
> +
> +    get_strz(pb, header, MAX_HEADER_SIZE);
> +    sscanf(header, " %ld %d %d %d %d %s %ld ", &out_size, &stereo, &temp, &rate_divisor, &temp, tempstr, &stream_size);

These types are just plain wrong.
First, and most critically, the %s misses a length specifier.
Secondly, %ld / %d _definitely_  is wrong for uint32_t/uint16_t,
and even %d/%hd for them would be very bad style, use the PRId32/PRId16
types from inttypes.h

> +    pkt->pts = iss->audio_frame_count;
> +    iss->audio_frame_count += (ret / s->streams[0]->codec->channels);

And I think it would be a better idea to make audio_frame_count
uint64_t, while it is unlikely that someone has a file with more than
ca. 27 hours of audio I think there is no reason not to support it.
Secondly, I think it would (purely theoretically) be more robust to do
the division by the number of channels when assigning, i.e.
> pkt->pts = iss->audio_sample_nr;
> iss->audio_frame_nr += ret;

but if you want to keep the current way, a name like last_pts seems
better to me, audio_frame_count seems bad to me, because
1) the term audio "frame" is quite often use to mean a whole decoding
block not just one sample (for each channel)
2) "count" to me sounds like the total number, not just the amount read
so far

Greetings,
Reimar D?ffinger