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

[Paul B Mahol]

On 4/6/20, Zane van Iperen <zane at zanevaniperen.com> wrote:
> On Mon, 06 Apr 2020 15:00:01 +0200
> "Anton Khirnov" <anton at khirnov.net> wrote:
>
>> Quoting Zane van Iperen (2020-03-29 19:18:20)
>> > Signed-off-by: Zane van Iperen <zane at zanevaniperen.com>
>> > +static int pp_bnk_read_header(AVFormatContext *s)
>> > +{
>> > +    int ret;
>> > +    AVStream *st;
>> > +    AVCodecParameters *par;
>> > +    PPBnkCtx *ctx = s->priv_data;
>> > +    uint8_t buf[FFMAX(PP_BNK_FILE_HEADER_SIZE, PP_BNK_TRACK_SIZE)];
>> > +    PPBnkHeader hdr;
>> > +
>> > +    if ((ret = avio_read(s->pb, buf, PP_BNK_FILE_HEADER_SIZE)) < 0)
>> > +        return ret;
>> > +    else if (ret != PP_BNK_FILE_HEADER_SIZE)
>> > +        return AVERROR(EIO);
>> > +
>> > +    pp_bnk_parse_header(&hdr, buf);
>> > +
>> > +    if (hdr.track_count == 0 || hdr.track_count > INT_MAX)
>> > +        return AVERROR_INVALIDDATA;
>> > +
>> > +    if (hdr.sample_rate == 0 || hdr.sample_rate > INT_MAX)
>> > +        return AVERROR_INVALIDDATA;
>> > +
>> > +    if (hdr.always1 != 1) {
>> > +        avpriv_request_sample(s, "Non-one header value");
>> > +        return AVERROR_PATCHWELCOME;
>> > +    }
>> > +
>> > +    ctx->track_count = hdr.track_count;
>> > +    if ((ret = av_reallocp_array(&ctx->tracks, hdr.track_count,
>> > sizeof(PPBnkCtxTrack))))
>>
>> Why realloc? Seems this is only allocated once.
>
> Good question. Fixed.
>
>
>> > +        return ret;
>> > +
>> > +    /* Parse and validate each track. */
>> > +    for (int i = 0; i < hdr.track_count; i++) {
>> > +        PPBnkTrack e;
>> > +
>> > +        if ((ret = avio_read(s->pb, buf, PP_BNK_TRACK_SIZE)) < 0) {
>> > +            av_freep(&ctx->tracks);
>>
>> You are duplicating this free at too many places. Would be better to
>> have a cleanup block at the end and jump to that.
>>
>
> I did that originally, however it's at the awkward spot where doing
> that causes the code to be larger than the way it is currently.
>
> I'll change it.
>
>> > +            return ret;
>> > +        } else if (ret != PP_BNK_TRACK_SIZE) {
>> > +            av_freep(&ctx->tracks);
>> > +            return AVERROR(EIO);
>> > +        }
>> > +
>> > +        pp_bnk_parse_track(&e, buf);
>> > +
>> > +        /* The individual sample rates of all tracks must match
>> > that of the file header. */
>> > +        if (e.sample_rate != hdr.sample_rate) {
>> > +            av_freep(&ctx->tracks);
>> > +            return AVERROR_INVALIDDATA;
>> > +        }
>> > +
>> > +        ctx->tracks[i].data_offset = avio_tell(s->pb);
>> > +        ctx->tracks[i].data_size   = e.size;
>> > +
>> > +        /* Skip over the data to the next stream header. */
>> > +        avio_skip(s->pb, e.size);
>> > +    }
>> > +
>> > +    /* Build the streams. */
>> > +    for (int i = 0; i < hdr.track_count; i++) {
>> > +
>>
>> nit: unnecessary empty line
>>
>
> Nit picked.
>
>> > +        if (!(st = avformat_new_stream(s, NULL))) {
>> > +            av_freep(&ctx->tracks);
>> > +            return AVERROR(ENOMEM);
>> > +        }
>> > +
>> > +        par                         = st->codecpar;
>> > +        par->codec_type             = AVMEDIA_TYPE_AUDIO;
>> > +        par->codec_id               =
>> > AV_CODEC_ID_ADPCM_IMA_CUNNING;
>> > +        par->format                 = AV_SAMPLE_FMT_S16;
>> > +        par->channel_layout         = AV_CH_LAYOUT_MONO;
>> > +        par->channels               = 1;
>> > +        par->sample_rate            = hdr.sample_rate;
>> > +        par->bits_per_coded_sample  = 4;
>> > +        par->bits_per_raw_sample    = 16;
>> > +        par->block_align            = 1;
>> > +        par->bit_rate               = par->sample_rate *
>> > par->bits_per_coded_sample; +
>> > +        avpriv_set_pts_info(st, 64, 1, par->sample_rate);
>> > +        st->start_time              = 0;
>> > +        st->duration                = ctx->tracks[i].data_size * 2;
>> > +    }
>> > +
>> > +    /* Seek to the start of the first stream. */
>> > +    if ((ret = avio_seek(s->pb, ctx->tracks[0].data_offset,
>> > SEEK_SET)) < 0) {
>> > +        av_freep(&ctx->tracks);
>> > +        return ret;
>> > +    } else if (ret != ctx->tracks[0].data_offset) {
>> > +        av_freep(&ctx->tracks);
>> > +        return AVERROR(EIO);
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> > +
>> > +static int pp_bnk_read_packet(AVFormatContext *s, AVPacket *pkt)
>> > +{
>> > +    int64_t ret;
>> > +    int size;
>> > +    PPBnkCtx *ctx = s->priv_data;
>> > +    PPBnkCtxTrack *trk = ctx->tracks + ctx->current_track;
>> > +
>> > +    av_assert0(ctx->bytes_read <= trk->data_size);
>> > +
>> > +    if (ctx->bytes_read == trk->data_size) {
>> > +
>>
>> nit: unnecessary empty line
>
> Fixed.
>
>>
>> Otherwise looks ok, but would be nice to have some tests.
>>
>
> I have tests ready, the plan was to send them if merged (and after
> samples are uploaded) to avoid https://patchwork.ffmpeg.org/ failures.
>
> Should I include them in this irregardless?

No, Never in same patch.

>
> Zane
>
>> --
>> Anton Khirnov
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".