Skip to content

Fix WASAPI playback for 5.0 #1589

Closed
wants to merge 12 commits into from

8 participants

@Karlson2k
Team Kodi member

Not so elegant, but fully working.
WASAPI initialization was completely overhauled.

@Karlson2k
Team Kodi member

@DDDamian To see how "best match" algorithm works, just allow usages of "double" in as.xml. As most hardware don't accept float and XBMC try to send float, you will see sequence of initialization attempts.

@DDDamian

The opening statement concerns me ;)

This is a total re-write, without actually mentioning a feature or a bug being fixed. For some of it I like what you've done, some just adds complexity and in a few places out-right unnecessary code. Simple is good.

I'll go thru in detail and add line comments or general comments, but the first one is that neither advancedsetting seems to be actually required, nor does the 64bit audio at this point. I think it's good to add, but we don't need another setting just to determine whether we consider it. Ditto for the maxsample rate.

While I go thru this can you give me a brief synopsis on what bugs you are trying to fix or what feature(s) you add with this?

@DDDamian

And yes, I know it is meant to fix the 5.0 FLAC issue ;)

@gnif
Team Kodi member
gnif commented on 7a78119 Oct 10, 2012

Nice, this should be a separate PR as I would merge it in a second.

@DDDamian

Agreed with gnif's comment above - that's a great standalone in the meantime :)

I've pulled locally for a thorough review and some testing for the wasapi code.

@Karlson2k
Team Kodi member

As MSDN say, starting from WinXP (SP1 or SP2 - I don't remember) KSAUDIO_SPEAKER_5POINT1_SURROUND should be standard.
I checked - integrated realtek returns OK only for KSAUDIO_SPEAKER_5POINT1_SURROUND. Creative returns OK for both. In other cases I checked KSAUDIO_SPEAKER_5POINT1_SURROUND is a equals or better that KSAUDIO_SPEAKER_5POINT1.

@Karlson2k
Team Kodi member

I checked format specifications and documentations, so everything is checked for correct values.

Team Kodi member

Not so sure, that it should be in WASAPI and not somewhere else in AE. But if it works fine here, it can be later moved to other place.

@Karlson2k
Team Kodi member

@DDDamian Opening statement just mean that I wish to do more cleanup and change "goto" to more c-style if-else-while-return...

@Karlson2k

This it a final check, but it's not correct. If sound card support 32-bit int, there is not guarantee that sound card will support 24-bit int with same freq/channels.

@Karlson2k
Team Kodi member

Just move part of code to separate method.
Now it easier to modify it on move to somewhere else.

@Karlson2k
Team Kodi member

More info about f4deec9.
First of all I move try and initialization code to same place. Sometimes card can't initialize with same parameters that was OK for IsFormatSupported. For example Creative cards report support for 48000/24-bit/5.1,but fail to use it. Now it's possible to try something else in that case.
Now, if format isn't supported we try following:
1. Try same freq, channels, but with bit-wider formats. For 16 bit we will try 24 bit, 32 bit and (if not disabled) even float and double. This is totally lossless (or almost lossless) and most preferred conversion.
2. If requested format don't have LFE, but user speakers configuration support LFE, that try to add LFE. This is absolutely lossless as now upmixing is needed in this case.
2.1. Try format + LFE with bit-wider (16->24->32..)
3. If requested format have SL SR but not BL BR, try to use BL BR instead of SL SR (or vice versa - SL SR instead of BL BR). This is close configurations and (soon) should be not up/down mixed.
3.1. Try side-back with bit-wider
3.2. Try side-back with LFE and bit-wider
4. Try full user speakers setup instead of format specified. As requested format contains only channels that user have, no upmixing should be required.
4.1. Try use speakers with bit-wider.
5. Try multiple of requested sample frequency. 22050->44100->88200... This is a almost perfect conversion with less possible harmonic distortion. For each frequency try bit-wider formats.
5.1. Try format + LFE with multiple of frequency and bit-wider.
5.2. Try side-back with multiple of frequency and bit-wider. Than try side-back + LFE with multiple of frequency and bit-wider.
5.3. Try user speakers with multiple of frequency and bit-wider.
6. Try format with standard known sample rates, that higher requested. This is not ideal transformations, but using higher frequency is better possible solution. For each frequency (sample rate) try bit-wider.
6.1. (Need to be added, not implemented now). For each frequency try +LFE, side-back, user speakers.
7. Try formats with lower bit resolution (float->32, 24->16).
7.1. For each format try +LFE, side-back, user speakers, multiple of frequency, standard frequencies but do not try bit-wider (as all bit-wider formats was already tried).
8. Try format with standard known sample rates, that lower requested. For each sample rate try wider bits.
9. Try formats with standard known sample rates, that lower requested and with lower bit resolution (float->32, 24->16). For each format try +LFE, side-back, user speakers, multiple of frequency, standard frForl equencies but do not try bit-wider or higher frequency (as all bit-wider and higher frequency formats was already tried).

So we try all possible options from best-match to worst case.

@Karlson2k
Team Kodi member

Description of changes is here: xbmc#1589 (comment)

@DDDamian

Thx karlson2k - it's certainly complete in terms of the range of parameters it checks. Short of the reported 5.0 flac issue I had seen no issues with finding the appropriate format previously.

I've been reviewing in general terms, but will have to "force" a few formats to test the extremities.

One other general impression: we already have enumerated all the device formats prior to doing the device initialization. It reports every supported data format, sample rate and channel count. Shouldn't we just use that?

This initialization needs to happen quite quickly when we switch tracks in the player. We already know what the device is capable of. A lot of unnecessary system calls for IsFormatSupported will add to that delay on format switching. This is why we enumerate the device capabilities on start-up, whereas this will happen with each hit of the skip button......

@Karlson2k
Team Kodi member

@DDDamian The longest part of format checking is writing to log.
Even before I correct IsCompatible, Initialization was called twice on beginning of playback (once on start and immediately again as format was signaled as incompatible) and even with allowed float for PCM (so a lot of combinations was checked) there was not noticeable delay.

Unfortunately we can't check all possible combination of parameters. Separate check for frequencies, data formats and channels is not enough.
For example on Creative card stereo can be played back as 16-bit, 24(3), 24(4) and 32 on any frequency: 44.1, 48, 64, 88,2, 96. But 5.1 can be used only as 16/48 or 24/96 and fail on 24/44.1, 24/48 and on 16/44.1

I see the only possible way to avoid a lot of checks, this is to build a map of requested and resulted format. But we will need to reset this map with every user sound configuration change.

@DDDamian

The plan is for the engine to store the max pcm bitdepth and sample rate returned by sink/device to prevent calling Initialization twice if float is rejected (as well as for an all-integer path). This still needs implementing in the engine though.

With the exception of the 64khz frequency noted above all those scenarios would have already worked with the current code. The difference would be that the current code would downsample rather than upsample if an exact match was not found for the sample rate, as you iterate upwards.

Again, these are just observations as well as some insight into other changes. With the exception of some complexity I'm liking what I see so far.

@Karlson2k
Team Kodi member

@DDDamian Thanks for positive response!
Actually engine already stores all bitdepths and sample rates in CAEDeviceInfo.
I suggest to use CAEDeviceInfo& instead of std::string &device. It's easily to adapt current code for this.
And I'd like to extend CAEDeviceInfo a little.
Is it worth to try?

@Karlson2k
Team Kodi member

Already in progress.
Basic idea is in 4ce647de03368b629cb848d59200a1656aa915ba

@DDDamian

@Karlson2k - yep - that's what I was referring to in my previous comment (CAEDeviceInfo).

I'm perfectly fine with extending that class - we've barely touched on its original intent. Nice work!

@DDDamian

@Karlson2k - check your email

@bulkzooi

So this initiative died?

@Montellese
Team Kodi member

@bulkzooi: Could you please stop spamming everyone unless you have something constructive to say about the code or the discussion? That would be nice, thanks.

@bulkzooi

@Montellese: i thought pinging an 8 months old and untouched PR would be constructive. Especially given the 180 (and growing) open PR's. Rationale: otherwise github will end up as Trac once was.

@FernetMenta FernetMenta was assigned Aug 5, 2013
@da-anda
Team Kodi member
da-anda commented Sep 2, 2013

@fritsch @FernetMenta can this one be closed in favor of ActiveAE (where it's working I suppose, couldn't test)?

@FernetMenta
Team Kodi member

i had a brief look but not sure what this PR does fix or improve. e.g. 1c53efb seems wrong to me. due to pack func DTSHD requires 192khz and 8 channels. the mentioned commit does change those values.

@MartijnKaijser
Team Kodi member

i suggest we close this and if needed @Karlson2k will open a new PR based on ActiveAE

Edit:
best do this for all PRs that are based on SoftAE

@MartijnKaijser
Team Kodi member

If needed please create an updated PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.