bug 2994 fix #17

Closed
tch wants to merge 1 commits from bug/2994/kmix-channels into master
tch commented 4 years ago
Collaborator

On branch bug/2994/kmix-channels
Changes to be committed:
modified: kmix/mixer_alsa9.cpp

On branch bug/2994/kmix-channels Changes to be committed: modified: kmix/mixer_alsa9.cpp
tch changed title from Committer: TCH <tch@protonmail.com> to bug 2994 fix 4 years ago
Owner

Congratulations, you have your first pull-request!

There are some formalities that need to be fixed:

  1. The comment in the git log should contain brief information about the change. Existing content is not good.
  2. When fixing a bug from a Bugzilla, it's a good idea to mention a phrase like: "This resolves bug XXXX."

    When fixing an issue from TGW, it's a good idea to mention a phrase like: "This resolves issue #XX."

    This will allow the generation of links to related bug reports and issues on change log.
  3. It is necessary for the git log to agree with the DCO using a Signed-off-by signature.

You can use the following procedure to fix the git log:

  1. git commit --amend -s

    This will open an editor where you will be able to replace the existing git log. At the same time, using the -s option ensures that a Signed-off-by line is automatically appended.
  2. git push origin HEAD -f

    This will push the new version of the commit over the existing one.
Congratulations, you have your first pull-request! There are some formalities that need to be fixed: 1. The comment in the git log should contain brief information about the change. Existing content is not good. 2. When fixing a bug from a Bugzilla, it's a good idea to mention a phrase like: "This resolves bug XXXX." <br/>When fixing an issue from TGW, it's a good idea to mention a phrase like: "This resolves issue #XX." <br/>This will allow the generation of links to related bug reports and issues on change log. 3. It is necessary for the git log to agree with the DCO using a Signed-off-by signature. You can use the following procedure to fix the git log: 1. `git commit --amend -s` <br/>This will open an editor where you will be able to replace the existing git log. At the same time, using the `-s` option ensures that a Signed-off-by line is automatically appended. 2. `git push origin HEAD -f` <br/>This will push the new version of the commit over the existing one.
tch commented 4 years ago
Poster
Collaborator

I cloned the project onto a RAM disk, so i no longer have the git settings which was applied to that repo. The comment contains the bug ticket's number (2994) though.

I cloned the project onto a RAM disk, so i no longer have the git settings which was applied to that repo. The comment contains the bug ticket's number (2994) though.
Owner

Hi @tch, I looked at your PR in details.

First of all, thanks for the time and the effort.

To be honest, the PR is a bit overly complex but the main point is that there is a problem with it: it can create two entries for the same device in case both play and capture capabilities are detected.

  1. "if (canPlay || isPlaySwitch || isEnum)" can create a device with play abilities
  2. "if (canCapture || isCaptureSwitch)" can create a device with capturing abilities

And since those two ifs are not excluding each other you could end up with two different MixDevice for the same actual device, which sounds wrong to me.

The current code in master branch (without this PR) gives priority to capturing over playing abilities, hence if a device can capture, it will end up in the input section rather than the output section.

One possible solution would be to invert this, but I am not sure it is the right way and further discussion together with Slavek is required.

I read once again the description of bug 2994. If I understand correctly, your problem is that you cannot change the volume from the KMix tray bar, right? If that is the case, there is a simple solution. Right click the tray icon, select "Select Master Channel..." and you are presented with a dialog which lists all output and input devices and where you can choose the channel associated to the tray control. If you choose the master on the input section, that should just work fine without the need for any software change.

Could you try and confirm that?

Hi @tch, I looked at your PR in details.<br/> First of all, thanks for the time and the effort.<br/> To be honest, the PR is a bit overly complex but the main point is that there is a problem with it: it can create two entries for the same device in case both play and capture capabilities are detected. 1. "if (canPlay || isPlaySwitch || isEnum)" can create a device with play abilities 2. "if (canCapture || isCaptureSwitch)" can create a device with capturing abilities And since those two ifs are not excluding each other you could end up with two different MixDevice for the same actual device, which sounds wrong to me. The current code in master branch (without this PR) gives priority to capturing over playing abilities, hence if a device can capture, it will end up in the input section rather than the output section.<br/> One possible solution would be to invert this, but I am not sure it is the right way and further discussion together with Slavek is required. I read once again the description of bug 2994. If I understand correctly, your problem is that you cannot change the volume from the KMix tray bar, right? If that is the case, there is a simple solution. Right click the tray icon, select "Select Master Channel..." and you are presented with a dialog which lists all output and input devices and where you can choose the channel associated to the tray control. If you choose the master on the input section, that should just work fine without the need for any software change.<br/> Could you try and confirm that?
tch commented 4 years ago
Poster
Collaborator

it can create two entries for the same device in case both play and capture capabilities are detected

Which was exactly the point. There are devices which can capture and play too, so they must appear on both panel.

And since those two ifs are not excluding each other you could end up with two different MixDevice for the same actual device, which sounds wrong to me.

But it's not. Please check my ALSAMixer layout as a reference: it's full of redundant entries, for a lot of devices on my sound card are bidirectional.

http://oscomp.hu/depot/alsamixer_all_part1.png

http://oscomp.hu/depot/alsamixer_all_part2.png

As you can see: devices "Master", "Bass", "Treble", "Synth", "Aux2", "Analog Mix", "Audigy CD" appear twice in "Full" view, because they appear on both "Playback" and "Capture". Hence the nature of my solution.

One possible solution would be to invert this, but I am not sure it is the right way and further discussion together with Slavek is required.

That is not a solution, you'll end up with bug's inverse version: hybrid devices will only appear on the "Output" panel instead of the "Input".

I read once again the description of bug 2994. If I understand correctly, your problem is that you cannot change the volume from the KMix tray bar, right?

No. My problem was that i cannot change the outgoing "Master" channel's volume, because only the incoming "Master" was available. And the problem existed because KMix did not put the Master device on the "Output" panel.

Right click the tray icon, select “Select Master Channel...” and you are presented with a dialog which lists all output and input devices and where you can choose the channel associated to the tray control. If you choose the master on the input section, that should just work fine without the need for any software change.

Wrong. The "Master" did not appeared on the "Output" panel at all, so i could not select it. I even illustrated the problem with screenshots:

http://oscomp.hu/depot/kmix_both.png

See? There is no "Master" channel on the "Output" panel.

> it can create two entries for the same device in case both play and capture capabilities are detected Which was exactly the point. There are devices which can capture and play too, so they must appear on both panel. > And since those two ifs are not excluding each other you could end up with two different MixDevice for the same actual device, which sounds wrong to me. But it's not. Please check my ALSAMixer layout as a reference: it's full of redundant entries, for a lot of devices on my sound card are bidirectional. http://oscomp.hu/depot/alsamixer_all_part1.png http://oscomp.hu/depot/alsamixer_all_part2.png As you can see: devices "Master", "Bass", "Treble", "Synth", "Aux2", "Analog Mix", "Audigy CD" appear twice in "Full" view, because they appear on both "Playback" and "Capture". Hence the nature of my solution. > One possible solution would be to invert this, but I am not sure it is the right way and further discussion together with Slavek is required. That is not a solution, you'll end up with bug's inverse version: hybrid devices will only appear on the "Output" panel instead of the "Input". > I read once again the description of bug 2994. If I understand correctly, your problem is that you cannot change the volume from the KMix tray bar, right? No. My problem was that i cannot change the outgoing "Master" channel's volume, because only the incoming "Master" was available. And the problem existed because KMix did not put the Master device on the "Output" panel. > Right click the tray icon, select “Select Master Channel...” and you are presented with a dialog which lists all output and input devices and where you can choose the channel associated to the tray control. If you choose the master on the input section, that should just work fine without the need for any software change. Wrong. The "Master" did not appeared on the "Output" panel at all, so i could not select it. I even illustrated the problem with screenshots: http://oscomp.hu/depot/kmix_both.png See? There is no "Master" channel on the "Output" panel.
Owner

In short: If a comment in the git log mentioned that it fix the detection of mixer items for devices that are bidirectional, suddenly the patch would be more obvious. But the current commentary completely lacks meaning. So it is obvious that a good description in the git log is a very important thing.

In short: If a comment in the git log mentioned that it fix the detection of mixer items for devices that are bidirectional, suddenly the patch would be more obvious. But the current commentary completely lacks meaning. So it is obvious that a good description in the git log is a very important thing.
tch commented 4 years ago
Poster
Collaborator

Perhaps. But the problem was explained in details (and with attached images) in the bugticket of 2994 which is referred in the commit log and the solution was explained in the neighboring gitea ticket, so it's beside this pull request.

Perhaps. But the problem was explained in details (and with attached images) in the bugticket of 2994 which is referred in the commit log and the solution was explained in the neighboring gitea ticket, so it's beside this pull request.
Owner

ok, thanks for the detailed explanation @tch. It wasn't very cleared before and I had to guess a bit. So you can basically set different values for input and output device on the same board, interesting. In that case it would make sense to have separated devices I guess.

I am going to make some extra comments in the code in a while, after I take a second view, there some things that could be simplified IMO.

ok, thanks for the detailed explanation @tch. It wasn't very cleared before and I had to guess a bit. So you can basically set different values for input and output device on the same board, interesting. In that case it would make sense to have separated devices I guess. I am going to make some extra comments in the code in a while, after I take a second view, there some things that could be simplified IMO.
Owner

@tch

for my understanding, can you explain why we consider an unknown device (MixDevice::UNDEFINED) as a playing switch? Not saying it is right or wrong, just want to understand the logic behind it.

@tch<br/> for my understanding, can you explain why we consider an unknown device (MixDevice::UNDEFINED) as a playing switch? Not saying it is right or wrong, just want to understand the logic behind it.
Owner

I have done some testing on my system (debian buster running inside a VM in a Windows host). Here are the info.

  1. the 3 pictures with black background show the alsamixer screen for out, in and all devices. You can see some devices like "line" and "CD" are duplicated. In the out tab we can set a volume bar and in the in tab we can select the device for capturing (but no volume can be set there)

  2. the file kmix-master.png (3rd picture) shows the out and in controls as shown in kmix with the code in master branch. You can see devices like "line" and "CD" shows up in the in tab, where it is possible to enable/disable the device (top orange rectangle shows the playback capabilities) and select it for capturing (botton yellow rectangle). The volume bar correspond to the alsamixer bar. Overall the devices and controls match alsa mixer.

  3. the file kmix-PR17.png (last picture) shows the out and in controls as shown in kmix with the code in the PR #17 branch. You can see devices like "line" and "CD" are duplicated in both out and in tabs. The playback controls are duplicated (yellow and red squared), the capture selection is still fine (yellow square), the volume setting is duplicated (pink round square). Moreover the volume in the out tab matches the alsamixer volume while the volume on the in tab is not scaled correctly. Moving the volume bar in the out tab affects the volume bar in the in tab and viceversa.

As it stands, the code in PR #17 definitely introduces issues and needs to be reviewed.

I don't know what is the best way forward and we need to discuss together. What I can say is that this is a sensitive topic that would affects all users, so we need to be very careful in what we do. For what I see so far, the code in master seems to work fine in most cases. This does not mean we want to ignore @tch case, but instead that we need to find a solution that works for him without affecting all other user cases. Perhaps there are some other device properties that can be looked at to decide when to create separate channels for in and out??

I have done some testing on my system (debian buster running inside a VM in a Windows host). Here are the info. 1) the 3 pictures with black background show the alsamixer screen for out, in and all devices. You can see some devices like "line" and "CD" are duplicated. In the out tab we can set a volume bar and in the in tab we can select the device for capturing (but no volume can be set there) 2) the file kmix-master.png (3rd picture) shows the out and in controls as shown in kmix with the code in master branch. You can see devices like "line" and "CD" shows up in the in tab, where it is possible to enable/disable the device (top orange rectangle shows the playback capabilities) and select it for capturing (botton yellow rectangle). The volume bar correspond to the alsamixer bar. Overall the devices and controls match alsa mixer. 3) the file kmix-PR17.png (last picture) shows the out and in controls as shown in kmix with the code in the PR #17 branch. You can see devices like "line" and "CD" are duplicated in both out and in tabs. The playback controls are duplicated (yellow and red squared), the capture selection is still fine (yellow square), the volume setting is duplicated (pink round square). Moreover the volume in the out tab matches the alsamixer volume while the volume on the in tab is not scaled correctly. Moving the volume bar in the out tab affects the volume bar in the in tab and viceversa. As it stands, the code in PR #17 definitely introduces issues and needs to be reviewed.<br/> I don't know what is the best way forward and we need to discuss together. What I can say is that this is a sensitive topic that would affects all users, so we need to be very careful in what we do. For what I see so far, the code in master seems to work fine in most cases. This does not mean we want to ignore @tch case, but instead that we need to find a solution that works for him without affecting all other user cases. Perhaps there are some other device properties that can be looked at to decide when to create separate channels for in and out??
tch commented 4 years ago
Poster
Collaborator

for my understanding, can you explain why we consider an unknown device (MixDevice::UNDEFINED) as a playing switch? Not saying it is right or wrong, just want to understand the logic behind it.

KMix always handled unknown typed devices as switches. This was the case before my patch. And if it's a switch, you have to enlist it either a play or capture switch. If i did not do that, then one of my switches disappeared.

> for my understanding, can you explain why we consider an unknown device (MixDevice::UNDEFINED) as a playing switch? Not saying it is right or wrong, just want to understand the logic behind it. KMix always handled unknown typed devices as switches. This was the case before my patch. And if it's a switch, you have to enlist it either a play or capture switch. If i did not do that, then one of my switches disappeared.
tch commented 4 years ago
Poster
Collaborator

As it stands, the code in PR #17 definitely introduces issues and needs to be reviewed.

No, it does not. Those "duplicated" entries are not errors. You can set the CD playback volume and the CD capture volume. You can play CD-s and you can capture their sound. If you look it again, you can see, that the two has two distinct volumes. Same stands for Line: the one on Output is Line Out and the other one on Input is Line In.

To me, it seems, that ALSAMixer wrongly handles your CD capture volume. Do you have a CD player? You should check it out if you try to play and capture from it and set the volume from both ALSAMixer and KMix.

> As it stands, the code in PR #17 definitely introduces issues and needs to be reviewed. No, it does not. Those "duplicated" entries are not errors. You can set the CD playback volume and the CD capture volume. You can play CD-s and you can capture their sound. If you look it again, you can see, that the two has two distinct volumes. Same stands for Line: the one on Output is Line Out and the other one on Input is Line In. To me, it seems, that ALSAMixer wrongly handles your CD capture volume. Do you have a CD player? You should check it out if you try to play and capture from it and set the volume from both ALSAMixer and KMix.
Owner

for my understanding, can you explain why we consider an unknown device (MixDevice::UNDEFINED) as a playing switch? Not saying it is right or wrong, just want to understand the logic behind it.

KMix always handled unknown typed devices as switches. This was the case before my patch. And if it's a switch, you have to enlist it either a play or capture switch. If i did not do that, then one of my switches disappeared.

ok, thanks for the explanation!!

> > for my understanding, can you explain why we consider an unknown device (MixDevice::UNDEFINED) as a playing switch? Not saying it is right or wrong, just want to understand the logic behind it. > > KMix always handled unknown typed devices as switches. This was the case before my patch. And if it's a switch, you have to enlist it either a play or capture switch. If i did not do that, then one of my switches disappeared. ok, thanks for the explanation!!
Owner

As it stands, the code in PR #17 definitely introduces issues and needs to be reviewed.

No, it does not. Those "duplicated" entries are not errors. You can set the CD playback volume and the CD capture volume. You can play CD-s and you can capture their sound. If you look it again, you can see, that the two has two distinct volumes. Same stands for Line: the one on Output is Line Out and the other one on Input is Line In.

To me, it seems, that ALSAMixer wrongly handles your CD capture volume. Do you have a CD player? You should check it out if you try to play and capture from it and set the volume from both ALSAMixer and KMix.

I can test tomorrow on another computer which runs linux directly (no VM) and has a CD player. Then I will report back.

Ultimately alsa is controlling the card, so we have to take that alsamixer is more accurate. When running on a VM without a CD causes weird issues I don't know, but we can't take that Kmix is more precise. For example as highlighted, the volume bar for CD in the output tab is linked to the one in the input tab: modifying the value of one changes the value of the other. So it is not the case that the two are independent bars as you say, in which case I would have agreed with you.

> > As it stands, the code in PR #17 definitely introduces issues and needs to be reviewed. > > No, it does not. Those "duplicated" entries are not errors. You can set the CD playback volume and the CD capture volume. You can play CD-s and you can capture their sound. If you look it again, you can see, that the two has two distinct volumes. Same stands for Line: the one on Output is Line Out and the other one on Input is Line In. > > To me, it seems, that ALSAMixer wrongly handles your CD capture volume. Do you have a CD player? You should check it out if you try to play and capture from it and set the volume from both ALSAMixer and KMix. > I can test tomorrow on another computer which runs linux directly (no VM) and has a CD player. Then I will report back. Ultimately alsa is controlling the card, so we have to take that alsamixer is more accurate. When running on a VM without a CD causes weird issues I don't know, but we can't take that Kmix is more precise. For example as highlighted, the volume bar for CD in the output tab is linked to the one in the input tab: modifying the value of one changes the value of the other. So it is not the case that the two are independent bars as you say, in which case I would have agreed with you.
Owner

Anyhow I have some ideas for a possible solution. Tomorrow I will implement the code, then ask you to test on your system as well, if you don't mind @tch

Anyhow I have some ideas for a possible solution. Tomorrow I will implement the code, then ask you to test on your system as well, if you don't mind @tch
tch commented 4 years ago
Poster
Collaborator

Ultimately alsa is controlling the card, so we have to take that alsamixer is more accurate. When running on a VM without a CD causes weird issues I don't know, but we can't take that Kmix is more precise.

We don't have to take anything, we have to test and judge on an empirical basis. I'm pretty sure, that the duplication is not an error, i have two "CD" bars in ALSAMixer too.

As for the different volume levels, that indeed can be an error, but not only in KMix, but in ALSAMixer too. I'm not entirely sure, but i think this will be the case, since your ALSAMixer images contradicts each others, two times:

  • You have a "Capture" labelled "CD" bar on the "Capture" panel, without the volume.
  • You have a "CD" bar on the "Playback" panel too, with the volume.
  • You have one "Capture" labelled "CD" bar on the "All" panel, but with the volume.

How is this possible? If you have a "CD" on both the "Playback" and "Capture" panels, then you should have two on the "All" panel, yet you have but one, and that one says that is the "Capture" one, but it has the volume, while the one on the "Capture" panel did not have the volume. Currently it seems to me, that KMix has shown your volumes correctly, while ALSAMixer confused the CD on the Full panel: it shows the "Capture" label, but it shows the "Playback" volume. That would explain your "cross-control" issue.

This calls for a test, not assumptions, that ALSAMixer must be correct. But unfortunately a VM won't cut it, as to test the real CD capturing, you'll need to plug your CD drive to your sound card and then you'll need to start a real CD player which do not grab the tracks by software, but instructs the drive itself to play them and you can check if you can lower/raise the volume from KMix. If yes, then KMix handles the playback of CD volume well. Then you start a recorder, set the source to the CD capture and then play the CD track again with the player and if you can lower/raise the volume of the captured stream from KMix, while the audible sound is not changed, then KMix does exactly it's job and ALSAMixer is erroneous. If you encounter any problems, then KMix handles the volumes badly, but that does not mean, that ALSAMixer does it correctly, so that should be tested too.

Edit: I don't mind, if it's necessary. But please, test it first. ALSAMixer is not error-proof.

> Ultimately alsa is controlling the card, so we have to take that alsamixer is more accurate. When running on a VM without a CD causes weird issues I don't know, but we can't take that Kmix is more precise. We don't have to take anything, we have to test and judge on an empirical basis. I'm pretty sure, that the duplication is not an error, i have two "CD" bars in ALSAMixer too. As for the different volume levels, that indeed can be an error, but not only in KMix, but in ALSAMixer too. I'm not entirely sure, but i think this will be the case, since your ALSAMixer images contradicts each others, two times: * You have a "Capture" labelled "CD" bar on the "Capture" panel, without the volume. * You have a "CD" bar on the "Playback" panel too, with the volume. * You have *one* "Capture" labelled "CD" bar on the "All" panel, but *with the volume*. How is this possible? If you have a "CD" on both the "Playback" and "Capture" panels, then you should have two on the "All" panel, yet you have but one, and that one says that is the "Capture" one, but it has the volume, while the one on the "Capture" panel did not have the volume. Currently it seems to me, that KMix has shown your volumes correctly, while ALSAMixer confused the CD on the Full panel: it shows the "Capture" label, but it shows the "Playback" volume. That would explain your "cross-control" issue. This calls for a test, not assumptions, that ALSAMixer must be correct. But unfortunately a VM won't cut it, as to test the real CD capturing, you'll need to plug your CD drive to your sound card and then you'll need to start a real CD player which do not grab the tracks by software, but instructs the drive itself to play them and you can check if you can lower/raise the volume from KMix. If yes, then KMix handles the playback of CD volume well. Then you start a recorder, set the source to the CD capture and then play the CD track again with the player and if you can lower/raise the volume of the captured stream from KMix, while the audible sound is not changed, then KMix does exactly it's job and ALSAMixer is erroneous. If you encounter any problems, then KMix handles the volumes badly, but that does not mean, that ALSAMixer does it correctly, so that should be tested too. Edit: I don't mind, if it's necessary. But please, test it first. ALSAMixer is not error-proof.
Owner

I agree a VM is not the best example for testing

Let's keep an open mind and test as much as we can before we decide the best way forward 😄

I agree a VM is not the best example for testing<br/> Let's keep an open mind and test as much as we can before we decide the best way forward :smile:
Owner

Reporting on the test done on a real laptop with a real CD player.

First of all, alsamixer does not show any CD control.

With code from master (1st picture), I entered an audio CD and played it in KsCD, I can change the volume using Master, Headphone/Speaker, PCM sliders.

With code from PR #17 (2nd picture), the CD still plays fine butI get again duplicated sliders for some entries (Mic Boost and Internal Mic Boost now showing up in the output tab as well, although they are clearly non output devices) and the sliders in the output tab changes also the sliders in the input tab for those duplicated entries.

I will do more testing with alternative code, will report back later. In any case, IMO there should be non duplicated sliders, e.g. each slider can be changed without affecting any other slider.

Reporting on the test done on a real laptop with a real CD player. First of all, alsamixer does not show any CD control. With code from master (1st picture), I entered an audio CD and played it in KsCD, I can change the volume using Master, Headphone/Speaker, PCM sliders. With code from PR #17 (2nd picture), the CD still plays fine butI get again duplicated sliders for some entries (Mic Boost and Internal Mic Boost now showing up in the output tab as well, although they are clearly non output devices) and the sliders in the output tab changes also the sliders in the input tab for those duplicated entries. I will do more testing with alternative code, will report back later. In any case, IMO there should be non duplicated sliders, e.g. each slider can be changed without affecting any other slider.
Owner

Well, it looks I am wrong after all. I checked against alsamixer on that computer and the controls in KMix with PR #17 are a perfect match for what alsamixer shows. Including duplicated sliders for Mic Boost and Internal Mic Boost that change together when one is modified.

I have nevertheless improved and simplified the original code to avoid showing controls in the input section if the device has a capture switch but not a capture volume (this is something alsamixer seems to be doing wrong in my VM, thanks @tch). I will post the code soon for @tch to test.

Well, it looks I am wrong after all. I checked against alsamixer on that computer and the controls in KMix with PR #17 are a perfect match for what alsamixer shows. Including duplicated sliders for Mic Boost and Internal Mic Boost that change together when one is modified. I have nevertheless improved and simplified the original code to avoid showing controls in the input section if the device has a capture switch but not a capture volume (this is something alsamixer seems to be doing wrong in my VM, thanks @tch). I will post the code soon for @tch to test.
Owner

@tch could you test PR #18 on your system and tell me how it goes? It's a cleaner version and only creates sliders in the input tab if the device has a capturing volume.

@tch could you test PR #18 on your system and tell me how it goes? It's a cleaner version and only creates sliders in the input tab if the device has a capturing volume.
tch commented 4 years ago
Poster
Collaborator

I have nevertheless improved and simplified the original code to avoid showing controls in the input section if the device has a capture switch but not a capture volume

If this happens, that is an error for sure, but then it is not caused by my patch as my patch only changed the part which creates the device and appends it to the list, but the "appender" itself is unchanged and that is the point where KMix decides, which panel the device should go. I did not touched that at all. If this error happens, then it must have happen with the current upstream KMix code, without my patch.

However, this is only an error, if the device really does not have a capture volume, just a capture switch. Did you double check that? If you put printf-s to those parts, where it sets the canCapture and isCaptureSwitch, then you get only the latter one and yet, the slider still appears on the Output panel? If yes, then it's an error. If no, you got two ones, then the device actually have a capture volume and it is not an error at all.

Edit: Testing PR #18 now.

> I have nevertheless improved and simplified the original code to avoid showing controls in the input section if the device has a capture switch but not a capture volume If this happens, that is an error for sure, but then it is not caused by my patch as my patch only changed the part which creates the device and appends it to the list, but the "appender" itself is unchanged and that is the point where KMix decides, which panel the device should go. I did not touched that at all. If this error happens, then it must have happen with the current upstream KMix code, without my patch. However, this is only an error, if the device really does not have a capture volume, just a capture switch. Did you double check that? If you put `printf`-s to those parts, where it sets the `canCapture` and `isCaptureSwitch`, then you get only the latter one and yet, the slider still appears on the Output panel? If yes, then it's an error. If no, you got two ones, then the device actually have a capture volume and it is not an error at all. Edit: Testing PR #18 now.
Owner

If this happens, that is an error for sure, but then it is not caused by my patch as my patch only changed the part which creates the device and appends it to the list, but the "appender" itself is unchanged and that is the point where KMix decides, which panel the device should go. I did not touched that at all. If this error happens, then it must have happen with the current upstream KMix code, without my patch.

It wasn't caused by your patch, it was already in KMix code 😄

However, this is only an error, if the device really does not have a capture volume, just a capture switch. Did you double check that?

Yes I did. I put this code in

printf("KMIX  play=%d  cap=%d  playSwitch=%d  capSwitch=%d  isEnum=%d  -  dev=%s\n", canPlay, canCapture, hasPlaySwitch, hasCaptureSwitch,
cc == MixDevice::ENUM, snd_mixer_selem_id_get_name(sid)); fflush(stdout);

and this is the output in my VM.

KMIX  play=1  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Master
KMIX  play=1  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Master Mono
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=3D Control - Switch
KMIX  play=1  cap=0  playSwitch=0  capSwitch=0  isEnum=0  -  dev=3D Control Sigmatel - Depth
KMIX  play=1  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=PCM
KMIX  play=0  cap=0  playSwitch=0  capSwitch=0  isEnum=1  -  dev=PCM Out Path & Mute
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Surround Phase Inversion
KMIX  play=1  cap=0  playSwitch=1  capSwitch=1  isEnum=0  -  dev=Line
KMIX  play=1  cap=0  playSwitch=1  capSwitch=1  isEnum=0  -  dev=CD
KMIX  play=1  cap=0  playSwitch=1  capSwitch=1  isEnum=0  -  dev=Mic
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Mic Boost (+20dB)
KMIX  play=0  cap=0  playSwitch=0  capSwitch=0  isEnum=1  -  dev=Mic Select
KMIX  play=1  cap=0  playSwitch=1  capSwitch=1  isEnum=0  -  dev=Video
KMIX  play=1  cap=0  playSwitch=1  capSwitch=1  isEnum=0  -  dev=Phone
KMIX  play=1  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Beep
KMIX  play=1  cap=0  playSwitch=1  capSwitch=1  isEnum=0  -  dev=Aux
KMIX  play=0  cap=0  playSwitch=0  capSwitch=0  isEnum=1  -  dev=Mono Output Select
KMIX  play=0  cap=1  playSwitch=0  capSwitch=1  isEnum=0  -  dev=Capture
KMIX  play=0  cap=0  playSwitch=0  capSwitch=1  isEnum=0  -  dev=Mix
KMIX  play=0  cap=0  playSwitch=0  capSwitch=1  isEnum=0  -  dev=Mix Mono
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=External Amplifier
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Sigmatel 4-Speaker Stereo
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Sigmatel ADC 6dB Attenuate
KMIX  play=0  cap=0  playSwitch=1  capSwitch=0  isEnum=0  -  dev=Sigmatel DAC 6dB Attenuate

You can see only one device has a capture volume, so this supports your hypothesis that alsamixer was showing wrong sliders in the input section in my VM.

> If this happens, that is an error for sure, but then it is not caused by my patch as my patch only changed the part which creates the device and appends it to the list, but the "appender" itself is unchanged and that is the point where KMix decides, which panel the device should go. I did not touched that at all. If this error happens, then it must have happen with the current upstream KMix code, without my patch. It wasn't caused by your patch, it was already in KMix code :smile: > > However, this is only an error, if the device really does not have a capture volume, just a capture switch. Did you double check that? Yes I did. I put this code in ``` printf("KMIX play=%d cap=%d playSwitch=%d capSwitch=%d isEnum=%d - dev=%s\n", canPlay, canCapture, hasPlaySwitch, hasCaptureSwitch, cc == MixDevice::ENUM, snd_mixer_selem_id_get_name(sid)); fflush(stdout); ``` and this is the output in my VM. ``` KMIX play=1 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Master KMIX play=1 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Master Mono KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=3D Control - Switch KMIX play=1 cap=0 playSwitch=0 capSwitch=0 isEnum=0 - dev=3D Control Sigmatel - Depth KMIX play=1 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=PCM KMIX play=0 cap=0 playSwitch=0 capSwitch=0 isEnum=1 - dev=PCM Out Path & Mute KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Surround Phase Inversion KMIX play=1 cap=0 playSwitch=1 capSwitch=1 isEnum=0 - dev=Line KMIX play=1 cap=0 playSwitch=1 capSwitch=1 isEnum=0 - dev=CD KMIX play=1 cap=0 playSwitch=1 capSwitch=1 isEnum=0 - dev=Mic KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Mic Boost (+20dB) KMIX play=0 cap=0 playSwitch=0 capSwitch=0 isEnum=1 - dev=Mic Select KMIX play=1 cap=0 playSwitch=1 capSwitch=1 isEnum=0 - dev=Video KMIX play=1 cap=0 playSwitch=1 capSwitch=1 isEnum=0 - dev=Phone KMIX play=1 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Beep KMIX play=1 cap=0 playSwitch=1 capSwitch=1 isEnum=0 - dev=Aux KMIX play=0 cap=0 playSwitch=0 capSwitch=0 isEnum=1 - dev=Mono Output Select KMIX play=0 cap=1 playSwitch=0 capSwitch=1 isEnum=0 - dev=Capture KMIX play=0 cap=0 playSwitch=0 capSwitch=1 isEnum=0 - dev=Mix KMIX play=0 cap=0 playSwitch=0 capSwitch=1 isEnum=0 - dev=Mix Mono KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=External Amplifier KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Sigmatel 4-Speaker Stereo KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Sigmatel ADC 6dB Attenuate KMIX play=0 cap=0 playSwitch=1 capSwitch=0 isEnum=0 - dev=Sigmatel DAC 6dB Attenuate ``` You can see only one device has a capture volume, so this supports your hypothesis that alsamixer was showing wrong sliders in the input section in my VM.
tch commented 4 years ago
Poster
Collaborator

It wasn't caused by your patch, it was already in KMix code

Yep, that was i talked about.

You can see only one device has a capture volume, so this supports your hypothesis that alsamixer was showing wrong sliders in the input section in my VM.

I see. Well, no software is error-proof, not even ALSAMixer.
Nevertheless, i've tested your code and works like a charm. Produced equivalent setup with both ALSAMixer and the original patch:

http://oscomp.hu/depot/kmix_good.png

> It wasn't caused by your patch, it was already in KMix code Yep, that was i talked about. > You can see only one device has a capture volume, so this supports your hypothesis that alsamixer was showing wrong sliders in the input section in my VM. I see. Well, no software is error-proof, not even ALSAMixer. Nevertheless, i've tested your code and works like a charm. Produced equivalent setup with both ALSAMixer and the original patch: http://oscomp.hu/depot/kmix_good.png
MicheleC closed this pull request 4 years ago
Owner

Closed as rejected since it has been replaced by PR #18.

Closed as rejected since it has been replaced by PR #18.
MicheleC deleted branch bug/2994/kmix-channels 4 years ago
MicheleC added the ST/rejected label 4 years ago
This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdemultimedia#17
Loading…
There is no content yet.