[osxsink] - properly set the ae device type based on either ca or heuris... #4336

Merged
merged 1 commit into from Mar 7, 2014

Conversation

Projects
None yet
5 participants
Owner

Memphiz commented Mar 5, 2014

...tical criteria - fixes DisplayPort not detected as LPCM capable and maybe others.

This hopefully fixes some false decisions the former code does. We treated displayports as optical (preventing the user from doing LPCM) and we might even HDMI devices as optical because we overwrite the device type during enumeration when we find some compatible 2.0 48000 digital stream.

Bug reported here:
http://forum.xbmc.org/showthread.php?tid=188165

@jmarshallnz @FernetMenta for comments ...

gotham material

Memphiz added the Gotham label Mar 5, 2014

Member

FernetMenta commented Mar 5, 2014

I think I can't help much here. To me it looks sane.

Owner

Memphiz commented Mar 5, 2014

user logs tell me that this is not exactly workingnlike it should :/

Owner

Memphiz commented Mar 5, 2014

@jmarshallnz - the isdigitaloutput method we are relying on here is not safe to use based on this statement from apple.

http://lists.apple.com/archives/coreaudio-api/2009/Aug/msg00113.html

I didn't run that code on my macmini yet - but as of this log from a tester

https://dl.dropboxusercontent.com/sh/sr1dz5m9m6hagkj/ZeLdvTFHEC/xbmc.log?dl=1&token_hash=AAGlqiGhB-5ZfinHztRnU_flJF7n1suWe0MhdNN0CWlbZw

This is exactly what fails - the "hasDigitalStreams" flag seems to stay false and so the displayport device keeps the initial pcm type instead of getting the dp type. Any other idea on how this log could happen or an idea how to prevent usage of the isdigitaloutput method?

Owner

Memphiz commented Mar 6, 2014

Updated - i found a way to query the transportType of the device instead of looking through the streams. On my macmini the streams never returned "isdigital" because my hdmi device had a terminaltype 1538 instead of 'hdmi' (see - its driver specific - we can't rely on that terminaltype).

The transportType of the device fits in my case (e.x. it returns 'hdmi').

I have also changed in using the iokit defines here as those are already there in 10.6 sdk (so i only need to define that thunderbolt thingy when compiling on older sdks...).

I let the user test again ...

Contributor

davilla commented Mar 6, 2014

we don't support less than 10.6 sdk :)

Owner

Memphiz commented Mar 6, 2014

Yeah with older sdks i mean 10.6 ... that thunderbolt define is not in 10.6 sdk and lower ;)

@jmarshallnz jmarshallnz and 1 other commented on an outdated diff Mar 6, 2014

xbmc/cores/AudioEngine/Sinks/AESinkDARWINOSX.cpp
@@ -199,6 +210,32 @@ static void EnumerateDevices(CADeviceList &list)
}
}
+ // decide the type of the device based on the discovered information
+ // in the streams
+ // device defaults to PCM (see start of the while loop)
+ // it can be HDMI, DisplayPort or Optical
+ // for all of those types it needs to support
+ // passthroughformats and needs to have at least one digital stream
+ if (hasPassthroughFormats && isDigital)
@jmarshallnz

jmarshallnz Mar 6, 2014

Member

This will break for the case where no passthrough formats are detected but isDigital is true. i.e. the case where the fake passthrough stuff is required?

@Memphiz

Memphiz Mar 6, 2014

Owner

this takes care of it 👍
https://github.com/xbmc/xbmc/pull/4336/files#diff-1fcb014e3b7898630c0bad96080d4d49R157

hasPassthroughFormats is set to true whenever we add AE_FMT_DTS or AE_FMT_AC3 to our supported format list (which is true in the fake passthrough stuff too...)

@jmarshallnz jmarshallnz commented on an outdated diff Mar 6, 2014

xbmc/cores/AudioEngine/Sinks/AESinkDARWINOSX.cpp
@@ -83,8 +83,23 @@ static void EnumerateDevices(CADeviceList &list)
device.m_displayName = device.m_deviceName;
device.m_displayNameExtra = "";
- if (device.m_deviceName.find("HDMI") != std::string::npos)
- device.m_deviceType = AE_DEVTYPE_HDMI;
+ // flag indicating that the device name "sounds" like HDMI
+ bool hasHdmiName = device.m_deviceName.find("HDMI") != std::string::npos;
+ // flag indicating that the device name "sounds" like DisplayPort
+ bool hasDisplayPortName = device.m_deviceName.find("DisplayPort") != std::string::npos;
@jmarshallnz

jmarshallnz Mar 6, 2014

Member

Cosmetically I'd move this down to where it's actually used (if other changes are necessary)

Owner

Memphiz commented Mar 6, 2014

mhhh to not introduce a regression here compared to the code before should i treat hasHdmiName as isDigital too? (before all devices with name "HDMI" got the AE_DEVTYPE_HDMI set. Now only those who are returned as isDigital or which have "digital" in their name ( and support 2channel 48000hz) are set to AE_DEVTYPE_HDMI.

So theoretically if there is a device out there which is called "HDMI" and doesn't have DTS/AC3 streams and also doesn't have "48000hz 2channel int" - would have been AE_DEVTYPE_HDMI with the old code and would now be AE_DEVTYPE_PCM.

Is this theory or relevant? ^^

Member

jmarshallnz commented Mar 6, 2014

Probably not a problem, but we mayaswell take that case as being HDMI.

Owner

Memphiz commented Mar 6, 2014

updated

  1. moved the flags to where they are used
  2. added overwrites for the devicetype for devices with the names containing "HDMI" and "DisplayPort" to match former code behaviour.
@Memphiz Memphiz [osxsink] - properly set the ae device type based on either ca or heu…
…ristical criteria - fixes DisplayPort not detected as LPCM capable and maybe others
dc87b59
Member

jmarshallnz commented Mar 6, 2014

jenkins build this please

jmarshallnz merged commit 5abc491 into xbmc:master Mar 7, 2014

1 check passed

default Merged build #322 succeeded in 54 min
Details

MartijnKaijser added this to the H* 14.0-alpha1 milestone Mar 7, 2014

jmarshallnz removed the Gotham label Mar 8, 2014

Memphiz deleted the Memphiz:osxdisplayport branch May 22, 2016

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