Re: [Musicpd-dev-team] [git pull] mixer pulse
Max Kellermann wrote: On 2009/01/20 22:51, Linel Patrice patnathan...@gmail.com wrote: This is an intermediate commit to do a push on another server. Don't submit intermediate commits. Having intermediate commits in the official repository may break bisections. A patch should be complete and self-contained, i.e. it improves MPD, and doesn't introduce a regression. If you fix bugs in a patch which isn't merged yet, fold the fixes into the existing patch. The patch Begin implementation for pulse mixer (d8efc9e0) looks pointless, because it only adds some commented code. Yes, but it is an old one , you want me to remove it ? Yes. I see no reason to merge this commit, it doesn't improve MPD. What does not work, is when i launch mpd without --no-daemon it fails to get the volume on the mixer. So I don't understand why. Will you be able to find out? Or do you need help? I will try , and I tell you know. Do you want me to rewrite all my commit before doing the rebase? I don't know if it makes sense to split the pulse mixer into separate patches. So the easiest way to get your code merged would be to copy your pulse_mixer.c into a new clone of the master repository, and only apply the Makefile.am changes. This way, you don't have to go through all the rebasing/merging pain again. Ok i'll do this after fixing the warnings. You prefer not to have a empty function (solving the certain unused variable), or have the warning? There are some of the warning ,I can not fix cause it is a callback function of pulse. I will never get tired of telling people to use stgit! stgit allows you to refine your patches until they are complete, without creating tons of useless non-self-cointained intermediate patches. Max -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [git pull] mixer pulse
Max Kellermann wrote: On 2009/01/20 21:29, Linel Patrice patnathan...@gmail.com wrote: I recoded the mixer , in a way it does not change the existing output pulse. git://git.musicpd.org/pat/mpd.git You should only need to pick the last commit. I cannot pick the last commit, because it is a merge commit. Don't merge, rebase on master instead (git pull --rebase or stg rebase). If you need any help with git usage, you're welcome to ask. There are two patches with identical description, but with a different subject (9218b2e3a and b354929). I don't understand the description at all. It is the merge commit, i thought i had to rewrite it. There are more patches, most of which I don't understand. e.g. 4fe20df is described as envoi pr david ??? This is an intermediate commit to do a push on another server. The patch Begin implementation for pulse mixer (d8efc9e0) looks pointless, because it only adds some commented code. Yes, but it is an old one , you want me to remove it ? Your code doesn't compile with --enable-werror (always enable this option during development!): cc1: warnings being treated as errors mixer/pulse_mixer.c:22: error: unused parameter 'context' mixer/pulse_mixer.c:44: error: unused parameter 'context' mixer/pulse_mixer.c: In function 'subscribe_cb': mixer/pulse_mixer.c:61: error: declaration of 'index' shadows a global declaration /usr/include/string.h:309: error: shadowed declaration is here mixer/pulse_mixer.c: At top level: mixer/pulse_mixer.c:61: error: unused parameter 'c' mixer/pulse_mixer.c: In function 'context_state_cb': mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_UNCONNECTED' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_CONNECTING' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_AUTHORIZING' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_SETTING_NAME' not handled in switch mixer/pulse_mixer.c: In function 'pulse_mixer_configure': mixer/pulse_mixer.c:160: error: unused variable 'ret' mixer/pulse_mixer.c:159: error: unused variable 'o' mixer/pulse_mixer.c: In function 'pulse_mixer_open': mixer/pulse_mixer.c:194: error: unused variable 'o' mixer/pulse_mixer.c:193: error: unused variable 'pm' mixer/pulse_mixer.c: In function 'pulse_mixer_close': mixer/pulse_mixer.c:208: error: unused variable 'pm' make[2]: *** [mpd-pulse_mixer.o] Error 1 More formal stuff: try to keep lines less than 80 characters long. Exceptions are allowed when every other solution is even less readable. That's not the case in pulse_mixer.c, please break those overlong lines. Try to keep MPD's code style. Why a closure for each case statement? pulse_mixer.c has lots of whitespace errors (spaces at the end of the line). Your code doesn't have proper error handling: if(!(pm-mainloop = pa_threaded_mainloop_new())) g_debug(Failed mainloop\n); if(!(pm-context = pa_context_new(pa_threaded_mainloop_get_api(pm-mainloop),Mixer mpd))) g_debug(Failed context\n); pa_context_set_state_callback(pm-context, context_state_cb, pm); So you check the result, but continue to work with NULL pointers. This will crash! I need some feedback on that please. It still not work in daemon. What do you mean, it does not work? Do not send a pull request if you know it doesn't work - we can talk about your code, but I'll pull only when every single patch you submit works and is self-contained, and doesn't introduce a regression. It's important that every commit works (at the best of our knowledge), because a broken commit would make bisecting hard or impossible. Please describe what exactly does not work. Max What does not work, is when i launch mpd without --no-daemon it fails to get the volume on the mixer. So I don't understand why. Do you want me to rewrite all my commit before doing the rebase? -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] Pulse mixer
Max Kellermann wrote: Hello Linel, Hi I happened to notice that you already pushed code to your public git repository. I cannot merge this right now, because Viliam has changed the mixer API meanwhile. Yes, I saw this, I'm a little busy this week but I already started to make the changes for the last API version. If you have any patches which should be merged into master, please send a pull request on the mailing list (pull git://git.musicpd.org/pat/mpd.git mixer_pulse, not replying to existing thread or I'll miss it), I'll merge your patches then. If patches get merged early, they will be adapted to all API changes by other developers. If you want the pulse mixer merged, please adapt it to the new API, and submit the new patch. If you create a patch named correct the new output pulse implementation, explain what exactly is corrected. It is not obvious to me, because I don't know pulse. When you submit the new patch, you can fold the correction patch into the initial pulse mixer patch. The patch Begin implementation for pulse mixer is superfluous, because it adds commented code. This does not make sens, and this patch does not introduce any progress. Your patch initializes a pa_threaded_mainloop. Does that introduce overhead? If yes, it must definitely be optional. The whole pulse mixer code should be optional, for those who prefer to use their sound chip's hardware volume control, and those people should not experience any slowdown due to your patches. The pa_threaded_mainloop control the mainloop of pulseaudio. So if the pulse output is not used, there are no reasons to introduce overhead. However I have a problem when a try daemonize mpd, may be the mainloop stay locked. I'll look at it as soon as possible. Max Pat. -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] Pulse mixer
Max Kellermann wrote: On 2009/01/13 10:29, Linel Patrice patnathan...@gmail.com wrote: The pa_threaded_mainloop control the mainloop of pulseaudio. So if the pulse output is not used, there are no reasons to introduce overhead. We didn't need the main loop stuff before your patch. I don't know the pulse API, please explain why you're using it now. Is it required for your mixer code? If the user chooses to not use your pulse mixer code (but uses a pulse output), can we switch off the main loop? Max You did not need the mainloop cause the mainloop was enclosed in the function of the simple api of pulse. To control the pulse mixer, you need to have the context( struct for connexion) , the mainloop of pulse , and the stream struct. Strictly , I only need a connexion to the server , the mainloop and the name of the stream. But if the output have it, let's use the same pointer and thus use the stream to drive the mixer. It is clear or not? -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] pulse audio mixer
Avuton Olrich wrote: On Thu, Jan 8, 2009 at 4:32 PM, Patrice patnathan...@gmail.com wrote: I saw the software mixer work. But it is ,in my opinion, better if when i set the the volume in mpd, it affect the pulse volume which is only the mpd one. I work on that and i already have a working version. Could this interest the community? Of course that's good. Interested in a git tree, or do you possibly want to just post patches here? If you want a git tree send me your pub key privately. Hi, I have a problem with the daemonize process. All work when i execute it with --no-daemon, but in daemon mode it hangs. I don't know much about daemon and how it affect the code. Can you advise me please? -- Check out the new SourceForge.net Marketplace. It is the best place to buy or sell services for just about anything Open Source. http://p.sf.net/sfu/Xq1LFB ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
[Musicpd-dev-team] pulse audio mixer
Hi, I want to know if you have in mind to develop the mixer for pulse audio? If no , why? Thanks -- Check out the new SourceForge.net Marketplace. It is the best place to buy or sell services for just about anything Open Source. http://p.sf.net/sfu/Xq1LFB ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
