After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759484 - directsoundsrc: add device property as it is done in directsoundsink
directsoundsrc: add device property as it is done in directsoundsink
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Windows
: Normal enhancement
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-12-15 11:09 UTC by Thomas Roos
Modified: 2015-12-18 11:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add guid device property as it is done in directsoundsink (6.37 KB, patch)
2015-12-15 11:09 UTC, Thomas Roos
none Details | Review
add guid device property as it is done in directsoundsink (31.17 KB, patch)
2015-12-16 15:45 UTC, Thomas Roos
none Details | Review
add guid device property as it is done in directsoundsink (8.52 KB, patch)
2015-12-16 15:47 UTC, Thomas Roos
none Details | Review
add guid device property as it is done in directsoundsink (6.27 KB, patch)
2015-12-18 07:57 UTC, Thomas Roos
none Details | Review
add guid device property as it is done in directsoundsink (1.10 KB, patch)
2015-12-18 11:05 UTC, Thomas Roos
none Details | Review
add guid device property as it is done in directsoundsink (6.30 KB, patch)
2015-12-18 11:29 UTC, Thomas Roos
committed Details | Review

Description Thomas Roos 2015-12-15 11:09:12 UTC
Created attachment 317410 [details] [review]
add guid device property as it is done in directsoundsink

add guid device property as it is done in directsoundsink, since it's not supported yet. And it's also a kinda workaround for bug 748681
Comment 1 Sebastian Dröge (slomo) 2015-12-15 11:28:22 UTC
Review of attachment 317410 [details] [review]:

::: sys/directsound/gstdirectsoundsrc.c
@@ +87,3 @@
 {
   PROP_0,
   PROP_DEVICE_NAME,

Just remove the device-name property, it doesn't really make sense. There could also be multiple devices with the same name (but different GUID).

Having only one property also makes the code easier.
Comment 2 Thomas Roos 2015-12-15 15:08:38 UTC
yes and no - the use case for device-name is that a user can change/configure it in windows. And as well the mixer settings (volume, name) is based on device-name.
Comment 3 Sebastian Dröge (slomo) 2015-12-15 15:24:48 UTC
The mixer is not related to a device's GUID?
Comment 4 Thomas Roos 2015-12-15 16:22:46 UTC
As it is done its a string compare to the device-name to find a matching volume control. It's because it uses an old mixer api. Nowadays you usually use Endpoint Api which support selecting an mixer control via guid or device. 
Which means enabling mixer support by guid would mean an backward search for device-name by device-guid, ugly...
Comment 5 Sebastian Dröge (slomo) 2015-12-16 09:04:00 UTC
Ok, let's go with this then. Only problem is that it doesn't apply cleanly to git master :) Can you update?
Comment 6 Thomas Roos 2015-12-16 15:45:44 UTC
Created attachment 317506 [details] [review]
add guid device property as it is done in directsoundsink
Comment 7 Thomas Roos 2015-12-16 15:47:11 UTC
Created attachment 317507 [details] [review]
add guid device property as it is done in directsoundsink
Comment 8 Sebastian Dröge (slomo) 2015-12-16 16:26:13 UTC
Review of attachment 317507 [details] [review]:

::: sys/directsound/gstdirectsoundsrc.c
@@ +429,3 @@
+    /* Create capture object */
+    hRes =
+        pDSoundCaptureCreate (dsoundsrc->device_guid, &dsoundsrc->pDSC, NULL);

You have this line twice here now

@@ +437,3 @@
+  // mixer is only supported when device-id is not set
+  if (!dsoundsrc->device_id) {
+    gst_directsound_src_mixer_init (dsoundsrc);

Or maybe we should just remove the mixer support completely here?

::: sys/directsound/gstdirectsoundsrc.h
@@ +65,3 @@
 #define GST_IS_DIRECTSOUND_SRC(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj),GST_TYPE_DIRECTSOUND_SRC))
 #define GST_IS_DIRECTSOUND_SRC_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass),GST_TYPE_DIRECTSOUND_SRC))
+typedef struct _GstDirectSoundSrc GstDirectSoundSrc;

Don't run gst-indent on headers, only on .c files
Comment 9 Thomas Roos 2015-12-18 07:57:43 UTC
Created attachment 317609 [details] [review]
add guid device property as it is done in directsoundsink

- removed the duplicate init function call
- add msg that mixer will not work when device is selected by guid
- gst-indent only on c file ;)
Comment 10 Sebastian Dröge (slomo) 2015-12-18 10:28:03 UTC
Comment on attachment 317609 [details] [review]
add guid device property as it is done in directsoundsink

Looks good but does not apply to GIT master :) Can you update?
Comment 11 Sebastian Dröge (slomo) 2015-12-18 10:29:41 UTC
It might also make sense to just remove the mixer functionality here. The sink also does not have it and its usefulness seems limited.
Comment 12 Thomas Roos 2015-12-18 10:54:54 UTC
I think the mixer functionality will work if you have the right encoding set on your windows and set the device by device-name. So it think just keep it and inform the user that it will not work with device set by guid.
Comment 13 Thomas Roos 2015-12-18 11:05:14 UTC
Created attachment 317622 [details] [review]
add guid device property as it is done in directsoundsink

if it again not applies to master, please tell me what I'm doing wrong.
made an "git reset --hard origin/master" and merged my changes, did gst-indent on c file, git commit, git format-patch -1 
->?
Comment 14 Sebastian Dröge (slomo) 2015-12-18 11:08:30 UTC
This patch looks incomplete, it only has the header changes and a comment change in the .c file.

Not sure how that could've happened. The only thing you need to ensure is that the top commit in your branch has all the changes you want to commit, then "git format-patch -1" will give you a patch with all the changes. Or -2 if the top two commits, etc.
Comment 15 Thomas Roos 2015-12-18 11:29:08 UTC
Created attachment 317624 [details] [review]
add guid device property as it is done in directsoundsink

tried again to generate the patch, please check again
Comment 16 Sebastian Dröge (slomo) 2015-12-18 11:40:13 UTC
Problem was that you had the header already converted to UNIX line endings. All good now :)

commit f9464ce3549c2b3948a582464631ba4f8dd0d5e7
Author: Thomas Roos <thomas.roos@industronic.de>
Date:   Fri Dec 18 12:26:16 2015 +0100

    directsoundsrc: add device property as it is done in directsoundsink
    
    This allows selection of the device by GUID instead of the name. The name is
    user-given and multiple devices can have the same name.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=759484

commit d4920948aa4ccd7fc76a2266223eed7cfb013a71
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Fri Dec 18 12:36:26 2015 +0100

    directsoundsrc: Convert header from (some) DOS line endings to UNIX
    
    A mix between different line endings in the same file is not a good idea,
    and the .c files are both with UNIX line endings so let's use that.