[FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec

[Aurelien Jacobs]

On Sun, Jan 14, 2018 at 05:19:12PM +0000, Rostislav Pehlivanov wrote:
> On 14 January 2018 at 13:06, Aurelien Jacobs <aurel at gnuage.org> wrote:
> 
> > On Tue, Jan 09, 2018 at 02:21:02PM +0000, Rostislav Pehlivanov wrote:
> > > On 9 January 2018 at 14:07, Rostislav Pehlivanov <atomnuker at gmail.com>
> > > wrote:
> > >
> > > >
> > > >
> > > > On 9 January 2018 at 09:00, Hendrik Leppkes <h.leppkes at gmail.com>
> > wrote:
> > > >
> > > >> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes <h.leppkes at gmail.com>
> > > >> wrote:
> > > >> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov
> > > >> > <atomnuker at gmail.com> wrote:
> > > >> >>
> > > >> >>> Anyway, all this discussion is moot as Hendrik pointed out that
> > > >> profile
> > > >> >>> can't be set outside of lavc to determine a decoder behavior.
> > > >> >>>
> > > >> >>
> > > >> >> What, based on a comment in lavc? Comments there describe the api
> > but
> > > >> they
> > > >> >> rarely define it. We're free to adjust them if need be and in this
> > > >> case,
> > > >> >> since the codec profile may not be set, the API user is forced to
> > deal
> > > >> with
> > > >> >> the lack of such set. Hence, we can clarify that it may be set by
> > lavf
> > > >> as
> > > >> >> well as lavc as well as not being set at all. And the decoder may
> > use
> > > >> it.
> > > >> >> I maintain that adding a new codec ID for this is unacceptable.
> > > >> >>
> > > >> >
> > > >> > We already have established methods to select codec sub-variants, we
> > > >> > don't need to invent a new one just because you feel like it.
> > > >> >
> > > >>
> > > >> On that note, we also have cases where the same codec has different
> > > >> codec ids based on popular known names.
> > > >> For example wmv3/vc1 - wmv3 is simple/main profile, vc1 is advanced
> > > >> profile, different codec ids, same implementation.
> > > >>
> > > >> Re-defining public API is what is unacceptable, it requires every
> > > >> caller of lavc to suddenly start handling one more field for no real
> > > >> reason.
> > > >>
> > > >
> > > > Then its a good thing I suggested something that doesn't involve having
> > > > every caller of lavc to handle another field.
> > > >
> > > >
> > > >
> > > >> Either use a separate codec ID if there is sufficient reason for that
> > > >> (mostly driven by external factors, if its handled as different codecs
> > > >> everywhere else, not only in marketing but actual (reference)
> > > >> implementations), or use a codec_tag if one is available (the codec id
> > > >> field from a2dp for example)
> > > >
> > > > I'd be fine with using codec tags, but not with codec IDs.
> > >
> > > Though for encoding using profiles would be best.
> >
> > Well, here is an updated patch which uses codec tags for the decoder and
> > profile for the encoder.
> >
> > I still maintain that this solution is worse than using separate
> > codec IDs.
> > It is worse for developper / maintainer as it adds a bit of compexity to
> > the code.
> >
> > It is worse for user as it requires adding a mandatory parameter for
> > encoding to aptX HD:
> >   ffmpeg -i sample.wav -profile:a 1 sample.aptxhd
> >
> 
> As opposed to having a mandatory parameter for encoder like -c:a
> aptx_hd?

No. As opposed to:
ffmpeg -i sample.wav sample.aptxhd

> Perhaps we should encode everything to aptx hd by default to not have to
> mess about by confusing users with these codecs or profiles things.

?
It is sensible to use the aptxhd encoder by default when muxing to raw
aptxhd. And it is sensible to use the aptx encoder by default when
muxing to raw aptx.
Or do you disagree about this ?

> > Without this -profile parameter, the encoding will just fail but user
> > will have to guess how to fix it.
> >
> 
> Here's how you could fix it: don't have separate muxers for aptx and
> aptxhd. Make the aptx muxer handle both .aptx and .aptxhd extensions and
> the codec tags.

Muxer handling the codec tags ? What do you mean ?
A muxer can't pass a codec tag to an encoder (encoder's init is called
before the muxer's init).

> > And the reported stream is much less explicit:
> >   Stream #0:0: Audio: aptx ([36][0][215][0] / 0xD70024), 48000 Hz, 2
> > channels, s32p
> > vs.
> >   Stream #0:0: Audio: aptx_hd, 48000 Hz, 2 channels, s32p
> >
> 
> Profiles are still explicit
> "Stream #0:0: Audio: aac (LC), 44100 Hz, stereo, fltp (16 bit), 128 kb/s"
> vs
> "Stream #0:0: Audio: aac (Main), 44100 Hz, stereo, fltp (16 bit), 128 kb/s"

Well, this doesn't work here for aptX. I havn't checked why. Maybe codec
tags is printed rather than profile, when present.

> > So for the good of users and maintainers, I suggest to follow the
> > wmv3/vc1 example, that is to acknowledge that aptX and aptX HD are
> > commonly used as 2 separate codecs (search for
> > libaptX-1.0.0-rel-Android21-ARMv7A.so and
> > libaptXHD-1.0.0-rel-Android21-ARMv7A.so for example) and use the
> > original patch with 2 codec IDs.
> >
> 
> Ah, android, a shining beacon of well-written, small, generic code, always
> happy to reuse code and other people's work. No, wait, the other way around.

Seriously ? This is just the way those two codecs are released by
Qualcomm. The 2 above mentioned libs are just a common example, but they
release 2 separate libs for each platform they support, so the fact that
they are commonly known as 2 separate codecs is not related to Android
at all.

> Anyway, code-wise, there's one or two minor issues:
> 
> +#define A2DP_VENDOR_ID_APT              0x0000004F
> > +#define A2DP_APT_CODEC_ID_APTX          0x0001
> > +
> > +#define A2DP_VENDOR_ID_QUALCOMM         0x000000D7
> > +#define A2DP_QUALCOMM_CODEC_ID_APTX_HD  0x0024
> >
> 
> You have 3 copies of those in both this patch and the muxer/demuxer one.
> Make libavcodec/aptx.h and put those there, then include it from lavf.

Yeah, maybe. But I seem to remember that lavf was not allowed to depend
on non-public headers from lavc (and I'm sure you don't want to make
this header part of the public API).
Am I remembering wrong ? Or has this policy changed ?

> > +OBJS-$(CONFIG_APTX_HD_DECODER)         += aptx.o
> > +OBJS-$(CONFIG_APTX_HD_ENCODER)         += aptx.o
> 
> No point in having those flags now that there's one encoder/decoder
> handling both.

Oh, right. This is just a leftover. Should be removed.

> > -    .video_codec       = AV_CODEC_ID_NONE,
> 
> Why?

Just to match the aptxhd muxer. But that should probably be in a
separate patch.
(it is useless to explicitely set video_codec to 0, just the same reason
why we don't explicitely set subtitle_codec or data_codec)

> That aside, patches look good to me.

But it is also already rejected so no much point updating it for now.