On 20 January 2018 at 17:25, Aurelien Jacobs <au...@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 <au...@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 < > atomnu...@gmail.com> > > > > wrote: > > > > > > > > > > > > > > > > > > > On 9 January 2018 at 09:00, Hendrik Leppkes <h.lepp...@gmail.com> > > > 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 > > > > >> > <atomnu...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
