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

Reimar Döffinger Reimar.Doeffinger
Sat Apr 5 21:41:51 CEST 2008

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

Reimar D?ffinger

More information about the ffmpeg-devel mailing list