| Message ID | M6RBrV5--3-2@lynne.ee |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] lavc/bsf: add an Opus metadata bitstream filter | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
May 3, 2020, 21:21 by dev@lynne.ee: > The only adjustable field is the gain. Some ripping/transcoding programs > have started to use it for replaygain adjustments. > > Patch attached. > Planning to push this tonight if there are no objections.
Lynne: > The only adjustable field is the gain. Some ripping/transcoding programs > have started to use it for replaygain adjustments. > > Patch attached. > > > > +typedef struct OpusBSFContext { > + const AVClass *class; > + int64_t gain; > +} OpusBSFContext; [...] > > +static const AVOption opus_metadata_options[] = { > + { "gain", "Gain, actual amplification is pow(10, gain/(20.0*256))", OFFSET(gain), > + AV_OPT_TYPE_INT, { .i64 = 0 }, -(INT16_MAX + 1), INT16_MAX, .flags = FLAGS }, > + > + { NULL }, > +}; You are using an AV_OPT_TYPE_INT parameter, yet the actual type of the destination is int64_t. This won't work on big endian systems. Make gain an int. - Andreas PS: Do we actually support two's complement architectures were -(INT16_MAX + 1) != INT16_MIN? (A two's complement architecture in which the representation where the sign bit is set and all other bits is unset is trap representation is legal according to the C standard. Does someone know whether it would also be legal according to POSIX?)
Andreas Rheinhardt (12020-05-05): > > +static const AVOption opus_metadata_options[] = { > > + { "gain", "Gain, actual amplification is pow(10, gain/(20.0*256))", OFFSET(gain), > > + AV_OPT_TYPE_INT, { .i64 = 0 }, -(INT16_MAX + 1), INT16_MAX, .flags = FLAGS }, > > + > > + { NULL }, > > +}; > > You are using an AV_OPT_TYPE_INT parameter, yet the actual type of the > destination is int64_t. This won't work on big endian systems. Make gain > an int. Or make it a float and multiply it by 256 or 5120 before giving it to libopus, possibly. Regards,
May 5, 2020, 09:59 by andreas.rheinhardt@gmail.com: > Lynne: > >> The only adjustable field is the gain. Some ripping/transcoding programs >> have started to use it for replaygain adjustments. >> >> Patch attached. >> >> > >> +typedef struct OpusBSFContext { >> + const AVClass *class; >> + int64_t gain; >> +} OpusBSFContext; >> > [...] > >> >> +static const AVOption opus_metadata_options[] = { >> + { "gain", "Gain, actual amplification is pow(10, gain/(20.0*256))", OFFSET(gain), >> + AV_OPT_TYPE_INT, { .i64 = 0 }, -(INT16_MAX + 1), INT16_MAX, .flags = FLAGS }, >> + >> + { NULL }, >> +}; >> > > You are using an AV_OPT_TYPE_INT parameter, yet the actual type of the > destination is int64_t. This won't work on big endian systems. Make gain > an int. > Thanks, made it an int, patch attached. > PS: Do we actually support two's complement architectures were > -(INT16_MAX + 1) != INT16_MIN? (A two's complement architecture in which > the representation where the sign bit is set and all other bits is unset > is trap representation is legal according to the C standard. Does > someone know whether it would also be legal according to POSIX?) > I think we already rely on that pretty heavily in our code.
May 5, 2020, 11:29 by dev@lynne.ee: > May 5, 2020, 09:59 by andreas.rheinhardt@gmail.com: > >> Lynne: >> >>> The only adjustable field is the gain. Some ripping/transcoding programs >>> have started to use it for replaygain adjustments. >>> >>> Patch attached. >>> >>> > >>> +typedef struct OpusBSFContext { >>> + const AVClass *class; >>> + int64_t gain; >>> +} OpusBSFContext; >>> >> [...] >> >>> >>> +static const AVOption opus_metadata_options[] = { >>> + { "gain", "Gain, actual amplification is pow(10, gain/(20.0*256))", OFFSET(gain), >>> + AV_OPT_TYPE_INT, { .i64 = 0 }, -(INT16_MAX + 1), INT16_MAX, .flags = FLAGS }, >>> + >>> + { NULL }, >>> +}; >>> >> >> You are using an AV_OPT_TYPE_INT parameter, yet the actual type of the >> destination is int64_t. This won't work on big endian systems. Make gain >> an int. >> > > Thanks, made it an int, patch attached. > > > >> PS: Do we actually support two's complement architectures were >> -(INT16_MAX + 1) != INT16_MIN? (A two's complement architecture in which >> the representation where the sign bit is set and all other bits is unset >> is trap representation is legal according to the C standard. Does >> someone know whether it would also be legal according to POSIX?) >> > > I think we already rely on that pretty heavily in our code. > Pushed.
Lynne (12020-05-05):
> Pushed.
I find rather rude that you did not even acknowledge the suggestion in
this mail:
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/262151.html
May 5, 2020, 23:15 by george@nsup.org: > Lynne (12020-05-05): > >> Pushed. >> > > I find rather rude that you did not even acknowledge the suggestion in > this mail: > > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/262151.html > I did acknowledge it but I didn't comment on it because I disliked the idea of modifying the values that get written in the bitstream or using floats in a bsf, and I wanted to comment on the other post more, so I replied to that one instead.
From 44f83786e057d468efee986ee865ba8c776ebe9b Mon Sep 17 00:00:00 2001 From: Lynne <dev@lynne.ee> Date: Sun, 3 May 2020 21:17:33 +0100 Subject: [PATCH] lavc/bsf: add an Opus metadata bitstream filter The only adjustable field is the gain. Some ripping/transcoding programs have started to use it. --- libavcodec/Makefile | 1 + libavcodec/bitstream_filters.c | 1 + libavcodec/opus_metadata_bsf.c | 72 ++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 libavcodec/opus_metadata_bsf.c diff --git a/libavcodec/Makefile b/libavcodec/Makefile index 28076c2c83..cf72f55aff 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -1116,6 +1116,7 @@ OBJS-$(CONFIG_MP3_HEADER_DECOMPRESS_BSF) += mp3_header_decompress_bsf.o \ OBJS-$(CONFIG_MPEG2_METADATA_BSF) += mpeg2_metadata_bsf.o OBJS-$(CONFIG_NOISE_BSF) += noise_bsf.o OBJS-$(CONFIG_NULL_BSF) += null_bsf.o +OBJS-$(CONFIG_OPUS_METADATA_BSF) += opus_metadata_bsf.o OBJS-$(CONFIG_PRORES_METADATA_BSF) += prores_metadata_bsf.o OBJS-$(CONFIG_REMOVE_EXTRADATA_BSF) += remove_extradata_bsf.o OBJS-$(CONFIG_TEXT2MOVSUB_BSF) += movsub_bsf.o diff --git a/libavcodec/bitstream_filters.c b/libavcodec/bitstream_filters.c index 6b5ffe4d70..f1b24baa53 100644 --- a/libavcodec/bitstream_filters.c +++ b/libavcodec/bitstream_filters.c @@ -49,6 +49,7 @@ extern const AVBitStreamFilter ff_mpeg4_unpack_bframes_bsf; extern const AVBitStreamFilter ff_mov2textsub_bsf; extern const AVBitStreamFilter ff_noise_bsf; extern const AVBitStreamFilter ff_null_bsf; +extern const AVBitStreamFilter ff_opus_metadata_bsf; extern const AVBitStreamFilter ff_prores_metadata_bsf; extern const AVBitStreamFilter ff_remove_extradata_bsf; extern const AVBitStreamFilter ff_text2movsub_bsf; diff --git a/libavcodec/opus_metadata_bsf.c b/libavcodec/opus_metadata_bsf.c new file mode 100644 index 0000000000..f0d794c1b3 --- /dev/null +++ b/libavcodec/opus_metadata_bsf.c @@ -0,0 +1,72 @@ +/* + * 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 "bsf.h" +#include "libavutil/intreadwrite.h" +#include "libavutil/opt.h" + +typedef struct OpusBSFContext { + const AVClass *class; + int64_t gain; +} OpusBSFContext; + +static int opus_metadata_filter(AVBSFContext *bsfc, AVPacket *pkt) +{ + return ff_bsf_get_packet_ref(bsfc, pkt); +} + +static int opus_metadata_init(AVBSFContext *bsfc) +{ + OpusBSFContext *s = bsfc->priv_data; + + if (bsfc->par_out->extradata_size < 19) + return AVERROR_INVALIDDATA; + + AV_WL16(bsfc->par_out->extradata + 16, s->gain); + + return 0; +} + +#define OFFSET(x) offsetof(OpusBSFContext, x) +#define FLAGS (AV_OPT_FLAG_AUDIO_PARAM|AV_OPT_FLAG_BSF_PARAM) +static const AVOption opus_metadata_options[] = { + { "gain", "Gain, actual amplification is pow(10, gain/(20.0*256))", OFFSET(gain), + AV_OPT_TYPE_INT, { .i64 = 0 }, -(INT16_MAX + 1), INT16_MAX, .flags = FLAGS }, + + { NULL }, +}; + +static const AVClass opus_metadata_class = { + .class_name = "opus_metadata_bsf", + .item_name = av_default_item_name, + .option = opus_metadata_options, + .version = LIBAVUTIL_VERSION_INT, +}; + +static const enum AVCodecID codec_ids[] = { + AV_CODEC_ID_OPUS, AV_CODEC_ID_NONE, +}; + +const AVBitStreamFilter ff_opus_metadata_bsf = { + .name = "opus_metadata", + .priv_data_size = sizeof(OpusBSFContext), + .priv_class = &opus_metadata_class, + .init = &opus_metadata_init, + .filter = &opus_metadata_filter, + .codec_ids = codec_ids, +}; -- 2.26.2