Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ao_wasapi: Use ks.h public abi instead of ksuuid.h #9667

Closed
wants to merge 1 commit into from

Conversation

FlyGoat
Copy link

@FlyGoat FlyGoat commented Jan 3, 2022

ks.h is the pubic api provided by microsoft while ksuuid.h
is undocumented.

To use ks.h api, we need to link against ksuser library. So
add ksuer detection to waf and meson.

Fixes: #9627

ks.h is the pubic api provided by microsoft while ksuuid.h
is undocumented.

To use ks.h api, we need to link against ksuser library. So
add ksuer detection to waf and meson.

Fixes: mpv-player#9627

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
@Dudemanguy
Copy link
Member

Dudemanguy commented Jan 3, 2022

Would it be sufficient to simply change the the wasapi check to only look for the ksuser library? You could remove the wasapi.c fragment altogether in that case. I don't really know anything about windows but if ksuser being available guarantees that you have AudioClient, using the library check alone would be better.

@FlyGoat
Copy link
Author

FlyGoat commented Jan 3, 2022

IAudioClient is not guaranteed if users are compile with earlier mingw (i.e. shipped with CentOS7).

@Dudemanguy
Copy link
Member

Cool, seems fine in that case. I'll let someone that actually knows about windows chime in.

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

ping?@Dudemanguy it is blocking windows build.

@avih
Copy link
Member

avih commented Jan 14, 2022

it is blocking windows build.

Blocking to whom? AFAIK it only blocks with mingw master, no?

Why is the ksuer detection? is it expected to not exists in some setups? If it can be missing (e.g. maybe older mingw?) and the commit disabled wasapi for such build setups, then I don't like it. Some distros can take a long time to update mingw, and without this commit it works fine with current/older mingw releases...

Also, which functions/symbols/whatever exist at the lib which are now needed and previously we didn't need the lib for them?

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

Why is the ksuer detection?

ksuser is always there since windows95, but it is a common practice to detect the existence of a library before using it.

Also, which functions/symbols/whatever exist at the lib which are now needed and previously we didn't need the lib for them?

KSDATAFORMAT_SUBTYPE_PCM family of symbol was provided by ksuuid.h as a macro, but actually it should't come from there, it should be a dynamic symbol imported from DLL.

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

The building issue actuall helped us discovered that long lasting problem. As ksuuid.h is not a part of public API, it is not guaranteed that KSDATAFORMAT_SUBTYPE_PCM won't be changed in future Windows releases. Linking with the library which actually provide the symbol is the proper way for windows development.

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

it is blocking windows build.

Blocking to whom? AFAIK it only blocks with mingw master, no?

It is blocking latest MSYS2 users.

@avih
Copy link
Member

avih commented Jan 14, 2022

ksuser is always there since windows95

Don't check a lib if it exists since win95 and also in all current+historic mingw versions (does it?).

The building issue actuall helped us discover ...

Who is "us"?

It is blocking latest MSYS2 users.

That's not a factor. MSYS2 uses master of everything, and we can't guarantee to be compatible with bleeding edge packages, unless noted otherwise. Our aim is to be compatible with release versions, preferably also older release version which are still out there, and hopefully bleeding edge too, but that's a bonus.

Either way, what happens with older mingw versions?

@ghost
Copy link

ghost commented Jan 14, 2022

I don't seem to have a new enough meson version on the latest msys2, but waf builds mpv fine for me with mingw-w64-x86_64-headers-git 9.0.0.6373.5be8fcd83-1.

@ghost
Copy link

ghost commented Jan 14, 2022

Found it, needed to install mingw-w64-meson, not one of the other versions. Also builds fine for me with meson. (Not to derail this PR, just chiming in since I happen to use msys2.)

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

ksuser is always there since windows95

Don't check a lib if it exists since win95 and also in all current+historic mingw versions (does it?).

Yep, it's a foundation part of Windows sound API.
As you can see in RPM find it exists in allmost all mingw versions
https://rpmfind.net/linux/rpm2html/search.php?query=mingw32(ksuser.dll)

The building issue actuall helped us discover ...

Who is "us"?

Well, I should say me :-)

It is blocking latest MSYS2 users.

That's not a factor. MSYS2 uses master of everything, and we can't guarantee to be compatible with bleeding edge packages, unless noted otherwise. Our aim is to be compatible with release versions, preferably also older release version which are still out there, and hopefully bleeding edge too, but that's a bonus.

Either way, what happens with older mingw versions?

Nothing happens, ksuser detection is expected to pass on all mingw versions.

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

Found it, needed to install mingw-w64-meson, not one of the other versions. Also builds fine for me with meson. (Not to derail this PR, just chiming in since I happen to use msys2.)

Hmm I just found that mingw had pushed a fix in master to workaorund this build issue. However it reveals a actual problem that we are relying on a behavior that is not guaranteed to be last forever. It is a correction of API misuse :-)

@avih
Copy link
Member

avih commented Jan 14, 2022

Nothing happens, ksuser detection is expected to pass

I meant will it build and work as expected? And as I said, remove the detection. It's not needed - at least not the detection of the lib.

The question is how are the symbols resolved to the runtime lib (currently), and does older mingw also able to resolve them from the lib at runtime? And if not - what happens then?

Hmm I just found that mingw had pushed a fix in master to workaorund this build issu

mingw has been changing since #9627 was opened (well, before, which resulted in that issue), and it could change a bit more, which is why we don't care too much about bleeding edge, especially in a state of flux. We should wait till it settles first IMO.

@FlyGoat
Copy link
Author

FlyGoat commented Jan 14, 2022

Nothing happens, ksuser detection is expected to pass

I meant will it build and work as expected? And as I said, remove the detection. It's not needed - at least not the detection of the lib.

The general rule of a build system is always test library availability before linking it. By doing this we can avoid tons of user environmental issues. Although it should be safe to assume ksuser.dll is exist when we can find relevant headers, but it is still a assumption.

Yes, for older mingw it still work as this family of symbol is declared in ks.h as external symbol.

mingw has been changing since #9627 was opened (well, before, which resulted in that issue), and it could change a bit more, which is why we don't care too much about bleeding edge, especially in a state of flux. We should wait till it settles first IMO.

mingw is always trying to align with official MS API. So the safest way for us (well, at least me) is just following Microsoft's recommended usage, not depending on some mingw trick.

@avih
Copy link
Member

avih commented Jan 17, 2022

@jeeb made a thorough analysis here #9627 (comment)
It does not seem to be dynamically loaded from any dll.

He'll comment here later.

@Dudemanguy
Copy link
Member

Similar to what I said in #9627, but it looks like mingw-w64 hasn't changed the behavior since then (i.e. it still works) so we don't have to change any code on our end. Plus this is old.

@Dudemanguy Dudemanguy closed this Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows: libksuser.a needs linking, audio/out/ao_wasapi_utils.c headers need changing?
3 participants