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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Provide audio format to pipe output command #4

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1f170e
Provide audio format to pipe output command
frankl-audio Apr 5, 2016
78f8e10
Only change environment for child process
frankl-audio Apr 10, 2016
1cb1191
Combine last two commits
frankl-audio Apr 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 35 additions & 7 deletions src/output/plugins/PipeOutputPlugin.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@

#include <stdio.h>
#include <stdlib.h>
#ifndef WIN32
#include <unistd.h>
#include <sys/types.h>
#include <sys/wait.h>
#endif

class PipeOutput {
friend struct AudioOutputWrapper<PipeOutput>;
Expand All @@ -37,6 +41,9 @@ class PipeOutput {

std::string cmd;
FILE *fh;
#ifndef WIN32
pid_t childpid;
#endif

PipeOutput()
:base(pipe_output_plugin) {}
Expand All @@ -49,7 +56,13 @@ class PipeOutput {
bool Open(AudioFormat &audio_format, Error &error);

void Close() {
#ifdef WIN32
pclose(fh);
#else
int status;
fclose(fh);
waitpid(childpid, &status, 0);
#endif
}

size_t Play(const void *chunk, size_t size, Error &error);
Expand Down Expand Up @@ -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++.

char strbuf[8];\
setenv("MPDPIPE_BITS", sample_format_to_string(audio_format.format), 1);\
snprintf(strbuf, sizeof(strbuf), "%u", audio_format.sample_rate);\
setenv("MPDPIPE_RATE", strbuf, 1);\
snprintf(strbuf, sizeof(strbuf), "%u", audio_format.channels);\
setenv("MPDPIPE_CHANNELS", strbuf, 1);\
}

inline bool
PipeOutput::Open(AudioFormat &audio_format, Error &error)
{
#ifdef WIN32
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.

This sets environment variables in the parent process, which I already told you is not acceptable. (Don't argue with me again. I don't accept it.)

fh = popen(cmd.c_str(), "w");
Copy link
Member

Choose a reason for hiding this comment

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

This commit re-adds code that has been removed before. Since I don't merge your original (bad) commit, I can't merge this one, because it conflicts.

if (fh == nullptr) {
error.FormatErrno("Error opening pipe \"%s\"",
cmd.c_str());
return false;
}

return true;
#else
int pfdes[2];
pid_t childpid;
char strbuf[8];

if (pipe(pfdes) == -1) {
error.FormatErrno("Error opening pipe for output.");
Expand All @@ -112,15 +143,12 @@ PipeOutput::Open(AudioFormat &audio_format, Error &error)
close(pfdes[1]);
dup2(pfdes[0], 0);
close(pfdes[0]);
setenv("MPDPIPE_BITS", sample_format_to_string(audio_format.format), 1);
snprintf(strbuf, sizeof(strbuf), "%u", audio_format.sample_rate);
setenv("MPDPIPE_RATE", strbuf, 1);
snprintf(strbuf, sizeof(strbuf), "%u", audio_format.channels);
setenv("MPDPIPE_CHANNELS", strbuf, 1);
set_MPDPIPE_vars(audio_format);
execlp("sh", "/bin/sh", "-c", cmd.c_str(), (char*)NULL);
error.FormatErrno("Cannot execute pipe program \"%s\"", cmd.c_str());
abort();
}
#endif
}

inline size_t
Expand Down