Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Mon, Feb 05, 2018 at 12:27:20AM +0100, Michael Niedermayer wrote: > On Sun, Feb 04, 2018 at 04:07:26PM +0100, Aurelien Jacobs wrote: > > On Sat, Jan 20, 2018 at 11:20:22PM +, Rostislav Pehlivanov wrote: > > > On 20 January 2018 at 17:26, Aurelien Jacobs wrote: > > > > > > > On Sun, Jan 14, 2018 at 10:54:34PM +0100, Carl Eugen Hoyos wrote: > > > > > 2018-01-14 14:06 GMT+01:00 Aurelien Jacobs : > > > > > > > > > > > Well, here is an updated patch which uses codec tags for the decoder > > > > and > > > > > > profile for the encoder. > > > > > > > > > > Sorry but I object to this patch: > > > > > We should not invent codec_tags. > > > > > > > > OK, I understand, and I agree. > > > > > > > > But now we are in an interlocking situation. > > > > We have 2 solutions to handle aptX vs. aptX HD but those 2 solutions > > > > have > > > > been rejected by 2 different person. > > > > > > > > Do anybody have a 3rd solution, that everyone would accept ? > > > > > > > > And if not, how do we resolve this ? > > > > Is there any policy nowadays to handle this kind of interlocking ? > > > > > > > > > > Fine, I see no choice but to use multiple codec IDs for aptxhd since the > > > format really provides you with nothing bitstream wise to determine what > > > it > > > is. Even game codecs have a bit or two for version. > > > > OK. Good. > > > Now, will someone push the original patchset ? > > I think you should be able to push it yourself if there are no review > comments remaining > you are listed in MAINTAINERS for some parts and i see a "ajacobs" for ffmpeg > so you should have git write access OK. Well I digged out my ssh key from old laptop, an I pushed it myself. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Sun, Feb 04, 2018 at 04:07:26PM +0100, Aurelien Jacobs wrote: > On Sat, Jan 20, 2018 at 11:20:22PM +, Rostislav Pehlivanov wrote: > > On 20 January 2018 at 17:26, Aurelien Jacobs wrote: > > > > > On Sun, Jan 14, 2018 at 10:54:34PM +0100, Carl Eugen Hoyos wrote: > > > > 2018-01-14 14:06 GMT+01:00 Aurelien Jacobs : > > > > > > > > > Well, here is an updated patch which uses codec tags for the decoder > > > and > > > > > profile for the encoder. > > > > > > > > Sorry but I object to this patch: > > > > We should not invent codec_tags. > > > > > > OK, I understand, and I agree. > > > > > > But now we are in an interlocking situation. > > > We have 2 solutions to handle aptX vs. aptX HD but those 2 solutions have > > > been rejected by 2 different person. > > > > > > Do anybody have a 3rd solution, that everyone would accept ? > > > > > > And if not, how do we resolve this ? > > > Is there any policy nowadays to handle this kind of interlocking ? > > > > > > > Fine, I see no choice but to use multiple codec IDs for aptxhd since the > > format really provides you with nothing bitstream wise to determine what it > > is. Even game codecs have a bit or two for version. > > OK. Good. > Now, will someone push the original patchset ? I think you should be able to push it yourself if there are no review comments remaining you are listed in MAINTAINERS for some parts and i see a "ajacobs" for ffmpeg so you should have git write access > Or should I rebase it and submt it again ? > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB it is not once nor twice but times without number that the same ideas make their appearance in the world. -- Aristotle signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Sat, Jan 20, 2018 at 11:20:22PM +, Rostislav Pehlivanov wrote: > On 20 January 2018 at 17:26, Aurelien Jacobs wrote: > > > On Sun, Jan 14, 2018 at 10:54:34PM +0100, Carl Eugen Hoyos wrote: > > > 2018-01-14 14:06 GMT+01:00 Aurelien Jacobs : > > > > > > > Well, here is an updated patch which uses codec tags for the decoder > > and > > > > profile for the encoder. > > > > > > Sorry but I object to this patch: > > > We should not invent codec_tags. > > > > OK, I understand, and I agree. > > > > But now we are in an interlocking situation. > > We have 2 solutions to handle aptX vs. aptX HD but those 2 solutions have > > been rejected by 2 different person. > > > > Do anybody have a 3rd solution, that everyone would accept ? > > > > And if not, how do we resolve this ? > > Is there any policy nowadays to handle this kind of interlocking ? > > > > Fine, I see no choice but to use multiple codec IDs for aptxhd since the > format really provides you with nothing bitstream wise to determine what it > is. Even game codecs have a bit or two for version. OK. Good. Now, will someone push the original patchset ? Or should I rebase it and submt it again ? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 20 January 2018 at 17:26, Aurelien Jacobs wrote: > On Sun, Jan 14, 2018 at 10:54:34PM +0100, Carl Eugen Hoyos wrote: > > 2018-01-14 14:06 GMT+01:00 Aurelien Jacobs : > > > > > Well, here is an updated patch which uses codec tags for the decoder > and > > > profile for the encoder. > > > > Sorry but I object to this patch: > > We should not invent codec_tags. > > OK, I understand, and I agree. > > But now we are in an interlocking situation. > We have 2 solutions to handle aptX vs. aptX HD but those 2 solutions have > been rejected by 2 different person. > > Do anybody have a 3rd solution, that everyone would accept ? > > And if not, how do we resolve this ? > Is there any policy nowadays to handle this kind of interlocking ? > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > Fine, I see no choice but to use multiple codec IDs for aptxhd since the format really provides you with nothing bitstream wise to determine what it is. Even game codecs have a bit or two for version. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 20 January 2018 at 17:25, Aurelien Jacobs wrote: > On Sun, Jan 14, 2018 at 05:19:12PM +, Rostislav Pehlivanov wrote: > > On 14 January 2018 at 13:06, Aurelien Jacobs wrote: > > > > > On Tue, Jan 09, 2018 at 02:21:02PM +, Rostislav Pehlivanov wrote: > > > > On 9 January 2018 at 14:07, Rostislav Pehlivanov < > atomnu...@gmail.com> > > > > wrote: > > > > > > > > > > > > > > > > > > > On 9 January 2018 at 09:00, Hendrik Leppkes > > > wrote: > > > > > > > > > >> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes < > h.lepp...@gmail.com> > > > > >> wrote: > > > > >> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov > > > > >> > 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
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Sun, Jan 14, 2018 at 10:54:34PM +0100, Carl Eugen Hoyos wrote: > 2018-01-14 14:06 GMT+01:00 Aurelien Jacobs : > > > Well, here is an updated patch which uses codec tags for the decoder and > > profile for the encoder. > > Sorry but I object to this patch: > We should not invent codec_tags. OK, I understand, and I agree. But now we are in an interlocking situation. We have 2 solutions to handle aptX vs. aptX HD but those 2 solutions have been rejected by 2 different person. Do anybody have a 3rd solution, that everyone would accept ? And if not, how do we resolve this ? Is there any policy nowadays to handle this kind of interlocking ? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Sun, Jan 14, 2018 at 05:19:12PM +, Rostislav Pehlivanov wrote: > On 14 January 2018 at 13:06, Aurelien Jacobs wrote: > > > On Tue, Jan 09, 2018 at 02:21:02PM +, Rostislav Pehlivanov wrote: > > > On 9 January 2018 at 14:07, Rostislav Pehlivanov > > > wrote: > > > > > > > > > > > > > > > On 9 January 2018 at 09:00, Hendrik Leppkes > > wrote: > > > > > > > >> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes > > > >> wrote: > > > >> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov > > > >> > 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. Ser
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
2018-01-14 14:06 GMT+01:00 Aurelien Jacobs : > Well, here is an updated patch which uses codec tags for the decoder and > profile for the encoder. Sorry but I object to this patch: We should not invent codec_tags. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 14 January 2018 at 13:06, Aurelien Jacobs wrote: > On Tue, Jan 09, 2018 at 02:21:02PM +, Rostislav Pehlivanov wrote: > > On 9 January 2018 at 14:07, Rostislav Pehlivanov > > wrote: > > > > > > > > > > > On 9 January 2018 at 09:00, Hendrik Leppkes > wrote: > > > > > >> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes > > >> wrote: > > >> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov > > >> > 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 0x004F > +#define A2DP_APT_CODEC_ID_APTX 0x0001 > + > +#define A2DP_VENDOR_ID_QUALCOMM 0x00D7 > +#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, th
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Tue, Jan 09, 2018 at 02:21:02PM +, Rostislav Pehlivanov wrote: > On 9 January 2018 at 14:07, Rostislav Pehlivanov > wrote: > > > > > > > On 9 January 2018 at 09:00, Hendrik Leppkes wrote: > > > >> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes > >> wrote: > >> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov > >> > 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 Without this -profile parameter, the encoding will just fail but user will have to guess how to fix it. 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 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. But anyway, if there is a consensus for using a single codec ID, this new patch can be applied. >From bdf5d5f4a3724a70efe0cad76179a57619c6fad0 Mon Sep 17 00:00:00 2001 From: Aurelien Jacobs Date: Sat, 6 Jan 2018 17:11:48 +0100 Subject: [PATCH 4/5] aptx: implement the aptX HD bluetooth codec --- Changelog | 2 +- libavcodec/Makefile | 2 + libavcodec/aptx.c | 352 +- libavcodec/avcodec.h | 3 + libavcodec/profiles.c | 6 + libavcodec/profiles.h | 1 + 6 files changed, 336 insertions(+), 30 deletions(-) diff --git a/Changelog b/Changelog index 3d966c202b..9349bf1e8d 100644 --- a/Changelog +++ b/Changelog @@ -11,7 +11,7 @@ version : - 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/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/aptx.c b/
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Tue, Jan 9, 2018 at 3:21 PM, Rostislav Pehlivanov wrote: > On 9 January 2018 at 14:07, Rostislav Pehlivanov > wrote: > >> >> >> On 9 January 2018 at 09:00, Hendrik Leppkes wrote: >> >>> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes >>> wrote: >>> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov >>> > 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) >>> >>> - Hendrik >> >> I'd be fine with using codec tags, but not with codec IDs. >> > > Though for encoding using profiles would be best. For encoding, profiles is perfectly normal and expected, and used by many other codecs as well. Just for decoding that field is not defined to be required to be set externally to even make decoding of a certain stream possible. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 9 January 2018 at 14:07, Rostislav Pehlivanov wrote: > > > On 9 January 2018 at 09:00, Hendrik Leppkes wrote: > >> On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes >> wrote: >> > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov >> > 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) >> >> - Hendrik >> ___ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > > I'd be fine with using codec tags, but not with codec IDs. > Though for encoding using profiles would be best. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 9 January 2018 at 09:00, Hendrik Leppkes wrote: > On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes > wrote: > > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov > > 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) > > - Hendrik > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > I'd be fine with using codec tags, but not with codec IDs. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Tue, Jan 9, 2018 at 9:33 AM, Hendrik Leppkes wrote: > On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov > 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. 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) - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Tue, Jan 9, 2018 at 5:07 AM, Rostislav Pehlivanov 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. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
2018-01-09 5:07 GMT+01:00 Rostislav Pehlivanov : > On 9 January 2018 at 01:21, Aurelien Jacobs wrote: >> So what ? It won't increase binary bloat unless we ever reach more >> than 2^32 codecs. >> And regarding public API bloat, the 2 options are: >> 1) define 2 codec ID consts >> 2) define 1 codec ID const and 2 profile consts >> Which one is bloating the public API more ? > > The first one. No? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 9 January 2018 at 01:21, Aurelien Jacobs wrote: > On Mon, Jan 08, 2018 at 12:38:19AM +, Rostislav Pehlivanov wrote: > > On 7 January 2018 at 22:54, Aurelien Jacobs wrote: > > > > > On Sun, Jan 07, 2018 at 05:23:24PM +, Rostislav Pehlivanov wrote: > > > > On 6 January 2018 at 16:48, Aurelien Jacobs > 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(-) > > > > > > > > > > > > No, don't add a new codec ID for what is very obviously a profile. > > > > > > [...] > > > 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. > > > > > > > It makes sense - HD is just a flavor of aptx. You can't call this a brand > > new codec - it just changes some tables and the way codewords are written > > (1 function). > > Of course it does make sense to us, the few people who are touching > codecs source code, that they are both flavors of the same code. > > > The only people who would call that brand new are in > > marketing and they're wrong. > > Of course calling aptX and aptX HD different codecs is pretty much > marketting bullshit. > > But that's not the point here. > We are not talking about some internal implementation details. > We are talking about ffmpeg's end user interface. So what we do > has to make sense to end user. And end users "knows very well" that > aptX and aptX HD are 2 different codecs (they are used for exemple > to install 2 differents libs on android to support both). > > They're not 2 different codecs, regardless of what is being said. The similarities are unquestionable. You even said it yourself. Now you're just bikeshedding because you've already done the patches one way and you don't want to change them to something new and unfamiliar. > > > 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 ? > > > > > > > Yes, a big one - we don't bloat the already humongous API. You might say: > > "Well, we have hundreds of codec IDs, what's one more?". If we had a new > > codec ID for every flavor of every codec, we'd have thousands. > > So what ? It won't increase binary bloat unless we ever reach more > than 2^32 codecs. > And regarding public API bloat, the 2 options are: > 1) define 2 codec ID consts > 2) define 1 codec ID const and 2 profile consts > Which one is bloating the public API more ? > The first one. > > > The profile systems allows us to discern between actually marketed > variants > > of one codec with the same name, for example AAC-LC with AAC-LTP. > > Do you really think that AAC-LTP is marketed as a unique codec to end > users ? I've never stumbled on this. > I absolutely do not. Hence I don't see aptx HD being marketed as a unique codec. > 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. You can keep refusing to change the patches and maintain that they're separate codecs. However other people who see things the other way are also free to tak
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Mon, Jan 08, 2018 at 12:38:19AM +, Rostislav Pehlivanov wrote: > On 7 January 2018 at 22:54, Aurelien Jacobs wrote: > > > On Sun, Jan 07, 2018 at 05:23:24PM +, Rostislav Pehlivanov wrote: > > > On 6 January 2018 at 16:48, Aurelien Jacobs 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(-) > > > > > > > > > No, don't add a new codec ID for what is very obviously a profile. > > > > [...] > > 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. > > > > It makes sense - HD is just a flavor of aptx. You can't call this a brand > new codec - it just changes some tables and the way codewords are written > (1 function). Of course it does make sense to us, the few people who are touching codecs source code, that they are both flavors of the same code. > The only people who would call that brand new are in > marketing and they're wrong. Of course calling aptX and aptX HD different codecs is pretty much marketting bullshit. But that's not the point here. We are not talking about some internal implementation details. We are talking about ffmpeg's end user interface. So what we do has to make sense to end user. And end users "knows very well" that aptX and aptX HD are 2 different codecs (they are used for exemple to install 2 differents libs on android to support both). > > 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 ? > > > > Yes, a big one - we don't bloat the already humongous API. You might say: > "Well, we have hundreds of codec IDs, what's one more?". If we had a new > codec ID for every flavor of every codec, we'd have thousands. So what ? It won't increase binary bloat unless we ever reach more than 2^32 codecs. And regarding public API bloat, the 2 options are: 1) define 2 codec ID consts 2) define 1 codec ID const and 2 profile consts Which one is bloating the public API more ? > The profile systems allows us to discern between actually marketed variants > of one codec with the same name, for example AAC-LC with AAC-LTP. Do you really think that AAC-LTP is marketed as a unique codec to end users ? I've never stumbled on this. Anyway, all this discussion is moot as Hendrik pointed out that profile can't be set outside of lavc to determine a decoder behavior. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Mon, Jan 08, 2018 at 01:27:16PM +0100, Carl Eugen Hoyos wrote: > 2018-01-08 11:32 GMT+01:00 Hendrik Leppkes : > > On Mon, Jan 8, 2018 at 1:38 AM, Rostislav Pehlivanov > > wrote: > >> > >> That's okay - for encoding switch the profile depending on both the > >> avctx->profile setting and the samplerate and list all supported > >> samplerates for all profiles in the AVCodec structure. We do something > >> similar in the AAC encoder where we pick different settings depending on > >> the profile and change the profile depending on the settings listed. > >> If there's an overlap, pick a sane default unless the user forces a profile > >> using profile:a. If forced and the samplerate isn't supported - error out > >> and let the user know. > >> For decoding set the profile via the demuxer. There doesn't have to be just > >> one demuxer for a codec. If there are major changes in how you parse > >> profiles go ahead and make a new one which sets the codec ID and the > >> profile. > >> > > > > I don't think there is any precedent for "profile" being mandatory for > > a decoder to work, so that might be iffy. Decoders usually set > > profile, don't require it. > > ie. from the docs: > > * - decoding: Set by libavcodec. Oh, very true. I guess that settles it for "profile". Public API do not allows using profile to select the appropriate decoder. > > There is plenty precedent for using "codec_tag" however, so that may > > be a better choice - and can hold the tag from the "container" as-is > > as well. > > While I don't understand why even having more than one codec_id for > the same codec (which afaiu isn't the case here) would be an issue, Same for me, I don't understand why adding a codec_id would be an issue. > I don't think we should invent codec_tags unless necessary. I agree. And I don't think there is a container used for aptX or aptX HD in the wild with some kind of codec_tag. So this is probably not an option either. I maintain that using 2 codec IDs is the most appropriate in this specific case. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
2018-01-08 13:37 GMT+01:00 Hendrik Leppkes : > On Mon, Jan 8, 2018 at 1:27 PM, Carl Eugen Hoyos wrote: >> 2018-01-08 11:32 GMT+01:00 Hendrik Leppkes : >>> On Mon, Jan 8, 2018 at 1:38 AM, Rostislav Pehlivanov >>> wrote: That's okay - for encoding switch the profile depending on both the avctx->profile setting and the samplerate and list all supported samplerates for all profiles in the AVCodec structure. We do something similar in the AAC encoder where we pick different settings depending on the profile and change the profile depending on the settings listed. If there's an overlap, pick a sane default unless the user forces a profile using profile:a. If forced and the samplerate isn't supported - error out and let the user know. For decoding set the profile via the demuxer. There doesn't have to be just one demuxer for a codec. If there are major changes in how you parse profiles go ahead and make a new one which sets the codec ID and the profile. >>> >>> I don't think there is any precedent for "profile" being mandatory for >>> a decoder to work, so that might be iffy. Decoders usually set >>> profile, don't require it. >>> ie. from the docs: >>> * - decoding: Set by libavcodec. >>> >>> There is plenty precedent for using "codec_tag" however, so that may >>> be a better choice - and can hold the tag from the "container" as-is >>> as well. >> >> While I don't understand why even having more than one codec_id for >> the same codec (which afaiu isn't the case here) would be an issue, I >> don't think we should invent codec_tags unless necessary. >> > > We're not inventing them if they are a copy from the container > bitstream tag field. I completely agree! Is there a container bitstream tag field in this case? Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Mon, Jan 8, 2018 at 1:27 PM, Carl Eugen Hoyos wrote: > 2018-01-08 11:32 GMT+01:00 Hendrik Leppkes : >> On Mon, Jan 8, 2018 at 1:38 AM, Rostislav Pehlivanov >> wrote: >>> >>> That's okay - for encoding switch the profile depending on both the >>> avctx->profile setting and the samplerate and list all supported >>> samplerates for all profiles in the AVCodec structure. We do something >>> similar in the AAC encoder where we pick different settings depending on >>> the profile and change the profile depending on the settings listed. >>> If there's an overlap, pick a sane default unless the user forces a profile >>> using profile:a. If forced and the samplerate isn't supported - error out >>> and let the user know. >>> For decoding set the profile via the demuxer. There doesn't have to be just >>> one demuxer for a codec. If there are major changes in how you parse >>> profiles go ahead and make a new one which sets the codec ID and the >>> profile. >>> >> >> I don't think there is any precedent for "profile" being mandatory for >> a decoder to work, so that might be iffy. Decoders usually set >> profile, don't require it. >> ie. from the docs: >> * - decoding: Set by libavcodec. >> >> There is plenty precedent for using "codec_tag" however, so that may >> be a better choice - and can hold the tag from the "container" as-is >> as well. > > While I don't understand why even having more than one codec_id for > the same codec (which afaiu isn't the case here) would be an issue, I > don't think we should invent codec_tags unless necessary. > We're not inventing them if they are a copy from the container bitstream tag field. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
2018-01-08 11:32 GMT+01:00 Hendrik Leppkes : > On Mon, Jan 8, 2018 at 1:38 AM, Rostislav Pehlivanov > wrote: >> >> That's okay - for encoding switch the profile depending on both the >> avctx->profile setting and the samplerate and list all supported >> samplerates for all profiles in the AVCodec structure. We do something >> similar in the AAC encoder where we pick different settings depending on >> the profile and change the profile depending on the settings listed. >> If there's an overlap, pick a sane default unless the user forces a profile >> using profile:a. If forced and the samplerate isn't supported - error out >> and let the user know. >> For decoding set the profile via the demuxer. There doesn't have to be just >> one demuxer for a codec. If there are major changes in how you parse >> profiles go ahead and make a new one which sets the codec ID and the >> profile. >> > > I don't think there is any precedent for "profile" being mandatory for > a decoder to work, so that might be iffy. Decoders usually set > profile, don't require it. > ie. from the docs: > * - decoding: Set by libavcodec. > > There is plenty precedent for using "codec_tag" however, so that may > be a better choice - and can hold the tag from the "container" as-is > as well. While I don't understand why even having more than one codec_id for the same codec (which afaiu isn't the case here) would be an issue, I don't think we should invent codec_tags unless necessary. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Mon, Jan 8, 2018 at 1:38 AM, Rostislav Pehlivanov wrote: > > That's okay - for encoding switch the profile depending on both the > avctx->profile setting and the samplerate and list all supported > samplerates for all profiles in the AVCodec structure. We do something > similar in the AAC encoder where we pick different settings depending on > the profile and change the profile depending on the settings listed. > If there's an overlap, pick a sane default unless the user forces a profile > using profile:a. If forced and the samplerate isn't supported - error out > and let the user know. > For decoding set the profile via the demuxer. There doesn't have to be just > one demuxer for a codec. If there are major changes in how you parse > profiles go ahead and make a new one which sets the codec ID and the > profile. > I don't think there is any precedent for "profile" being mandatory for a decoder to work, so that might be iffy. Decoders usually set profile, don't require it. ie. from the docs: * - decoding: Set by libavcodec. There is plenty precedent for using "codec_tag" however, so that may be a better choice - and can hold the tag from the "container" as-is as well. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Mon, 8 Jan 2018 00:38:19 +, Rostislav Pehlivanov wrote: > > > No, don't add a new codec ID for what is very obviously a profile. > > 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 ? > > itself to any profile, that makes a lot of sense. But for aptX and > > aptX HD it doesn't make any sense to me. in ffmpeg we generally try not to create new original internal fourcc / isom tags. because then people think our tag is the real spec tag, and then bad things happen. if another software uses the tag or the sample of that tag is in the wild then we use it. i dont know enough about aptx / aptx hd to make an opinion. some codecs make different tags for stupid things, like sony hdcam has different tags for different fps. we support these tags because other software uses them and samples exist in the wild. not sure if i've helped here. -compn ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 7 January 2018 at 22:54, Aurelien Jacobs wrote: > On Sun, Jan 07, 2018 at 05:23:24PM +, Rostislav Pehlivanov wrote: > > On 6 January 2018 at 16:48, Aurelien Jacobs 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(-) > > > > > > 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. > Sorry, I meant demuxer. > > 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. > That's okay, I don't mind. > 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. > It makes sense - HD is just a flavor of aptx. You can't call this a brand new codec - it just changes some tables and the way codewords are written (1 function). The only people who would call that brand new are in marketing and they're wrong. This is just changes the tables and the way codewords are written in order to optimize the codec for a different bitrate. That's all, not a new codec, its too small a change. Its a profile. > 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 ? > Yes, a big one - we don't bloat the already humongous API. You might say: "Well, we have hundreds of codec IDs, what's one more?". If we had a new codec ID for every flavor of every codec, we'd have thousands. The profile system saves us a decent dozen for when there are differences besides the "there's one extra block of bits/there's some reordering" - which remain hidden by the decoder. In that case, as far as the user is concerned they're not decoding a Chinese Y528 codec, an H264 variant with 1 extra bit of padding found in one province's TV broadcast to region lock, they're decoding regular H264. The profile systems allows us to discern between actually marketed variants of one codec with the same name, for example AAC-LC with AAC-LTP. The LTP profile of AAC is quite similar in what it aims to do compared to aptx HD - its just the same baseline codec with some changes to make it work better for a particular bitrate. > > 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. > That's okay - for encoding switch the profile depending on both the avctx->profile setting and the samplerate and list all supported samplerates for all profiles in the AVCodec structure. We do something similar in the AAC encoder where we pick different settings depending on the profile and change the profile depending on the settings listed. If there's an overlap, pick a sane default unless the user forces a profile using profile:a. If forced and the samplerate isn't supported - error out and let the user know. For decoding set the profile via the demuxer. There doesn't have to be just one demuxer for a codec. If there are major changes in how you
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On Sun, Jan 07, 2018 at 05:23:24PM +, Rostislav Pehlivanov wrote:
> On 6 January 2018 at 16:48, Aurelien Jacobs 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 :
> > - 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,
Re: [FFmpeg-devel] [PATCH 4/5] aptx: implement the aptX HD bluetooth codec
On 6 January 2018 at 16:48, Aurelien Jacobs 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 :
> - 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
> @@ -333,6 +333,7 @@ static void register_all(void)
> REGISTER_DECODER(AMRWB, amrwb);
> REGISTER_DECODER(APE, ape);
> REGISTER_ENCDEC (APTX, aptx);
> +REGISTER_ENCDEC (APTX_HD, aptx_hd);
> REGISTER_DECODER(ATRAC1,atrac1);
> REGISTER_DECODER(ATRAC3,atrac3);
> REGISTER_DECODER(ATRAC3AL, atrac3al);
> diff --git a/libavcodec/aptx.c b/libavcodec/aptx.c
> index 4173402d03..6c0f3d35a9 100644
> --- a/libavcodec/aptx.c
> +++ b/libavcodec/aptx.c
> @@ -89,6 +89,8 @@ typedef struct {
> } Channel;
>
> typedef struct {
> +int hd;
> +int block_size;
> int32_t sync_idx;
> Channel channels[NB_CHANNELS];
> AudioFrameQueue afq;
> @@ -182,6 +184,205 @@ static const int16_t quantize_factor_select_offset_HF[5]
> = {
> 0, -8, 33, 95, 262,
> };
>
> +
> +static const int32_t hd_quantize_intervals_LF[257] = {
> + -2436,2436,7308, 12180, 17054, 21930, 26806,
> 31686,
> + 36566, 41450, 46338, 51230, 56124, 61024, 65928,
> 70836,
> + 75750, 80670, 85598, 90530, 95470, 100418, 105372,
> 110336,
> + 115308, 120288, 125278, 130276, 135286, 140304, 145334,
> 150374,
> + 155426, 160490, 165566, 170654, 175756, 180870, 185998,
> 191138,
> + 196294, 201466, 206650, 211850, 217068, 222300, 227548,
> 232814,
> + 238096, 243396, 248714, 254050, 259406, 264778, 270172,
> 275584,
> + 281018, 286470, 291944, 297440, 302956, 308496, 314056,
> 319640,
> + 325248, 330878, 336532, 342212, 347916, 353644, 359398,
> 365178,
> + 370986, 376820, 382680, 388568, 394486, 400430, 406404,
> 412408,
> + 418442, 424506, 430600, 436726, 442884, 449074, 455298,
> 461554,
> + 467844, 474168, 480528, 486922, 493354, 499820, 506324,
> 512866,
> + 519446, 526064, 532722, 539420, 546160, 552940, 559760,
> 566624,
> + 573532, 580482, 587478, 594520, 601606, 608740, 615920,
> 623148,
> + 630426, 637754, 645132, 652560, 660042, 667576, 675164,
> 682808,
> + 690506, 698262, 706074, 713946, 721876, 729868, 737920,
> 746036,
> + 754216, 762460, 770770, 779148, 787594, 796108, 804694,
> 813354,
> + 822086, 830892, 839774, 848736, 857776, 866896, 876100,
> 885386,
> + 894758, 904218, 913766, 923406, 933138, 942964, 952886,
> 962908,
> + 973030, 983254, 993582, 1004020, 1014566, 1025224, 1035996,
> 1046886,
> +1057894, 1069026, 1080284, 1091670, 1103186, 1114838, 1126628,
> 1138558,
> +1150634, 1162858, 1175236, 1187768, 1200462, 1213320, 122
