Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide audio format to pipe output command #4

Conversation

frankl-audio
Copy link

So far, the pipe output of MPD had the problem that the commands
in the pipe had no information about the format of the raw audio data
sent into the pipe.

With this patch mpd sets the environment variables
MPDPIPE_BITS,
MPDPIPE_CHANNELS,
MPDPIPE_RATE
to the number of bits per sample,
the number of channels, and the sample rate of the raw PCM sent to the
pipe, respectively. These variables can then be used in the command or
within a script which is started for the pipe.

So far, the pipe output of MPD had the problem that the commands
in the pipe had no information about the format of the raw audio data
sent into the pipe.

With this patch mpd sets the environment variables
    MPDPIPE_BITS,
    MPDPIPE_CHANNELS,
    MPDPIPE_RATE
to the number of bits per sample,
the number of channels, and the sample rate of the raw PCM sent to the
pipe, respectively. These variables can then be used in the command or
within a script which is started for the pipe.
@frankl-audio
Copy link
Author

This is a nicer version to achieve the same as the rejected pull request #3.

Thanks for the hint to set environment variables; I was not aware of the rules for the environment of a process started with popen but checked that it will always see the variables set here.

The code works fine for me (and is indeed much nicer than in #3), an adjustment of the documentation is included.

@MaxKellermann
Copy link
Member

This implementation is not acceptable, because it leaks the environment variables to the MPD process as well. The problem is, of course, that popen() doesn't allow setting environment variables for the child process. This is where you should use fork()/execve() instead of popen() if available.

@frankl-audio
Copy link
Author

I cannot see any problem with setting these environment variables for the MPD process itself.
Nevertheless, with the new commit the environment is only changed for the child process.

@MaxKellermann
Copy link
Member

Your code will not compile on Windows, because Windows has no fork().

@frankl-audio
Copy link
Author

I wasn't aware that MPD can be used on Windows. I don't have any Windows system and do not know how to find out what is available there. (Have only used cygwin which does have fork().)

You didn't mention Windows after commit d1f170e. Does that work on Windows?
I need a suggestion how to proceed.
From other files I now guess that Windows specific code can be specified with the help of #ifdef WIN32, is this what should be used?

@MaxKellermann
Copy link
Member

I wrote "This is where you should use fork()/execve() instead of popen() if available". You missed the last part. Yes, #ifdef WIN32 would be an acceptable compile-time test.

Use earlier version on Windows because fork() is not available.
Add explicit call of wait to Close method to avoid zombies.
@frankl-audio
Copy link
Author

Ok, thanks for the hint.

@@ -84,12 +97,30 @@ PipeOutput::Create(const ConfigBlock &block, Error &error)
return po;
}

#define set_MPDPIPE_vars(audio_format) {\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't write preprocessor macros when you can write C++.

@MaxKellermann
Copy link
Member

It's been 3 months without a reply. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants