Re: [FFmpeg-devel] [PATCH] OSQ lossless audio format support

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 7023 bytes --]

On Thu, Aug 24, 2023 at 11:52:45AM +0200, Paul B Mahol wrote:
> Patches attached.
> 
> Stereo decoding have some issues with some predictors so not yet bitexact.
> 
> Please comment.


[...]
> diff --git a/libavformat/osq.c b/libavformat/osq.c
> new file mode 100644
> index 0000000000..36ce25313f
> --- /dev/null
> +++ b/libavformat/osq.c
> @@ -0,0 +1,117 @@
> +/*
> + * OSQ demuxer
> + * Copyright (c) 2023 Paul B Mahol
> + *
> + * 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 "libavutil/intreadwrite.h"
> +#include "avio_internal.h"
> +#include "avformat.h"
> +#include "demux.h"
> +#include "internal.h"
> +#include "rawdec.h"
> +
> +static int osq_probe(const AVProbeData *p)
> +{

> +    if (AV_RL32(p->buf) != MKTAG('O','S','Q',' '))
> +        return 0;
> +    if (AV_RL32(p->buf + 4) != 48)
> +        return 0;
> +    if (AV_RL16(p->buf + 8) != 1)
> +        return 0;

this can be simplified to a single memcmp() with a 10 byte array


> +    if (!p->buf[10])
> +        return 0;
> +    if (!p->buf[11])
> +        return 0;
> +    if (AV_RL32(p->buf + 12) == 0)
> +        return 0;
> +    if (AV_RL16(p->buf + 16) == 0)
> +        return 0;

inconsistant, you mix !x and == 0 for the same kind of check


> +
> +    return AVPROBE_SCORE_MAX;
> +}
> +
> +static int osq_read_header(AVFormatContext *s)
> +{
> +    uint32_t t, size;
> +    AVStream *st;
> +    int ret;
> +
> +    t = avio_rl32(s->pb);
> +    if (t != MKTAG('O','S','Q',' '))
> +        return AVERROR_INVALIDDATA;
> +
> +    size = avio_rl32(s->pb);
> +    if (size != 48)
> +        return AVERROR_INVALIDDATA;
> +
> +    st = avformat_new_stream(s, NULL);
> +    if (!st)
> +        return AVERROR(ENOMEM);
> +    if ((ret = ff_get_extradata(s, st->codecpar, s->pb, size)) < 0)
> +        return ret;
> +

> +    t = avio_rl32(s->pb);
> +    if (t != MKTAG('R','I','F','F'))
> +        return AVERROR_INVALIDDATA;
> +    avio_skip(s->pb, 8);
> +
> +    t = avio_rl32(s->pb);
> +    if (t != MKTAG('f','m','t',' '))
> +        return AVERROR_INVALIDDATA;
> +    size = avio_rl32(s->pb);
> +    avio_skip(s->pb, size);
> +
> +    t = avio_rl32(s->pb);
> +    size = avio_rl32(s->pb);
> +    while (t != MKTAG('d','a','t','a')) {
> +        avio_skip(s->pb, size);
> +
> +        t = avio_rl32(s->pb);
> +        size = avio_rl32(s->pb);
> +        if (avio_feof(s->pb))
> +            return AVERROR_INVALIDDATA;
> +    }

This looks familiar, can code be shared with other RIFF based formats ?


> +
> +    st->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
> +    st->codecpar->codec_id    = AV_CODEC_ID_OSQ;
> +    st->codecpar->sample_rate = AV_RL32(st->codecpar->extradata + 4);
> +    if (st->codecpar->sample_rate == 0)
> +        return AVERROR_INVALIDDATA;

missing check for negative numbers


[...]
> diff --git a/libavcodec/osq.c b/libavcodec/osq.c
> new file mode 100644
> index 0000000000..b6dc5c1bb4
> --- /dev/null
> +++ b/libavcodec/osq.c
> @@ -0,0 +1,435 @@
> +/*
> + * OSQ audio decoder
> + * Copyright (c) 2023 Paul B Mahol
> + *
> + * 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
> + */
> +

> +#define ASSERT_LEVEL 5

that looks strange
assert level is set by the user building this, also there is no level 5


> +#include "libavutil/avassert.h"
> +#include "libavutil/internal.h"
> +#include "libavutil/intreadwrite.h"
> +#include "avcodec.h"
> +#include "codec_internal.h"
> +#include "decode.h"
> +#include "internal.h"
> +#define BITSTREAM_READER_LE
> +#include "get_bits.h"
> +#include "unary.h"
> +
> +typedef struct OSQChannel {
> +    int prediction;
> +    int coding_mode;
> +    int residue_parameter;
> +    int residue_bits;
> +} OSQChannel;
> +
> +typedef struct OSQContext {
> +    GetBitContext gb;
> +    OSQChannel ch[2];
> +
> +    uint8_t *bitstream;

> +    int64_t max_framesize;

i think the max_framesize fits in 29bits currently so 64 should not be needed


[...]


> +
> +    s->bitstream = av_calloc(s->max_framesize + AV_INPUT_BUFFER_PADDING_SIZE, sizeof(*s->bitstream));
> +    if (!s->bitstream)
> +        return AVERROR(ENOMEM);
> +
> +    for (int ch = 0; ch < avctx->ch_layout.nb_channels; ch++) {
> +        s->decode_buffer[ch] = av_calloc(s->frame_samples + 4,
> +                                         sizeof(*s->decode_buffer[ch]));
> +        if (!s->decode_buffer[ch])
> +            return AVERROR(ENOMEM);
> +    }
> +
> +    s->pkt = avctx->internal->in_pkt;
> +
> +    return 0;
> +}
> +
> +static uint32_t get_urice(GetBitContext *gb, int k)
> +{
> +    uint32_t z, x, b;
> +
> +    x = get_unary(gb, 1, 512);
> +    b = get_bits_long(gb, k & 31);
> +    z = b | x << (k & 31);

the k & 31 seems unneeded


[...]
> +        }
> +        break;
> +    case AV_SAMPLE_FMT_S32P:
> +        for (int ch = 0; ch < avctx->ch_layout.nb_channels; ch++) {
> +            int32_t *dst = (int32_t *)frame->extended_data[ch];
> +            int32_t *src = s->decode_buffer[ch] + 4;
> +
> +            for (int n = 0; n < frame->nb_samples; n++)
> +                dst[n] = src[n];

memcpy() unless these can overlap

thx


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".