On Mon, 06 Apr 2020 15:00:01 +0200
"Anton Khirnov" <an...@khirnov.net> wrote:
> Quoting Zane van Iperen (2020-03-29 19:18:20)
> > Signed-off-by: Zane van Iperen <z...@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?
Zane
> --
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
