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

[Reimar Döffinger]

On Sat, Apr 05, 2008 at 11:03:20PM +0100, M?ns Rullg?rd wrote:
> Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:
> >> +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
> 
> Better yet, make them all int (possibly unsigned), and use %d.  Much
> nicer and simpler.

Esp. since for *scanf SCNd32 etc. would be the right macros...

> > 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;

and it should be
> pkt->pts = iss->audio_sample_nr / ...->channels;
here, forgot to actually add the division. Though this kind of change
seems much less relevant now...