CC'ing Руслан Ижбулатов who added support for the device provider here. Does this look fine to you?

Oh-kay. That was some years back, but i'll try to figure it out.

Now, the GUID testing at the beginning - i don't know what i was thinking, but my guess is that a check for KSCATEGORY_CAPTURE or KSCATEGORY_RENDER was intended to ensure that only I/O media devices get processed. The commit message mentions that this code *could* support audio, in theory, so i might have been thinking in that direction. However, when later the device is actually found, the code always calls new_video_source() to create a video device, so only video is supported at the moment.

I'm less sure about the ks_enumerate_devices(). Why have you changed that call? Did it not work in its current form? Enumerating the devices that match the GUID of the new device serves to narrow down the list (according to what i wrote back in bug 747757).

It is my understanding that DBT_DEVICEARRIVAL processing here is meant to find the new device that just arrived. It checks dbcc_name to ensure that it only gets that specific new device, and not the rest of the devices. 

According to what i wrote years before, we get multiple DBT_DEVICEARRIVAL, for each (?) supported class, so a capture device would always eventually give us a message that matches KSCATEGORY_CAPTURE, and then we'll enumerate all capture devices (kind of suboptimal, but i guess i was trying to achieve most with the least amount of code...), we find the newly-added device, and, well, add it to GST.

My guess that in your case it grabs the new audio device (since by that point it checks only by name, not by type, and the new audio device matches).

A more correct fix would be to only change the ks_enumerate_devices() call, changing the first argument to &KSCATEGORY_VIDEO. And add a comment mentioning that for audio support we will have to call it *again*, but with &KSCATEGORY_AUDIO, and then call a hypothetical new_audio_source() function if we find a match.
Actually, for the initial GUID check the &KSCATEGORY_RENDER should be removed, since, again, we only support capture-devices. And, again, a comment mentioning it (that for playback support we need to check KSCATEGORY_RENDER devices) would be nice.

That way we'll only look at capture devices, and of those we'll look only at the ones that are also *video* devices. Something along these lines:

> /* Only capture devices supported at the moment.
>  * Check also for KSCATEGORY_RENDER to support playback devices.
>  */
> if (!IsEqualGUID (&bcdi->dbcc_classguid, &KSCATEGORY_CAPTURE))
>   break;
>
> /* List all video devices that also support dbcc_classguid,
>  * i.e. KSCATEGORY_CAPTURE (see above). For audio recording devices
>  * repeat the same, but with &KSCATEGORY_AUDIO.
>  * For A/V playback devices repeat with KSCATEGORY_RENDER.
>  */
> devices =
>     ks_enumerate_devices (&KSCATEGORY_VIDEO,
>     &bcdi->dbcc_classguid);

Now that i'm thinking about this, it does kind of look like the patch you've submitted...

I changed the call to ks_enumerate_devices to be consistent with how it's called from gst_ks_device_provider_probe. I'll check if your suggestions work; honestly I'm not a KS API specialist and I went for what looked to make sense :)

Nope. I'm not sure why, but all device arrival messages with dbcc_classguid equal to KSCATEGORY_CAPTURE have the path of the audio capture device. Looks like video capture devices are only notified through KSCATEGORY_VIDEO.

Aaaaaaah according to https://docs.microsoft.com/en-us/windows-hardware/drivers/install/kscategory-capture the KSCATEGORY_CAPTURE is for devices that "captures wave or MIDI data streams".

Only sound then. What. The. Hell.

Okay, after some documentation reading and a few experiments with various devices, here are my conclusions as to what device interface category are dispatched in a DEVICEARRIVAL message:

1. For video capture devices, KSCATEGORY_VIDEO only
2. For audio capture devices, KSCATEGORY_AUDIO and KSCATEGORY_CAPTURE
3. For audio playback devices, KSCATEGORY_AUDIO and KSCATEGORY_RENDER

It looks like KSCATEGORY_CAPTURE and KSCATEGORY_RENDER have a different meaning when used as the AliasInterfaceClassGuid argument to SetupDiGetDeviceInterfaceAlias. In this context it seems to mean what the name actually suggests (or else the initial enumeration in ksdeviceprovider would not work). This is kind of explicit in the documentation for audio adapters device interfaces, but I can't find any mention for video devices... Knowing Microsoft, the fact that it works for video devices may well be a "special case" because too many programs assumed it would...

Based on this, I'll provide a slightly modified patch (with comments for future-proofing) on thursday.

Interesting. But if you can get a "wrong" path, does that mean that your USB camera actually present itself as two separate devices, with different paths?

What do you see in INFO logs when you plug this camera in? AFAICS, the code will log something like:
> New device, class interface GUID ..., path ...
for every DBT_DEVICEARRIVAL message.

Yes, my USB cameras that also support sound capture lead to at least three DEVICEARRIVAL messages: AUDIO, CAPTURE and VIDEO. The device paths are different for the audio and video parts.

Another thing - whenever i google this up, i always get facefull of driver inf files. Could you look up the driver file for your USB camera (devmgmt.msc -> doubleclick on the camera -> Details -> Inf name), find it in C:\Windows\INF and look for AddInterface lines? I think i'll be able to dig up a driver for a non-audio-capable camera later today, for comparison.

Yep, those device interfaces should be specified in INF files. I'm nearing the end of the day and won't be working tomorrow but I can try this on thursday. Don't worry about the non-audio/audio camera, I've got a cardboard full of both here :)

Comment on attachment 373328 [details] [review]
Fix against git master

Looks good to me.

I'll wait though for your confirmation before merging.

Thanks. The only sure thing right now is that the patch works, we're just fuzzy about why it does.

Well, I would add that from what I read it also make sense, you find an video capable device, and if it has VIDEO/CAPTURE we keep it. The other GUID used before seemed rather obscure to me. I'll wait till tomorrow, if we are lucky someone will have time to test it.

I'll make more tests on thursday, when I'm back to work.

Great, thanks.

Okay, most USB cameras use the Microsoft-provided UVC driver, which declares ALL of the interfaces. On the other hand, Logitech cameras use their own driver; in the corresponding INF files, video capture devices declare both KSCATEGORY_VIDEO and KSCATEGORY_CAPTURE, whereas audio capture devices declare KSCATEGORY_AUDIO and KSCATEGORY_CAPTURE. The right thing to do is thus to first filter on KSCATEGORY_VIDEO and then enumerate using this class uuid and KSCATEGORY_CAPTURE as a secondary one. New patch attached, with comments for future proofing.

Created attachment 373356 [details] [review]
Fix against git master - take two

That looks okay. Does it work for you?

Review of attachment 373356 [details] [review]:

Makes sense.

commit 203d1825c51481b6efe7ae2b275742cd789452c4 (HEAD -> master)
Author: Jerome Laheurte <jlaheurte@quividi.net>
Date:   Thu Aug 16 12:35:50 2018 +0200

    ksvideosrc: fix device enumeration when hotplugging a camera
    
    Since both audio and video capture devices declare the KSCATEGORY_CAPTURE interface,
    plugging a camera that supports both could result in an audio device being mistaken
    for a video one.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=796958

It's a good candidate for 1.14 backport.

Picked into 1.14 as well.