Loading…
Reference in new issue
There is no content yet.
Delete Branch 'bug/2994/kmix-channels'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Deleting a branch is permanent. It CANNOT be undone. Continue?
On branch bug/2994/kmix-channels
Changes to be committed:
modified: kmix/mixer_alsa9.cpp
Committer: TCH <tch@protonmail.com>to bug 2994 fix 4 years agoCongratulations, you have your first pull-request!
There are some formalities that need to be fixed:
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.
You can use the following procedure to fix the git log:
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.git push origin HEAD -f
This will push the new version of the commit over the existing one.
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.
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.
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?
Which was exactly the point. There are devices which can capture and play too, so they must appear on both panel.
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.
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".
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.
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.
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.
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.
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.
@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.
I have done some testing on my system (debian buster running inside a VM in a Windows host). Here are the info.
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)
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.
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??
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.
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.
ok, thanks for the explanation!!
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.
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
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:
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.
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 😄
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.
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.
@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.
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 thecanCapture
andisCaptureSwitch
, 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.
It wasn't caused by your patch, it was already in KMix code 😄
Yes I did. I put this code in
and this is the output in my VM.
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.
Yep, that was i talked about.
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
Closed as rejected since it has been replaced by PR #18.