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

Rostislav Pehlivanov atomnuker at gmail.com
Sat Jan 20 20:55:13 EET 2018


On 20 January 2018 at 17:25, Aurelien Jacobs <aurel at gnuage.org> wrote:

> 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 ?
>

I think its sensible to specify a codec, always, and never rely on the
defaults. Especially for raw formats like 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).
>

Not handling, accepting. You currently have 2 muxers doing the same thing
but accepting different codec tags and extensions. You can make it 1 muxer
accepting multiple codec tags and extensions.



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

Then don't give them as an example that should be followed! That was my
point and you completely missed it.



>
> > 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 ?
>

There has never been such policy. Many things in lavf depend on lavc like
opus in mpegts.


More information about the ffmpeg-devel mailing list