Skip to content

Loading…

[AE] drop AE_FMT_LPCM from non-CoreAudio builds #1512

Closed
wants to merge 1 commit into from

6 participants

@anssih
Team Kodi member

While looking at AudioEngine I got confused by AE_FMT_LPCM and what it is used for. Turns out it actually seems to only be used as a hack on CoreAudio to enable a "hog" mode needed for multichannel (for some reason this is used instead of looking directly at the channel count).

To avoid future confusion, this will make AE_FMT_LPCM depend on TARGET_DARWIN. I guess one other alternative would be to simply rename it to something that clearly indicates its purpose.

Detailed commit message below:

AE has a separate AEDataFormat AE_FMT_LPCM for multichannel PCM audio.
However, the data content is actually the same as AE_FMT_FLOAT, and
AE_FMT_LPCM is only used by CoreAudioAE.

CoreAudio uses the AE_FMT_FLOAT <=> AE_FMT_LPCM differentation to enter
a special "hog" mode, which is reportedly needed for multichannel HDMI
playback on CoreAudio. AE_FMT_LPCM mode is entered on CoreAudio systems
instead of AE_FMT_FLOAT (or other regular modes) when the multichannel
LPCM playback option is enabled in audio settings and the output device
type is HDMI.

Since AE_FMT_LPCM has no meaning outside CoreAudio, clearly mark it as
CoreAudio specific by making it conditional on TARGET_DARWIN, to avoid
confusion regarding the actual use case of that format type.

AE_FMT_LPCM is currently also added in device enumeration to
m_dataFormats on ALSA devices when the HDMI sink EDID contains a Short
Audio Descriptor with audio code 1 (LPCM). However, that makes no sense,
since the audio code simply means that the device accepts PCM input,
which all HDMI audio devices have to (obviously) support according to
the specification. Also, AESinkWASAPI adds AE_FMT_LPCM to m_dataFormats
for devices with 4+ channels, which is pointless as there is a separate
m_channelLayout member in AEAudioFormat for this purpose.
Both of these can be safely removed since m_dataFormats is used for
debug log purposes only (any actual users should use m_channelLayout
instead, which works with all sinks).

In the future CoreAudio could probably be changed to enter hog mode
based on the stream channel count directly, in order to avoid having a
special CoreAudio-specific format and related code in general
AudioEngine part.

@anssih anssih [AE] drop AE_FMT_LPCM from non-CoreAudio builds
AE has a separate AEDataFormat AE_FMT_LPCM for multichannel PCM audio.
However, the data content is actually the same as AE_FMT_FLOAT, and
AE_FMT_LPCM is only used by CoreAudioAE.

CoreAudio uses the AE_FMT_FLOAT <=> AE_FMT_LPCM differentation to enter
a special "hog" mode, which is reportedly needed for multichannel HDMI
playback on CoreAudio. AE_FMT_LPCM mode is entered on CoreAudio systems
instead of AE_FMT_FLOAT (or other regular modes) when the multichannel
LPCM playback option is enabled in audio settings and the output device
type is HDMI.

Since AE_FMT_LPCM has no meaning outside CoreAudio, clearly mark it as
CoreAudio specific by making it conditional on TARGET_DARWIN, to avoid
confusion regarding the actual use case of that format type.

AE_FMT_LPCM is currently also added in device enumeration to
m_dataFormats on ALSA devices when the HDMI sink EDID contains a Short
Audio Descriptor with audio code 1 (LPCM). However, that makes no sense,
since the audio code simply means that the device accepts PCM input,
which all HDMI audio devices have to (obviously) support according to
the specification. Also, AESinkWASAPI adds AE_FMT_LPCM to m_dataFormats
for devices with 4+ channels, which is pointless as there is a separate
m_channelLayout member in AEAudioFormat for this purpose.
Both of these can be safely removed since m_dataFormats is used for
debug log purposes only (any actual users should use m_channelLayout
instead, which works with all sinks).

In the future CoreAudio could probably be changed to enter hog mode
based on the stream channel count directly, in order to avoid having a
special CoreAudio-specific format and related code in general
AudioEngine part.
e720a28
@elupus
Team Kodi member
@davilla
Team Kodi member

From commit log:
AE_FMT_LPCM is currently also added in device enumeration to
m_dataFormats on ALSA devices when the HDMI sink EDID contains a Short
Audio Descriptor with audio code 1 (LPCM). However, that makes no sense,
since the audio code simply means that the device accepts PCM input,
which all HDMI audio devices have to (obviously) support according to
the specification.

As noted, this is used for debug.log purposes only.

@davilla

did you test under darwin and windows ?

@anssih
Team Kodi member

I knew I forgot to mention something :)

No, not tested on darwin and windows. Since there are no actual behavior changes (outside xbmc.log), I expect no issues (other than possible typos preventing build). Feel free to test to make sure of that, though :)

@DDDamian

Gnif and Gimli got quite animated about AE_FMT_LPCM. Gnif did not want it in - Gimli wanted it for the hog-mode issue.

I was the honest broker between them for my own selfish reasons: to hijack it for an all-integer mode for 24bit and 32bit integer passthrough (no float conversions). Unless we create two new formats (one for the all-integer mode mentioned and one for CA hog-mode or a fix for that) please do NOT take it out.

WASAPI enumerates it for a reason: on HDMI or USB we can send multichannel PCM, especially for TrueHD decoding and multichannel FLAC. That's just another reason it's in there!

@anssih
Team Kodi member

I don't think an all-integer mode should have its own AEDataFormat, there are already AE_FMT_S24x and AE_FMT_S32x for 24bit and 32bit formats. No float conversions is something that I'd like to see as well, but IMO that is unrelated to data format (just like hog mode) and therefore doesn't belong to AEDataFormat.
Even if AEDataFormat would be used for that despite the above, IMO it should use a separate format since obviously "all-integer" 24bit/32bit output is completely different from CoreAudio hog mode float output.

As for WASAPI, the code actually does nothing with AE_FMT_LPCM except put it in m_dataFormats, which is only outputted in xbmc.log by AEDeviceInfo.cpp. Multichannel support is indicated by specifying a channel map that has more than 2 channels.

@DDDamian

@anssih - quite correct in that it was only piggy-backing on the CA usage, and I know gnif would love to see that out of there.

The whole point of the enumeration code was not so much for the log (although it's been damn useful) but as a means to auto-set capabilites and setup, including a requested mode to upsample/upchannel eveything to the max supported. Without pointing fingers at who requested it's being bandied about as PikeMode(tm) ;)

The no-float-conversion thing is something I can shoe-horn in without this, but for the above it's good to know the device is multichannel PCM capable.

I guess the question is: we can use it in several useful ways - is it hurting anything?

@davilla

Seems like it's there for a reason on two platforms. Since it is not really hurting anything, I'd leave it alone until the real reasons for the presence is fixed, a) no float conversion and b) a better way to handle drawing hog mode. If if really offends, rename it to AE_FMT_OINK.

@gnif
Team Kodi member

+1 to renaming it, but not to OINK... perhaps AE_FMT_NATIVE or AE_FMT_BASIC, not sure what would quite fit here.

@anssih
Team Kodi member

@DDDamian - Yeah, I do understand that obviously m_dataFormats is supposed to plugged in later to other stuff than simply xbmc.log. However, I don't see the need for AE_FMT_LPCM to see if device is multichannel capable since the enumerated devices have a channel map (m_channels) and you should be able to see the multichannel support from that already, no? Or am I missing something? :)

If I am missing something above, and we need a separate flag for "multichannel support", the format name should be something like AE_FMT_HDMI_MULTICHANNEL_LPCM. But as said, IMO m_channels (or a separate flag if that is unsuitable for some reason that I don't see) should be used for this, since "multichannel support" is not a real format, just a device flag (when outputting data, the data is not actually in "AE_FMT_LPCM" format since it is not real).

As for the CoreAudio hog mode, maybe name it AE_FMT_FLOAT_CA_MULTI_HOG_MODE?

And if we want a audio format for no-float-conversion, that would be AE_FMT_DIRECT or AE_FMT_NATIVE or something. However, I don't see how a single audio format could be used there, since that same field is used to indicate the actual dataformat (AE_FMT_S16LE, AE_FMT_S32BE etc), and no-float-conversion AFAICS would need just an additional flag, not a format type (since you can do no-float-conversion output with any integer format, which there are many of).

The above 3 things are all very different use cases and AFAICS none of them should have their own AEDataFormat, instead the channel map could be used or a separate flag, depending on case.. And if we really really really have to use AEDataFormat for them, they should have different formats since they are different meanings (or we rename AE_FMT_LPCM to AE_FMT_MULTIPURPOSE_HACK).

Anyway, currently only CoreAudio makes "actual" use (even that is unnecessary AFAICS) of the field, and I don't see how the other proposed future use cases belong to AEDataFormat at all.

@gnif
Team Kodi member

@anssih - The addition of AE_FMT_LPCM in the enumerated data is obviously in error and should be removed.

I have never accepted the need for AE_FMT_LPCM and it was only left there to keep the CA devs happy, IMO it is a format that should be dropped or renamed as it is not really a format, just an obscure name that does not define anything at all.

I like the ifdefs for CA, and think that it should be renamed to AE_FMT_CUSTOM or AE_FMT_COREAUDIO... etc. AE_FMT_FLOAT == AE_FMT_LPCM, that is all there is to it.

@elupus
Team Kodi member
@huceke

@gnif: we had a long discussion why and how it used on OSX and agreed to keep it.

Give another unique raw format criteria for the COREAUDIO_IS_RAW macro. So we know the format is a raw.

https://github.com/xbmc/xbmc/blob/master/xbmc/cores/AudioEngine/Engines/CoreAudio/CoreAudioAE.h#L40

@gnif
Team Kodi member

@huceke - I agreed to keep it just to get AE out the door, I never agreed that it was acceptable. Also why is that macro duplicated? AE_IS_RAW does the same thing unless AAC is not considered RAW in CoreAudio?

@huceke

@gnif: welcome back and great that we have a new maintainer for CoreAudio.

@gnif
Team Kodi member

@huceke - Please don't turn this into what it is not, my questions are valid.

My suggestion is to rename it as it causes confusion just as I predicted it would many months ago (see Anssih's questions about it here and in IRC chatlog) and make it CoreAudio specific if at all possible. I do not understand the reason for needing this format, nor do I pretend to, I just do not agree with platform specific defines entering into the platform agnostic headers.

@huceke

@gnif: look, it is pure resignation. It was explained why it was done like that. this explanation is ignored. A strong dejavu in my head. So there is nothing left to say in that case.

@gnif
Team Kodi member

@huceke - I did not say it was not needed, nor do I care why it is needed, I have submitted to the fact that you know the platform and the CoreAudio code... all I have done is ask questions, and point out that if it is possible, it should be moved out of the agnostic headers as it only makes sense on OSX which is the exact same conclusion @Anssih came to when he reviewed the code, and thus this PR.

@davilla

AE_FMT_COREAUDIO_HOG, and never mind what it is or does, skip the ifdefs as it's already been shown by gimli's port that none of the other AE_FMT_xxx quite fit for it's usage and it is used as an AE format. Hog mode (or 'oink' as it is defined by its 4 char type) is a fact of life under OSX, it is the only way to configure the output for passthough of any kind, be it one of the traditional encoded flavors or multichannel pcm. To output multichannel pcm over hdmi, you MUST be in hog mode.

At some time, I do plan on attempting a CoreAudioSink to replace the CoreAudioEngine and bring darwin under SoftAE, then we can further discuss the need/usage of AE_FMT_COREAUDIO_HOG if needed.

@gnif
Team Kodi member

@davilla - Agreed on the rename, not sure I agree on making CA a sink.

I could be wrong, but I thought that CA was able to perform much of the software implemented features such as audio mixing, resampling, and gui sounds in it's API already, the CA engine should leverage the operating system's API and be a very simple wrapper around CA.

@davilla

It has already been discussed on irc the benefits of a CoreAudioSink, the largest is common code which reduces the amount of maintenance required, plus common behavior which exposes potential bugs/issues through cross-platform usage.

@elupus elupus commented on the diff
...res/dvdplayer/DVDCodecs/Audio/DVDAudioCodecFFmpeg.cpp
@@ -217,6 +221,7 @@ void CDVDAudioCodecFFmpeg::ConvertToFloat()
m_iBufferSize2 = len * m_dllAvUtil.av_get_bytes_per_sample(AV_SAMPLE_FMT_FLT);
}
}
+#endif
@elupus Team Kodi member
elupus added a note

To poor some more oil on the fire :)

@huceke Why is this conversion stuff needed? Ie, why should the codec convert to float? If CoreAE needs float for some odd reason, wouldn't it better to do that there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@DDDamian

Wow - turn my back for a few hours...... ;)

I cannot speak to the CA side (except to wish it was not a seperate engine if at all possible without ifdef'ing to death) but clearly Davilla, Memphiz and Gimli can so I leave that side to them.

I'm good with a seperate flag for squeaking thru integer audio as it must go thru the passthrough path regardless (we don't want to litter the code with tests for it when doing any processing like DSP's, volume control, etc). It would be much easier to add though by using an AEFormat by whatever name.

This was the opportunity I saw with AE_FMT_LPCM. For my purposes that just becomes a passthrough format of 32bits which go thru the chain untouched except for perhaps an endian swap where required.

I don't mind if it becomes AE_FMT_NATIVE or AE_FMT_WHATEVER. It doesn't prevent integer passthrough to remove it, but it does mean more test points littered thru the code.

@gnif
Team Kodi member

@DDDamian - I have been giving that feature some thought and I do not believe a AE_FMT_X would be the correct approach, the better way to handle it would be to start passing a struct which contains AEFormat and a bitfield for extra parameters, such as, RAW for bitstreaming, which would deprecate the AE_IS_RAW macro, and a flag for native/passthrough, whatever :)

@DDDamian

@gnif - sure, I'm good with that - encapsulation there makes good sense. I'll defer based on that and the fact that it's post-Frodo regardless given the release schedule.

I withdraw the objection :)

@anssih
Team Kodi member

Closing at this time. I will re-look at this later, and if still needed, propose a more gradual and less controversial PR.

@anssih anssih closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 30, 2012
  1. @anssih

    [AE] drop AE_FMT_LPCM from non-CoreAudio builds

    anssih committed
    AE has a separate AEDataFormat AE_FMT_LPCM for multichannel PCM audio.
    However, the data content is actually the same as AE_FMT_FLOAT, and
    AE_FMT_LPCM is only used by CoreAudioAE.
    
    CoreAudio uses the AE_FMT_FLOAT <=> AE_FMT_LPCM differentation to enter
    a special "hog" mode, which is reportedly needed for multichannel HDMI
    playback on CoreAudio. AE_FMT_LPCM mode is entered on CoreAudio systems
    instead of AE_FMT_FLOAT (or other regular modes) when the multichannel
    LPCM playback option is enabled in audio settings and the output device
    type is HDMI.
    
    Since AE_FMT_LPCM has no meaning outside CoreAudio, clearly mark it as
    CoreAudio specific by making it conditional on TARGET_DARWIN, to avoid
    confusion regarding the actual use case of that format type.
    
    AE_FMT_LPCM is currently also added in device enumeration to
    m_dataFormats on ALSA devices when the HDMI sink EDID contains a Short
    Audio Descriptor with audio code 1 (LPCM). However, that makes no sense,
    since the audio code simply means that the device accepts PCM input,
    which all HDMI audio devices have to (obviously) support according to
    the specification. Also, AESinkWASAPI adds AE_FMT_LPCM to m_dataFormats
    for devices with 4+ channels, which is pointless as there is a separate
    m_channelLayout member in AEAudioFormat for this purpose.
    Both of these can be safely removed since m_dataFormats is used for
    debug log purposes only (any actual users should use m_channelLayout
    instead, which works with all sinks).
    
    In the future CoreAudio could probably be changed to enter hog mode
    based on the stream channel count directly, in order to avoid having a
    special CoreAudio-specific format and related code in general
    AudioEngine part.
This page is out of date. Refresh to see the latest.
View
4 xbmc/cores/AudioEngine/AEAudioFormat.h
@@ -59,7 +59,9 @@ enum AEDataFormat
AE_FMT_EAC3,
AE_FMT_TRUEHD,
AE_FMT_DTSHD,
- AE_FMT_LPCM,
+#ifdef TARGET_DARWIN
+ AE_FMT_LPCM, /* used for the CoreAudio-specific multichannel hog mode; data is identical to AE_FMT_FLOAT */
+#endif
AE_FMT_MAX
};
View
17 xbmc/cores/AudioEngine/Sinks/AESinkWASAPI.cpp
@@ -757,8 +757,6 @@ void CAESinkWASAPI::EnumerateDevicesEx(AEDeviceInfoList &deviceInfoList)
wfxex.Format.nBlockAlign = wfxex.Format.nChannels * (wfxex.Format.wBitsPerSample >> 3);
wfxex.Format.nAvgBytesPerSec = wfxex.Format.nSamplesPerSec * wfxex.Format.nBlockAlign;
- bool hasLpcm = false;
-
// Try with KSAUDIO_SPEAKER_DIRECTOUT
for (unsigned int k = WASAPI_SPEAKER_COUNT; k > 0; k--)
{
@@ -769,11 +767,6 @@ void CAESinkWASAPI::EnumerateDevicesEx(AEDeviceInfoList &deviceInfoList)
hr = pClient->IsFormatSupported(AUDCLNT_SHAREMODE_EXCLUSIVE, &wfxex.Format, NULL);
if (SUCCEEDED(hr))
{
- if (k > 3) // Add only multichannel LPCM
- {
- deviceInfo.m_dataFormats.push_back(AE_FMT_LPCM);
- hasLpcm = true;
- }
break;
}
}
@@ -788,11 +781,6 @@ void CAESinkWASAPI::EnumerateDevicesEx(AEDeviceInfoList &deviceInfoList)
hr = pClient->IsFormatSupported(AUDCLNT_SHAREMODE_EXCLUSIVE, &wfxex.Format, NULL);
if (SUCCEEDED(hr))
{
- if ( !hasLpcm && k > 3) // Add only multichannel LPCM
- {
- deviceInfo.m_dataFormats.push_back(AE_FMT_LPCM);
- hasLpcm = true;
- }
break;
}
}
@@ -810,11 +798,6 @@ void CAESinkWASAPI::EnumerateDevicesEx(AEDeviceInfoList &deviceInfoList)
{
if ( deviceChannels.Count() < nmbOfCh)
deviceChannels = layoutsList[i];
- if ( !hasLpcm && nmbOfCh > 3) // Add only multichannel LPCM
- {
- deviceInfo.m_dataFormats.push_back(AE_FMT_LPCM);
- hasLpcm = true;
- }
}
}
pClient->Release();
View
1 xbmc/cores/AudioEngine/Utils/AEELDParser.cpp
@@ -194,7 +194,6 @@ void CAEELDParser::Parse(const uint8_t *data, size_t length, CAEDeviceInfo& info
case CEA_861_FORMAT_DTS : fmt = AE_FMT_DTS ; break;
case CEA_861_FORMAT_DTSHD: fmt = AE_FMT_DTSHD ; break;
case CEA_861_FORMAT_EAC3 : fmt = AE_FMT_EAC3 ; break;
- case CEA_861_FORMAT_LPCM : fmt = AE_FMT_LPCM ; break;
case CEA_861_FORMAT_MLP : fmt = AE_FMT_TRUEHD; break;
}
View
4 xbmc/cores/AudioEngine/Utils/AEUtil.cpp
@@ -106,7 +106,9 @@ const unsigned int CAEUtil::DataFormatToBits(const enum AEDataFormat dataFormat)
16, /* EAC3 */
16, /* TRUEHD */
16, /* DTS-HD */
+#ifdef TARGET_DARWIN
32 /* LPCM */
+#endif
};
return formats[dataFormat];
@@ -148,7 +150,9 @@ const char* CAEUtil::DataFormatToStr(const enum AEDataFormat dataFormat)
"AE_FMT_EAC3",
"AE_FMT_TRUEHD",
"AE_FMT_DTSHD",
+#ifdef TARGET_DARWIN
"AE_FMT_LPCM"
+#endif
};
return formats[dataFormat];
View
9 xbmc/cores/dvdplayer/DVDCodecs/Audio/DVDAudioCodecFFmpeg.cpp
@@ -173,13 +173,17 @@ int CDVDAudioCodecFFmpeg::Decode(BYTE* pData, int iSize)
m_iBuffered += iBytesUsed;
else
m_iBuffered = 0;
-
+
+#ifdef TARGET_DARWIN
if(m_bLpcmMode)
ConvertToFloat();
+#endif
return iBytesUsed;
}
+#ifdef TARGET_DARWIN
+// used by the CoreAudio-specific "LPCM hog mode"
void CDVDAudioCodecFFmpeg::ConvertToFloat()
{
if(m_pCodecContext->sample_fmt != AV_SAMPLE_FMT_FLT && m_iBufferSize1 > 0)
@@ -217,6 +221,7 @@ void CDVDAudioCodecFFmpeg::ConvertToFloat()
m_iBufferSize2 = len * m_dllAvUtil.av_get_bytes_per_sample(AV_SAMPLE_FMT_FLT);
}
}
+#endif
@elupus Team Kodi member
elupus added a note

To poor some more oil on the fire :)

@huceke Why is this conversion stuff needed? Ie, why should the codec convert to float? If CoreAE needs float for some odd reason, wouldn't it better to do that there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
int CDVDAudioCodecFFmpeg::GetData(BYTE** dst)
{
@@ -264,11 +269,13 @@ int CDVDAudioCodecFFmpeg::GetEncodedSampleRate()
enum AEDataFormat CDVDAudioCodecFFmpeg::GetDataFormat()
{
+#if TARGET_DARWIN
if(m_bLpcmMode)
{
return AE_FMT_LPCM;
}
else
+#endif
{
switch(m_pCodecContext->sample_fmt)
{
Something went wrong with that request. Please try again.