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

[Rostislav Pehlivanov]

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


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. There's no difference in how either are encoded since its
all raw. Sure, it prevents strictly mandating that an aptxhd extension will
always carry aptxhd bitstreams, but what do you expect, its a raw format
and there are no guarantees that extensions always mandate what's in a
file. That's why we probe.
I'm fine with either having a separate muxer or a common one with multiple
extensions, so I don't mind things as they are here.



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



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


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.


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


-    .video_codec       = AV_CODEC_ID_NONE,
>

Why?

That aside, patches look good to me.