[FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
Aurelien Jacobs
aurel at gnuage.org
Mon Jan 8 00:54:39 EET 2018
On Sun, Jan 07, 2018 at 05:23:24PM +0000, Rostislav Pehlivanov wrote:
> On 6 January 2018 at 16:48, Aurelien Jacobs <aurel at gnuage.org> wrote:
>
> > ---
> > Changelog | 2 +-
> > configure | 2 +
> > libavcodec/Makefile | 2 +
> > libavcodec/allcodecs.c | 1 +
> > libavcodec/aptx.c | 352 ++++++++++++++++++++++++++++++
> > ++++++++++++++----
> > libavcodec/avcodec.h | 1 +
> > libavcodec/codec_desc.c | 7 +
> > 7 files changed, 339 insertions(+), 28 deletions(-)
> >
> > diff --git a/Changelog b/Changelog
> > index 3d966c202b..9349bf1e8d 100644
> > --- a/Changelog
> > +++ b/Changelog
> > @@ -11,7 +11,7 @@ version <next>:
> > - TiVo ty/ty+ demuxer
> > - Intel QSV-accelerated MJPEG encoding
> > - PCE support for extended channel layouts in the AAC encoder
> > -- native aptX encoder and decoder
> > +- native aptX and aptX HD encoder and decoder
> > - Raw aptX muxer and demuxer
> > - NVIDIA NVDEC-accelerated H.264, HEVC, MPEG-1/2/4, VC1, VP8/9 hwaccel
> > decoding
> > - Intel QSV-accelerated overlay filter
> > diff --git a/configure b/configure
> > index 1d2fffa132..c496346a06 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2459,6 +2459,8 @@ apng_encoder_deps="zlib"
> > apng_encoder_select="llvidencdsp"
> > aptx_decoder_select="audio_frame_queue"
> > aptx_encoder_select="audio_frame_queue"
> > +aptx_hd_decoder_select="audio_frame_queue"
> > +aptx_hd_encoder_select="audio_frame_queue"
> > asv1_decoder_select="blockdsp bswapdsp idctdsp"
> > asv1_encoder_select="bswapdsp fdctdsp pixblockdsp"
> > asv2_decoder_select="blockdsp bswapdsp idctdsp"
> > diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> > index cfacd6b70c..a9ecf7ea5e 100644
> > --- a/libavcodec/Makefile
> > +++ b/libavcodec/Makefile
> > @@ -190,6 +190,8 @@ OBJS-$(CONFIG_ANSI_DECODER) += ansi.o
> > cga_data.o
> > OBJS-$(CONFIG_APE_DECODER) += apedec.o
> > OBJS-$(CONFIG_APTX_DECODER) += aptx.o
> > OBJS-$(CONFIG_APTX_ENCODER) += aptx.o
> > +OBJS-$(CONFIG_APTX_HD_DECODER) += aptx.o
> > +OBJS-$(CONFIG_APTX_HD_ENCODER) += aptx.o
> > OBJS-$(CONFIG_APNG_DECODER) += png.o pngdec.o pngdsp.o
> > OBJS-$(CONFIG_APNG_ENCODER) += png.o pngenc.o
> > OBJS-$(CONFIG_SSA_DECODER) += assdec.o ass.o
> > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> > index ed1e7ab06e..93d31f8688 100644
> > --- a/libavcodec/allcodecs.c
> > +++ b/libavcodec/allcodecs.c
> > [...]
> > @@ -844,6 +1105,24 @@ AVCodec ff_aptx_decoder = {
> > };
> > #endif
> >
> > +#if CONFIG_APTX_HD_DECODER
> > +AVCodec ff_aptx_hd_decoder = {
> > + .name = "aptx_hd",
> > + .long_name = NULL_IF_CONFIG_SMALL("aptX HD (Audio
> > Processing Technology for Bluetooth)"),
> > + .type = AVMEDIA_TYPE_AUDIO,
> > + .id = AV_CODEC_ID_APTX_HD,
> > + .priv_data_size = sizeof(AptXContext),
> > + .init = aptx_init,
> > + .decode = aptx_decode_frame,
> > + .close = aptx_close,
> > + .capabilities = AV_CODEC_CAP_DR1,
> > + .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE,
> > + .channel_layouts = (const uint64_t[]) { AV_CH_LAYOUT_STEREO, 0},
> > + .sample_fmts = (const enum AVSampleFormat[]) {
> > AV_SAMPLE_FMT_S32P,
> > +
> > AV_SAMPLE_FMT_NONE },
> > +};
> > +#endif
> > +
> > #if CONFIG_APTX_ENCODER
> > AVCodec ff_aptx_encoder = {
> > .name = "aptx",
> > @@ -862,3 +1141,22 @@ AVCodec ff_aptx_encoder = {
> > .supported_samplerates = (const int[]) {8000, 16000, 24000, 32000,
> > 44100, 48000, 0},
> > };
> > #endif
> > +
> > +#if CONFIG_APTX_HD_ENCODER
> > +AVCodec ff_aptx_hd_encoder = {
> > + .name = "aptx_hd",
> > + .long_name = NULL_IF_CONFIG_SMALL("aptX HD (Audio
> > Processing Technology for Bluetooth)"),
> > + .type = AVMEDIA_TYPE_AUDIO,
> > + .id = AV_CODEC_ID_APTX_HD,
> > + .priv_data_size = sizeof(AptXContext),
> > + .init = aptx_init,
> > + .encode2 = aptx_encode_frame,
> > + .close = aptx_close,
> > + .capabilities = AV_CODEC_CAP_SMALL_LAST_FRAME,
> > + .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE,
> > + .channel_layouts = (const uint64_t[]) { AV_CH_LAYOUT_STEREO, 0},
> > + .sample_fmts = (const enum AVSampleFormat[]) {
> > AV_SAMPLE_FMT_S32P,
> > +
> > AV_SAMPLE_FMT_NONE },
> > + .supported_samplerates = (const int[]) {8000, 16000, 24000, 32000,
> > 44100, 48000, 0},
> > +};
> > +#endif
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index c13deb599f..95d164abc1 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -634,6 +634,7 @@ enum AVCodecID {
> > AV_CODEC_ID_ATRAC3PAL,
> > AV_CODEC_ID_DOLBY_E,
> > AV_CODEC_ID_APTX,
> > + AV_CODEC_ID_APTX_HD,
> >
> > /* subtitle codecs */
> > AV_CODEC_ID_FIRST_SUBTITLE = 0x17000, ///< A dummy ID
> > pointing at the start of subtitle codecs.
> > diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> > index c3688de1d6..ca18bb2b67 100644
> > --- a/libavcodec/codec_desc.c
> > +++ b/libavcodec/codec_desc.c
> > @@ -2866,6 +2866,13 @@ static const AVCodecDescriptor codec_descriptors[]
> > = {
> > .long_name = NULL_IF_CONFIG_SMALL("aptX (Audio Processing
> > Technology for Bluetooth)"),
> > .props = AV_CODEC_PROP_LOSSY,
> > },
> > + {
> > + .id = AV_CODEC_ID_APTX_HD,
> > + .type = AVMEDIA_TYPE_AUDIO,
> > + .name = "aptx_hd",
> > + .long_name = NULL_IF_CONFIG_SMALL("aptX HD (Audio Processing
> > Technology for Bluetooth)"),
> > + .props = AV_CODEC_PROP_LOSSY,
> > + },
> >
> > /* subtitle codecs */
> > {
>
>
> No, don't add a new codec ID for what is very obviously a profile.
>
> Here's what you need to do:
>
> 1.) Add FF_PROFILE_APTX_HD to libavcodec/avcodec.h
> 2.) During parsing set par->profile to FF_PROFILE_APTX_HD
Parsing ? Do you mean demuxer ?
There is no aptx parser for now.
> 3.) During decoding init set s->hd to avctx->profile == FF_PROFILE_APTX_HD
> - or better yet don't add a bool variable but do this check every time
> something is different
The bool variable makes things easier because the avctx is not
accessible to functions using it.
Anyway, I do understand how I could use a profile instead of a new codec
ID, but I really don't understand what advantage it would bring ?
For a codec known with one name, but supporting a lot of different
profiles with flags in the bitstream so that the decoder can adapt
itself to any profile, that makes a lot of sense. But for aptX and
aptX HD it doesn't make any sense to me.
I don't think it would make the code simpler.
The decoder itself has no flag in the bitstream to adapt to the correct
"profile".
And I think it would be confusing for end user. aptX HD is publicly know
as a different codec than aptX. A user looking at the list of supported
codec would probably conclude that aptX is supported but not aptX HD.
Also, the closest thing I can think as a standard container for aptX
is the bluetooth A2DP protocol. And in this protocol, aptX and aptX HD
are differentiated with a different codec ID, just the same way they
are differentiated from SBC or LDAC.
So in the end, using different codec ID seems pretty natural, while
using different profiles seems akward and doesn't seem to bring any
advantage.
Can you give any technical reason why think using profile would be
better ?
> Could you do this for sbc as well so we can get that merged finally?
I already replied to this in the SBC thread, and the discussion should
probably continue there, but in the case of SBC, using profile is
even more problematic as SBC and mSBC don't support the same samplerate,
and having a single codec prevent to have too different samplerate
lists and prevent ffmpeg to automatically convert input audio to the
only samplerate supported by mSBC.
More information about the ffmpeg-devel
mailing list