[FFmpeg-devel] [PATCH v2 2/2] avformat: add demuxer for Pro Pinball Series' Soundbanks
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Wed Mar 18 17:05:40 EET 2020
Zane van Iperen:
> Signed-off-by: Zane van Iperen <zane at zanevaniperen.com>
> ---
> Changelog | 1 +
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 1 +
> libavformat/pp_bnk.c | 218 +++++++++++++++++++++++++++++++++++++++
> libavformat/version.h | 2 +-
> 5 files changed, 222 insertions(+), 1 deletion(-)
> create mode 100644 libavformat/pp_bnk.c
>
> diff --git a/Changelog b/Changelog
> index 927dc89c51..0970bfb11e 100644
> --- a/Changelog
> +++ b/Changelog
> @@ -55,6 +55,7 @@ version <next>:
> - CRI HCA decoder
> - CRI HCA demuxer
> - Cunning Developments ADPCM decoder
> +- Pro Pinball Series Soundbank demuxer
>
>
> version 4.2:
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 8fd0d43721..9df99133fa 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -428,6 +428,7 @@ OBJS-$(CONFIG_PCM_VIDC_DEMUXER) += pcmdec.o pcm.o
> OBJS-$(CONFIG_PCM_VIDC_MUXER) += pcmenc.o rawenc.o
> OBJS-$(CONFIG_PJS_DEMUXER) += pjsdec.o subtitles.o
> OBJS-$(CONFIG_PMP_DEMUXER) += pmpdec.o
> +OBJS-$(CONFIG_PP_BNK_DEMUXER) += pp_bnk.o
> OBJS-$(CONFIG_PVA_DEMUXER) += pva.o
> OBJS-$(CONFIG_PVF_DEMUXER) += pvfdec.o pcm.o
> OBJS-$(CONFIG_QCP_DEMUXER) += qcp.o
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index 39d2c352f5..3919c9e4c1 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -341,6 +341,7 @@ extern AVInputFormat ff_pcm_u8_demuxer;
> extern AVOutputFormat ff_pcm_u8_muxer;
> extern AVInputFormat ff_pjs_demuxer;
> extern AVInputFormat ff_pmp_demuxer;
> +extern AVInputFormat ff_pp_bnk_demuxer;
> extern AVOutputFormat ff_psp_muxer;
> extern AVInputFormat ff_pva_demuxer;
> extern AVInputFormat ff_pvf_demuxer;
> diff --git a/libavformat/pp_bnk.c b/libavformat/pp_bnk.c
> new file mode 100644
> index 0000000000..dbb160576e
> --- /dev/null
> +++ b/libavformat/pp_bnk.c
> @@ -0,0 +1,218 @@
> +/*
> + * Pro Pinball Series Soundbank (44c, 22c, 11c, 5c) demuxer.
> + *
> + * Copyright (C) 2020 Zane van Iperen (zane at zanevaniperen.com)
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +#include "avformat.h"
> +#include "internal.h"
> +#include "libavutil/intreadwrite.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/internal.h"
> +
> +#define PP_BNK_MAX_READ_SIZE 4096
> +#define PP_BNK_FILE_HEADER_SIZE 20
> +#define PP_BNK_TRACK_SIZE 20
> +
> +typedef struct PPBnkHeader {
> + uint32_t bank_id; /*< Bank ID, useless for our purposes. */
> + uint32_t sample_rate; /*< Sample rate of the contained tracks. */
> + uint32_t always1; /*< Unknown, always seems to be 1. */
> + uint32_t track_count; /*< Number of tracks in the file. */
> + uint32_t unknown; /*< Possibly flags. 2 == music, 0 == sfx? */
> +} PPBnkHeader;
> +
> +typedef struct PPBnkTrack {
> + uint32_t id; /*< Track ID. Usually track[i].id == track[i-1].id + 1, but not always */
> + uint32_t size; /*< Size of the data in bytes. */
> + uint32_t sample_rate; /*< Sample rate. */
> + uint32_t always1_1; /*< Unknown, always seems to be 1. */
> + uint32_t always1_2; /*< Unknown, always seems to be 1. */
> +} PPBnkTrack;
> +
> +typedef struct PPBnkCtxTrack {
> + int64_t data_offset;
> + uint32_t data_size;
> +} PPBnkCtxTrack;
> +
> +typedef struct PPBnkCtx {
> + int track_count;
> + PPBnkCtxTrack *tracks;
> + uint32_t i;
How about using a more descriptive name for this like current_track?
> + uint32_t bytes_read;
> +} PPBnkCtx;
> +
> +static void pp_bnk_parse_header(PPBnkHeader *hdr, const uint8_t *buf)
> +{
> + hdr->bank_id = AV_RL32(buf + 0);
> + hdr->sample_rate = AV_RL32(buf + 4);
> + hdr->always1 = AV_RL32(buf + 8);
> + hdr->track_count = AV_RL32(buf + 12);
> + hdr->unknown = AV_RL32(buf + 16);
> +}
> +
> +static void pp_bnk_parse_track(PPBnkTrack *trk, const uint8_t *buf)
> +{
> + trk->id = AV_RL32(buf + 0);
> + trk->size = AV_RL32(buf + 4);
> + trk->sample_rate = AV_RL32(buf + 8);
> + trk->always1_1 = AV_RL32(buf + 12);
> + trk->always1_2 = AV_RL32(buf + 16);
> +}
> +
> +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))))
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.)
> + 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)
> + return ret;
> + else if (ret != PP_BNK_TRACK_SIZE)
> + 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) {
> + 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++) {
> +
> + if (!(st = avformat_new_stream(s, NULL)))
> + 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)
> + return ret;
> + else if (ret != ctx->tracks[0].data_offset)
> + return AVERROR(EIO);
> +
> + return 0;
> +}
> +
> +static int pp_bnk_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> + int ret, size;
> + PPBnkCtx *ctx = s->priv_data;
> + PPBnkCtxTrack *trk = ctx->tracks + ctx->i;
> +
> + av_assert0(ctx->bytes_read <= trk->data_size);
> +
> + if (ctx->bytes_read == trk->data_size) {
> + ctx->bytes_read = 0;
> + ctx->i += 1;
> + trk += 1;
Why don't you simply use an increment ++?
> +
> + 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.
> + 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.
> + }
> +
> + if (ctx->i == ctx->track_count)
> + return AVERROR_EOF;
> +
> + size = FFMIN(trk->data_size - ctx->bytes_read, PP_BNK_MAX_READ_SIZE);
> +
> + if ((ret = av_get_packet(s->pb, pkt, size)) < 0)
> + return ret;
> + else if (ret != size)
> + return AVERROR_INVALIDDATA;
> +
> + ctx->bytes_read += ret;
> +
> + pkt->stream_index = ctx->i;
> + pkt->duration = ret * 2;
> + return 0;
> +}
> +
> +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.
> +
> + return 0;
> +}
> +
> +AVInputFormat ff_pp_bnk_demuxer = {
> + .name = "pp_bnk",
> + .long_name = NULL_IF_CONFIG_SMALL("Pro Pinball Series Soundbank"),
> + .priv_data_size = sizeof(PPBnkCtx),
> + .read_header = pp_bnk_read_header,
> + .read_packet = pp_bnk_read_packet,
> + .read_close = pp_bnk_read_close,
> + .extensions = "5c,11c,22c,44c"
> +};
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 18c2f5fec2..493a0b337f 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,7 +32,7 @@
> // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
> // Also please add any ticket numbers that you believe might be affected here
> #define LIBAVFORMAT_VERSION_MAJOR 58
> -#define LIBAVFORMAT_VERSION_MINOR 42
> +#define LIBAVFORMAT_VERSION_MINOR 43
> #define LIBAVFORMAT_VERSION_MICRO 100
>
> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
>
More information about the ffmpeg-devel
mailing list