[FFmpeg-devel] [PATCH v2 2/2] avformat: add demuxer for Pro Pinball Series' Soundbanks

[Zane van Iperen]

On Wed, 18 Mar 2020 16:05:40 +0100
"Andreas Rheinhardt" <andreas.rheinhardt at gmail.com> wrote:

> > +
> > +typedef struct PPBnkCtx {
> > +    int             track_count;
> > +    PPBnkCtxTrack   *tracks;
> > +    uint32_t        i;  
> 
> How about using a more descriptive name for this like current_track?
> 

Done.


> > +
> > +    ctx->track_count = hdr.track_count;
> > +    if ((ret = av_reallocp_array(&ctx->tracks, hdr.track_count,
> > sizeof(PPBnkCtxTrack))))  
> 
> read_close is not called if read_header fails and therefore this array
> leaks if any of the following things that can fail fail.
> (It would probably make sense to call it automatically, but this will
> require changes in some demuxers.)
> 

Fixed, I assumed it was called irregardless of failure.

I wonder how much work it would be to port all of the demuxers to
handle this...


> > +        ctx->bytes_read  = 0;
> > +        ctx->i          += 1;
> > +        trk             += 1;  
> 
> Why don't you simply use an increment ++?
> 

Stylistic reasons. Is moot anyway as I had to change that code to fix a
different bug.

> > +
> > +        if ((ret = avio_seek(s->pb, trk->data_offset, SEEK_SET)) <
> > 0)  
> 
> avio_seek returns int64_t, yet ret is only an int. This will cause
> problems when you seek to positions from 2GB to 4GB.
> 

Fixed.

> > +            return ret;
> > +        else if (ret != trk->data_offset)
> > +            return AVERROR(EIO);  
> 
> I wonder whether the seek should be attempted before you set
> bytes_read and i. If the seek fails and the caller retries to read a
> packet, it will (try to) read the wrong data.
> 

Fixed. This lead me to another issue where it overreads trk before
checking for EOF (also fixed).


> > +static int pp_bnk_read_close(AVFormatContext *s)
> > +{
> > +    PPBnkCtx *ctx = s->priv_data;
> > +
> > +    if (ctx->tracks)
> > +        av_freep(&ctx->tracks);  
> 
> The check here is unnecessary as av_freep() already checks for this;
> and moreover, read_close is only called if read_header succeeded, so
> ctx->tracks is always != NULL.

Fixed.

Zane