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

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:

> 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).

Agree.

>> +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.

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

Since we're already commenting on variable names, I think "ret" is
rather nondescript as a name.  I'd prefer something with more of an
indication of the meaning of the variable.

-- 
M?ns Rullg?rd
mans at mansr.com