Re: [FFmpeg-devel] [PATCH] avformat: add Limitless Audio Format demuxer

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Paul B Mahol:
> +static int laf_read_header(AVFormatContext *ctx)
> +{
> +    LAFContext *s = ctx->priv_data;
> +    AVIOContext *pb = ctx->pb;
> +    unsigned st_count, mode;
> +    unsigned sample_rate;
> +    int64_t duration;
> +    int codec_id;
> +    int quality;
> +    int bpp;
> +
> +    avio_skip(pb, 9);
> +    if (avio_rb32(pb) != MKBETAG('H','E','A','D'))
> +        return AVERROR_INVALIDDATA;
> +
> +    quality = avio_r8(pb);
> +    if (quality > 3)
> +        return AVERROR_INVALIDDATA;
> +    mode = avio_r8(pb);
> +    if (mode > 1)
> +        return AVERROR_INVALIDDATA;
> +    st_count = avio_rl32(pb);
> +    if (st_count == 0 || st_count > 1024)

I don't know whether the limit of 1024 is arbitrary or something from
some spec. If it is the latter, you should use a #define for it and also
for the size of the StreamParams array in the ctx. If it is the former,
you might just use FF_ARRAY_ELEMS(s->p) instead of 1024 here. Or a
define, as you prefer.

> +        return AVERROR_INVALIDDATA;
> +
> +    for (int i = 0; i < st_count; i++) {
> +        StreamParams *stp = &s->p[i];
> +
> +        stp->vertical = av_int2float(avio_rl32(pb));
> +        stp->horizontal = av_int2float(avio_rl32(pb));
> +        stp->lfe = avio_r8(pb);
> +        if (stp->lfe) {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MASK(1, (AV_CH_LOW_FREQUENCY));
> +        } else if (stp->vertical == 0.f &&
> +                   stp->horizontal == 0.f) {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MASK(1, (AV_CH_FRONT_CENTER));
> +        } else if (stp->vertical == 0.f &&
> +                   stp->horizontal == -30.f) {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MASK(1, (AV_CH_FRONT_LEFT));
> +        } else if (stp->vertical == 0.f &&
> +                   stp->horizontal == 30.f) {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MASK(1, (AV_CH_FRONT_RIGHT));
> +        } else if (stp->vertical == 0.f &&
> +                   stp->horizontal == -110.f) {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MASK(1, (AV_CH_SIDE_LEFT));
> +        } else if (stp->vertical == 0.f &&
> +                   stp->horizontal == 110.f) {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MASK(1, (AV_CH_SIDE_RIGHT));
> +        } else {
> +            stp->layout = (AVChannelLayout)AV_CHANNEL_LAYOUT_MONO;
> +        }
> +    }
> +
> +    sample_rate = avio_rl32(pb);
> +    duration = avio_rl64(pb) / st_count;
> +    switch (quality) {
> +    case 0:
> +        codec_id = AV_CODEC_ID_PCM_U8;
> +        bpp = 1;
> +        break;
> +    case 1:
> +        codec_id = AV_CODEC_ID_PCM_S16LE;
> +        bpp = 2;
> +        break;
> +    case 2:
> +        codec_id = AV_CODEC_ID_PCM_F32LE;
> +        bpp = 4;
> +        break;
> +    case 3:
> +        codec_id = AV_CODEC_ID_PCM_S24LE;
> +        bpp = 3;
> +        break;
> +    }
> +
> +    s->index = 0;
> +    s->stored_index = 0;
> +    s->bpp = bpp;
> +    s->data = av_mallocz(st_count * sample_rate * bpp);

sample_rate is read via avio_rl32() and therefore the multiplication on
the right can overflow (it's performed in 32bits, so this can happen
even on 64bit systems). Maybe use av_calloc(sample_rate, st_count *
bpp). But you also need to ensure that sample_rate actually fits into an
int and that st_count * sample_rate * bpp performed in the avio_read()
below also fits into an int, so you should probably just ensure this here.

> +    if (!s->data)
> +        return AVERROR(ENOMEM);
> +
> +    for (int st = 0; st < st_count; st++) {
> +        StreamParams *stp = &s->p[st];
> +        LAFStream *lafst;
> +        AVCodecParameters *par;
> +        AVStream *st = avformat_new_stream(ctx, NULL);
> +        if (!st)
> +            return AVERROR(ENOMEM);
> +
> +        par = st->codecpar;
> +        par->codec_id = codec_id;
> +        par->codec_type = AVMEDIA_TYPE_AUDIO;
> +        par->ch_layout.nb_channels = 1;
> +        par->ch_layout = stp->layout;
> +        par->sample_rate = sample_rate;
> +        st->duration = duration;
> +        st->priv_data = lafst = av_mallocz(sizeof(LAFStream));

lafst is set-but-unused. And given that you are already imposing a
hardcoded limit on the number of streams you could just add an array of
1024 uint8_t to your context.

> +        if (!st->priv_data)
> +            return AVERROR(ENOMEM);
> +
> +        avpriv_set_pts_info(st, 64, 1, st->codecpar->sample_rate);
> +    }
> +
> +    return 0;
> +}
> +
> +static int laf_read_packet(AVFormatContext *ctx, AVPacket *pkt)
> +{
> +    AVIOContext *pb = ctx->pb;
> +    LAFContext *s = ctx->priv_data;
> +    AVStream *st = ctx->streams[0];
> +    LAFStream *lafst = st->priv_data;
> +    const int bpp = s->bpp;
> +    int header_len = (ctx->nb_streams / 8) + !!(ctx->nb_streams & 7);

(ctx->nb_streams + 7) / 8.

> +    int64_t pos;
> +    int ret;
> +
> +again:
> +    if (avio_feof(pb))
> +        return AVERROR_EOF;
> +
> +    pos = avio_tell(pb);
> +
> +    if (s->index >= ctx->nb_streams) {
> +        int cur_st = 0, st_count = 0, st_index = 0;
> +
> +        for (int i = 0; i < header_len; i++) {
> +            uint8_t val = avio_r8(pb);

Given that you impose a limit of 1024 for the number of streams, you can
actually put an uint8_t [128] on the stack in this loop and read all the
values at once. This would allow to remove the outer loop.
(If you used an array of uint8_t instead of the st->priv_data for
stored, you could also use that array.)

> +
> +            for (int j = 0; j < 8 && cur_st < ctx->nb_streams; j++, cur_st++) {
> +                AVStream *st = ctx->streams[st_index];
> +                LAFStream *lafst = st->priv_data;
> +
> +                lafst->stored = 0;
> +                if (val & 1) {
> +                    lafst->stored = 1;
> +                    st_count++;
> +                }
> +                val >>= 1;
> +                st_index++;
> +            }
> +        }
> +
> +        s->index = s->stored_index = 0;
> +        s->nb_stored = st_count;
> +        if (!st_count)
> +            return AVERROR_INVALIDDATA;
> +        ret = avio_read(pb, s->data, st_count * st->codecpar->sample_rate * bpp);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    st = ctx->streams[s->index];
> +    lafst = st->priv_data;
> +    while (!lafst->stored) {
> +        s->index++;
> +        if (s->index >= ctx->nb_streams)
> +            goto again;
> +        lafst = ctx->streams[s->index]->priv_data;
> +    }
> +    st = ctx->streams[s->index];
> +
> +    ret = av_new_packet(pkt, st->codecpar->sample_rate * bpp);
> +    if (ret < 0)
> +        return ret;
> +
> +    for (int n = 0; n < st->codecpar->sample_rate; n++)
> +        memcpy(pkt->data + n * bpp, s->data + n * s->nb_stored * bpp + s->stored_index * bpp, bpp);

This looks like something that can easily trigger a timeout.

> +
> +    pkt->stream_index = s->index;
> +    pkt->pos = pos;

If you have data from multiple streams interleaved, then the first
stream will get the position from before reading header_len bytes, but
all the other streams will get the position from after reading the
common data. IMO all packets should get the position of the common data.

> +    s->index++;
> +    s->stored_index++;
> +
> +    return ret;

return 0 -- it is not really defined what happens in case read_packet
callbacks return positive values (it is currently ignored and some
demuxers return the size of the packet, but that is a remnant of an
earlier API) which could happen if av_new_packet() were changed to allow
to return positive values.

> +}
> +
> +static int laf_read_seek(AVFormatContext *ctx, int stream_index,
> +                         int64_t timestamp, int flags)
> +{
> +    LAFContext *s = ctx->priv_data;
> +
> +    s->stored_index = s->index = 0;
> +
> +    return -1;
> +}
> +
> +const AVInputFormat ff_laf_demuxer = {
> +    .name           = "laf",
> +    .long_name      = NULL_IF_CONFIG_SMALL("LAF (Limitless Audio Format)"),
> +    .priv_data_size = sizeof(LAFContext),
> +    .read_probe     = laf_probe,
> +    .read_header    = laf_read_header,
> +    .read_packet    = laf_read_packet,
> +    .read_seek      = laf_read_seek,
> +    .extensions     = "laf",
> +    .flags          = AVFMT_GENERIC_INDEX,
> +};

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".