[pulseaudio-discuss] [PATCH 2/2] add module-tunnel-sink-new: experimental rewrite of module-tunnel using libpulse
Tanu Kaskinen
tanu.kaskinen at linux.intel.com
Fri Aug 2 01:17:26 PDT 2013
On Mon, 2013-07-22 at 01:50 +0200, Alexander Couzens wrote:
> Signed-off-by: Alexander Couzens <lynxis at fe80.eu>
The commit message needs more content: short introduction to why this
rewrite is being done, and what functionality has been intentionally
omitted from this first version.
> ---
> src/Makefile.am | 7 +
> src/modules/module-tunnel-sink-new.c | 516 +++++++++++++++++++++++++++++++++++
> 2 files changed, 523 insertions(+)
> create mode 100644 src/modules/module-tunnel-sink-new.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 6de6e96..a602ee4 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1097,6 +1097,7 @@ modlibexec_LTLIBRARIES += \
> module-remap-sink.la \
> module-remap-source.la \
> module-ladspa-sink.la \
> + module-tunnel-sink-new.la \
> module-tunnel-sink.la \
> module-tunnel-source.la \
> module-position-event-sounds.la \
> @@ -1368,6 +1369,7 @@ SYMDEF_FILES = \
> module-ladspa-sink-symdef.h \
> module-equalizer-sink-symdef.h \
> module-match-symdef.h \
> + module-tunnel-sink-new-symdef.h \
> module-tunnel-sink-symdef.h \
> module-tunnel-source-symdef.h \
> module-null-sink-symdef.h \
> @@ -1638,6 +1640,11 @@ module_match_la_SOURCES = modules/module-match.c
> module_match_la_LDFLAGS = $(MODULE_LDFLAGS)
> module_match_la_LIBADD = $(MODULE_LIBADD)
>
> +module_tunnel_sink_new_la_SOURCES = modules/module-tunnel-sink-new.c
> +module_tunnel_sink_new_la_CFLAGS = -DTUNNEL_SINK_NEW=1 $(AM_CFLAGS)
-DTUNNEL_SINK_NEW=1 is not necessary. -DTUNNEL_SINK is used with the old
module, because the same file is used for both sink and source, with
ifdefs for handling the parts that are different between sink and
source. Those ifdefs use the TUNNEL_SINK definition, but your code
doesn't use TUNNEL_SINK_NEW for anything.
> +module_tunnel_sink_new_la_LDFLAGS = $(MODULE_LDFLAGS)
> +module_tunnel_sink_new_la_LIBADD = $(MODULE_LIBADD)
> +
> module_tunnel_sink_la_SOURCES = modules/module-tunnel.c
> module_tunnel_sink_la_CFLAGS = -DTUNNEL_SINK=1 $(AM_CFLAGS)
> module_tunnel_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
> diff --git a/src/modules/module-tunnel-sink-new.c b/src/modules/module-tunnel-sink-new.c
> new file mode 100644
> index 0000000..aa7cefc
> --- /dev/null
> +++ b/src/modules/module-tunnel-sink-new.c
> @@ -0,0 +1,516 @@
> +/***
> + This file is part of PulseAudio.
> +
> + Copyright 2013 Alexander Couzens
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as published
> + by the Free Software Foundation; either version 2.1 of the License,
> + or (at your option) any later version.
> +
> + PulseAudio is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with PulseAudio; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> + USA.
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <pulse/context.h>
> +#include <pulse/rtclock.h>
> +#include <pulse/timeval.h>
> +#include <pulse/xmalloc.h>
> +#include <pulse/stream.h>
> +#include <pulse/mainloop.h>
> +
> +#include <pulsecore/core.h>
> +#include <pulsecore/core-util.h>
> +#include <pulsecore/i18n.h>
> +#include <pulsecore/sink.h>
> +#include <pulsecore/modargs.h>
> +#include <pulsecore/log.h>
> +#include <pulsecore/thread.h>
> +#include <pulsecore/thread-mq.h>
> +#include <pulsecore/rtpoll.h>
> +#include <pulsecore/poll.h>
> +
> +#include "module-tunnel-sink-new-symdef.h"
> +
> +PA_MODULE_AUTHOR("Alexander Couzens");
> +PA_MODULE_DESCRIPTION(_("Create a network sink which connects via a stream to a remote pulseserver"));
"Pulseserver" is not good language in my opinion, "PulseAudio server"
would be better.
> +PA_MODULE_VERSION(PACKAGE_VERSION);
> +PA_MODULE_LOAD_ONCE(false);
> +PA_MODULE_USAGE(_("sink_name=<name of sink>"));
sink_properties and remote_server aren't mentioned in the usage text.
> +
> +#define DEFAULT_SINK_NAME "remote_sink"
I prefer the old default sink name: tunnel-sink.<server_name>
> +
> +#define MEMBLOCKQ_MAXLENGTH (16*1024*1024)
This is not used for anything.
> +
> +/* libpulse callbacks */
> +static void stream_state_callback(pa_stream *stream, void *userdata);
> +static void context_state_callback(pa_context *c, void *userdata);
> +
> +struct userdata {
> + pa_module *module;
> +
> + pa_sink *sink;
> + pa_rtpoll *rtpoll;
rtpoll is not used for anything.
> + pa_thread_mq thread_mq;
> + pa_thread_mq thread_mq_rt_shadow; /* same as thread_mq but has swapped inq/outq and own io_events */
thread_mq_rt_shadow is not used for anything.
> + pa_thread *thread;
> +
> + pa_memchunk memchunk;
> +
> + unsigned channels;
> + pa_usec_t block_usec;
> + pa_usec_t timestamp;
channels, block_usec and timestamp are not used for anything.
> +
> + // libpulse context
No C++ style comments, please. Also, this comment doesn't have any
useful information (I think we can expect the reader to know what
pa_context is).
> + pa_context *context;
> + pa_stream *stream;
> +
> + bool connected;
> +
> + pa_mainloop *rt_mainloop;
> +
> + const char *remote_server;
Strings that you allocate and free yourself shouldn't be const.
> +};
> +
> +static const char* const valid_modargs[] = {
> + "sink_name",
> + "sink_properties",
> + "remote_server",
In the old tunnel module the argument name was just "server". We
shouldn't break compatibility with old configuration files, so please
rename "remote_server" to "server".
> + NULL,
> +};
> +
> +enum {
> + SINK_MESSAGE_PASS_SOCKET = PA_SINK_MESSAGE_MAX,
> + SINK_MESSAGE_RIP_SOCKET
> +};
RIP_SOCKET isn't used, and PASS_SOCKET doesn't seem useful either.
> +
> +static void thread_func(void *userdata) {
> + struct userdata *u = userdata;
> + pa_proplist *proplist;
> + pa_mainloop_api *rt_mainloop;
The rt_mainloop variable doesn't seem very useful.
> +
> + pa_assert(u);
> +
> + pa_log_debug("Tunnelstream: Thread starting up");
"Tunnelstream" could be removed from the message. The log message will
anyway contain the file name and the thread name.
> +
> + rt_mainloop = pa_mainloop_get_api(u->rt_mainloop);
> + pa_log("rt_mainloop_api : %p", rt_mainloop );
Looks like a debug message that you forgot to remove.
> +
> + pa_thread_mq_install(&u->thread_mq);
> +
> +
> + u->timestamp = pa_rtclock_now();
> +
> +
> +
Too many empty lines. One empty line ought to be enough for anybody.
> + /* TODO: think about volume stuff remote<--stream--source */
While it's not wrong in general to have TODO comments, this seems to be
a bit out of place (perhaps in the beginning of the file would have been
a better place). Since you have already implemented the volume
functionality, this comment can be removed from this patch.
> + proplist = pa_proplist_new();
> + pa_proplist_sets(proplist, PA_PROP_APPLICATION_NAME, _("PulseAudio mod-tunnelstream"));
The application name is just PulseAudio.
> + pa_proplist_sets(proplist, PA_PROP_APPLICATION_ID, "mod-tunnelstream");
This used to be org.PulseAudio.PulseAudio. I see no reason to change it.
> + pa_proplist_sets(proplist, PA_PROP_APPLICATION_ICON_NAME, "audio-card");
> + pa_proplist_sets(proplist, PA_PROP_APPLICATION_VERSION, PACKAGE_VERSION);
The old tunnel module called pa_init_proplist(), which was probably a
good idea (it sets a bunch of additional properties).
> +
> + /* init libpulse */
> + if (!(u->context = pa_context_new_with_proplist(pa_mainloop_get_api(u->rt_mainloop),
> + "tunnelstream",
> + proplist))) {
We don't have explicit rules for aligning wrapped lines, but I'm pretty
sure it would be more consistent with other code (and better looking
IMO) to align the wrapped parts to start after the opening parenthesis,
i.e. indent by 6 more spaces.
> + pa_log("Failed to create libpulse context");
> + goto fail;
> + }
> +
> + pa_context_set_state_callback(u->context, context_state_callback, u);
> + if (pa_context_connect(u->context,
> + u->remote_server,
> + PA_CONTEXT_NOFAIL | PA_CONTEXT_NOAUTOSPAWN,
> + NULL) < 0) {
Here the alignment is off too by one space.
The NOFAIL flag doesn't do anything when the server parameter is
non-NULL. The reason is that the NOFAIL functionality monitors D-Bus and
reacts when PulseAudio registers itself on the bus, but if you connect
to a remote machine, there is no D-Bus bus that libpulse could connect
to.
> + pa_log("Failed to connect libpulse context");
> + goto fail;
> + }
> +
> + pa_proplist_free(proplist);
If pa_context_new_with_proplist() or pa_context_connect() fails, the
proplist gets leaked.
> +
> + for(;;)
> + {
Missing space after "for" and the opening brace should be on the
previous line.
> + int ret;
> + const void *p;
> + pa_usec_t now = 0;
now is not used.
> +
> + size_t writeable = 0;
I guess both "writeable" and "writable" are correct English, but since
this variable is assigned the return value of pa_stream_writable_size(),
it would be more consistent to use "writable" as the variable name.
> +
> + if(pa_mainloop_iterate(u->rt_mainloop, 1, &ret) < 0) {
Missing space after "if".
> + if(ret == 0)
> + goto finish;
> + else
> + goto fail;
> +
This empty line looks ugly to me.
> + }
> +
> + if (PA_UNLIKELY(u->sink->thread_info.rewind_requested))
> + pa_sink_process_rewind(u->sink, 0);
> +
> + if (PA_SINK_IS_OPENED(u->sink->thread_info.state))
> + now = pa_rtclock_now();
> +
> + if (u->connected &&
> + PA_STREAM_IS_GOOD(pa_stream_get_state(u->stream)) &&
> + PA_SINK_IS_OPENED(u->sink->thread_info.state)) {
> + /* TODO: use IS_RUNNING + cork stream */
It's not a good idea to use IS_RUNNING. You should keep the stream
running when the sink is opened (IDLE or RUNNING). You should cork the
stream when the sink is suspended. For this patch you don't need to
implement the corking, but you should change the PA_SINK_IS_OPENED check
to PA_SINK_IS_LINKED, because if you don't cork the stream while the
sink is suspended, you need to generate audio all the time.
> +
> + if (pa_stream_is_corked(u->stream)) {
> + pa_stream_cork(u->stream, 0, NULL, NULL);
You have to unref the pa_operation object that pa_stream_cork() returns.
> + } else {
> + writeable = pa_stream_writable_size(u->stream);
> + if (writeable > 0) {
> + if (u->memchunk.length <= 0)
u->memchunk.length is always 0 here.
BTW, there doesn't seem to be use for u->memchunk outside this function,
so it could be a local variable.
> + pa_sink_render(u->sink, writeable, &u->memchunk);
> +
> + pa_assert(u->memchunk.length > 0);
> +
> +
Extra empty line.
> + /* we have new data to write */
> + p = (const uint8_t *) pa_memblock_acquire(u->memchunk.memblock);
> + ret = pa_stream_write(u->stream,
> + ((uint8_t*) p + u->memchunk.index), /**< The data to write */
> + u->memchunk.length, /**< The length of the data to write in bytes */
> + NULL, /**< A cleanup routine for the data or NULL to request an internal copy */
> + 0, /**< Offset for seeking, must be 0 for upload streams */
> + PA_SEEK_RELATIVE /**< Seek mode, must be PA_SEEK_RELATIVE for upload streams */
> + );
The indentation alignment is again off by two spaces.
The comments that are copied from stream.h look ugly. At the very least
align the comments, but I don't think they are very useful at all (there
is some value in commenting what the NULL and 0 values mean, though).
> + pa_memblock_release(u->memchunk.memblock);
> + pa_memblock_unref(u->memchunk.memblock);
> + pa_memchunk_reset(&u->memchunk);
> +
> + if (ret != 0) {
> + /* TODO: we should consider a state change or is that already done ? */
> + pa_log_warn("Could not write data into the stream ... ret = %i", ret);
If pa_stream_writable_size() says that N bytes can be written, and then
writing N bytes fails, we can just give up and goto fail.
> + }
> + }
Now data is written whenever the mainloop wakes up, but I think the
write pattern would be easier to reason about if you wrote data only
when the write callback is called. Note that if you implement that
model, you shouldn't render audio inside the callback, because rewind
requests have to be always processed before rendering. The write
callback should only set a flag, and thread_func() should then do the
rendering after processing the rewind requests, if the flag is set.
> + }
> + }
> + }
> +fail:
> + /* If this was no regular exit from the loop we have to continue
> + * processing messages until we received PA_MESSAGE_SHUTDOWN
> + *
> + * Note: is this a race condition? When a PA_MESSAGE_SHUTDOWN already within the queue?
> + */
> + pa_asyncmsgq_flush(u->thread_mq.inq, false);
There wouldn't be a race condition if you didn't flush the message
queue. Flushing is not needed.
> + pa_asyncmsgq_post(u->thread_mq.outq, PA_MSGOBJECT(u->module->core), PA_CORE_MESSAGE_UNLOAD_MODULE, u->module, 0, NULL, NULL);
> + pa_asyncmsgq_wait_for(u->thread_mq.inq, PA_MESSAGE_SHUTDOWN);
> +
> +finish:
> + pa_asyncmsgq_flush(u->thread_mq.inq, false);
Flushing is not needed here either - it's a bug if somebody sends a
message after PA_MESSAGE_SHUTDOWN.
> +
> + if (u->stream)
> + pa_stream_disconnect(u->stream);
> +
> + if (u->context)
> + pa_context_disconnect(u->context);
> +
> + if(u->rt_mainloop)
> + pa_mainloop_free(u->rt_mainloop);
u->rt_mainloop is created in the main thread, so it should also be freed
in the main thread, for consistency reasons and also because your
current code leaks u->rt_mainloop if pa__init() fails before the thread
starts.
> +
> + pa_log_debug("Thread shutting down");
> +}
> +
> +static void stream_state_callback(pa_stream *stream, void *userdata) {
> + struct userdata *u = userdata;
> +
> + pa_assert(u);
> + pa_assert(stream == u->stream);
> +
> + switch(pa_stream_get_state(stream)) {
> + case PA_STREAM_FAILED:
> + pa_log_debug("Context failed.");
Incorrect log message - it's the stream that failed, not context.
> + pa_stream_unref(stream);
> + u->stream = NULL;
> +
> + /* TODO: think about killing the context or should we just try again a creationg of a stream ? */
We don't know the reason for the stream failure. It may have been killed
by the user at the remote end. Recreating the stream in that case is
probably not what the user wants. I think we should tie
module-tunnel-sink lifetime to the lifetime of a single stream. If the
stream dies, module-tunnel-sink should be unloaded.
To handle cases where the remote server disappears temporarily,
module-zeroconf-discover should take care of loading the tunnel sink
again (I don't know if it does this already, and if it doesn't, I don't
know why).
> + if(u->context)
> + pa_context_disconnect(u->context);
I think it would be simpler to put all teardown code (disconnecting and
unreffing the stream and context) at the end of thread_func(). When
failure occurs, you can just tell the mainloop to quit.
> + break;
> + case PA_STREAM_TERMINATED:
> + pa_log_debug("Context terminated.");
> + pa_stream_unref(stream);
> + u->stream = NULL;
> +
> + if(u->context)
> + pa_context_disconnect(u->context);
Same comments as for PA_STREAM_FAILED.
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static void context_state_callback(pa_context *c, void *userdata) {
> + struct userdata *u = userdata;
> + int c_errno;
c_errno is not used for anything.
> +
> + pa_assert(u);
> + pa_assert(u->context == c);
> +
> + switch(pa_context_get_state(c)) {
Missing space after "switch".
> + case PA_CONTEXT_UNCONNECTED:
> + case PA_CONTEXT_CONNECTING:
> + case PA_CONTEXT_AUTHORIZING:
> + case PA_CONTEXT_SETTING_NAME:
> + pa_log_debug("Connection unconnected");
I think this is not helpful message, so it should be removed.
> + break;
> + case PA_CONTEXT_READY: {
> + pa_proplist *proplist;
> + pa_buffer_attr bufferattr;
> +
> + pa_log_debug("Connection successful. Creating stream.");
> + pa_assert(!u->stream);
> +
> + proplist = pa_proplist_new();
> + pa_assert(proplist);
> +
> +
Extra empty line.
> + u->stream = pa_stream_new_with_proplist(u->context,
> + "mod-tunnelstream",
The old tunnel module uses stream name "<sink_name> for <user>@<host>".
I think that or something similar would be better than
"mod-tunnelstream".
> + &u->sink->sample_spec,
> + &u->sink->channel_map,
> + proplist);
This call can fail, and the failure needs to be handled.
Also, there's no point calling pa_stream_new_with_proplist() if you
don't set any properties.
> +
> + pa_proplist_free(proplist);
> +
> +
Extra space.
> + memset(&bufferattr, 0, sizeof(pa_buffer_attr));
We have pa_zero() for zeroing structs.
> +
> + bufferattr.maxlength = (uint32_t) - 1;
> + bufferattr.minreq = (uint32_t) - 1;
> + bufferattr.prebuf = (uint32_t) - 1;
> + bufferattr.tlength = (uint32_t) - 1;
"-1" instead of "- 1".
> +
> + pa_stream_set_state_callback(u->stream, stream_state_callback, userdata);
> + pa_stream_connect_playback(u->stream,
> + NULL,
u->sink_name should be used here.
> + &bufferattr,
> + PA_STREAM_START_CORKED | PA_STREAM_AUTO_TIMING_UPDATE,
The old tunnel module sets also the PA_STREAM_DONT_MOVE flag, and I
think you should set that too.
> + NULL,
> + NULL);
pa_stream_connect_playback() can fail, at least in theory. I checked the
function implementation, and it doesn't seem possible that the function
could fail in practice, but the implementation can change in the future.
> +
> + pa_asyncmsgq_post(u->thread_mq.inq, PA_MSGOBJECT(u->sink), SINK_MESSAGE_PASS_SOCKET, NULL, 0, NULL, NULL);
No need to send messages, just set connected = true.
> + break;
> + }
> + case PA_CONTEXT_FAILED:
> + c_errno = pa_context_errno(u->context);
> + pa_log_debug("Context failed.");
> + u->connected = false;
> + pa_context_unref(u->context);
> + u->context = NULL;
> +
> + pa_module_unload_request(u->module, false);
If you put all teardown code in the end of thread_func() as I suggested,
you won't need to request module unloading here. Just tell the mainloop
to quit. Calling pa_module_unload_request() from the IO thread is not
safe anyway.
> + break;
> +
> + case PA_CONTEXT_TERMINATED:
> + c_errno = pa_context_errno(u->context);
> + pa_log_debug("Context terminated.");
> + u->connected = false;
> + pa_context_unref(u->context);
> + u->context = NULL;
> +
> + pa_module_unload_request(u->module, false);
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static int sink_process_msg_cb(pa_msgobject *o, int code, void *data, int64_t offset, pa_memchunk *chunk) {
> + struct userdata *u = PA_SINK(o)->userdata;
> +
> + switch (code) {
> +
> + case PA_SINK_MESSAGE_GET_LATENCY: {
> + int negative;
> + pa_usec_t remote_latency;
> +
> + if (!PA_SINK_IS_LINKED(u->sink->thread_info.state)) {
> + *((pa_usec_t*) data) = 0;
> + return 0;
> + }
> +
> + if(!u->stream) {
Missing space after "if".
> + *((pa_usec_t*) data) = 0;
> + return 0;
> + }
> +
> + if(!PA_STREAM_IS_GOOD(pa_stream_get_state(u->stream))) {
Missing space after "if".
> + *((pa_usec_t*) data) = 0;
> + return 0;
> + }
> +
> + if(pa_stream_get_latency(u->stream, &remote_latency, &negative) < 0) {
Missing space after "if".
> + *((pa_usec_t*) data) = 0;
> + return 0;
> + }
> +
> + *((pa_usec_t*) data) =
> + /* Add the latency from libpulse */
> + remote_latency;
> + /* do we have to add more latency here ? */
If you would store any data in the sink in some buffer, then you would
need to add that, but I don't think there is any such buffer
(u->memchunk never contains any data).
> + return 0;
> + }
> + case SINK_MESSAGE_PASS_SOCKET: {
> + u->connected = true;
> + return 0;
> + }
> + }
> + return pa_sink_process_msg(o, code, data, offset, chunk);
> +}
> +
> +int pa__init(pa_module*m) {
Missing space before "*".
> + struct userdata *u = NULL;
> + pa_modargs *ma = NULL;
> + pa_sink_new_data sink_data;
> + pa_sample_spec ss;
> + pa_channel_map map;
> + pa_proplist *proplist = NULL;
proplist is not used for anything.
> + const char *remote_server = NULL;
> +
> + pa_assert(m);
> +
> + if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
> + pa_log("Failed to parse module arguments.");
> + goto fail;
> + }
> +
> + ss = m->core->default_sample_spec;
> + map = m->core->default_channel_map;
> + if (pa_modargs_get_sample_spec_and_channel_map(ma, &ss, &map, PA_CHANNEL_MAP_DEFAULT) < 0) {
"rate", "format", "channels" and "channel_map" should be added to
valid_modargs if you're going to allow configuration of the parameters.
And well, actually you need to add them anyway to keep compatibility
with the old tunnel sink.
> + pa_log("Invalid sample format specification or channel map");
> + goto fail;
> + }
> +
> + remote_server = pa_modargs_get_value(ma, "remote_server", NULL);
> + if (!remote_server) {
> + pa_log("No remote_server given!");
> + goto fail;
> + }
> +
> + u = pa_xnew0(struct userdata, 1);
> + u->module = m;
> + m->userdata = u;
> + u->remote_server = strdup(remote_server);
We have pa_xstrdup() which should be used here.
> + pa_memchunk_reset(&u->memchunk);
> + u->rtpoll = pa_rtpoll_new();
> + u->rt_mainloop = pa_mainloop_new();
> + if(u->rt_mainloop == NULL) {
Missing space after "if", and I'd prefer "!u->rt_mainloop" instead of
"u->rt_mainloop == NULL".
> + pa_log("Failed to create mainloop");
> + goto fail;
> + }
> +
> + pa_thread_mq_init_rtmainloop(&u->thread_mq, m->core->mainloop, pa_mainloop_get_api(u->rt_mainloop));
> +
> + /* Create sink */
> + pa_sink_new_data_init(&sink_data);
> + sink_data.driver = __FILE__;
> + sink_data.module = m;
> +
> + pa_sink_new_data_set_name(&sink_data, pa_modargs_get_value(ma, "sink_name", DEFAULT_SINK_NAME));
> + pa_sink_new_data_set_sample_spec(&sink_data, &ss);
> + pa_sink_new_data_set_channel_map(&sink_data, &map);
> +
> + /* TODO: set DEVICE CLASS */
What does this mean?
> + pa_proplist_sets(sink_data.proplist, PA_PROP_DEVICE_CLASS, "abstract");
> +
> + pa_proplist_setf(sink_data.proplist, PA_PROP_DEVICE_DESCRIPTION, "tunnel to remote pulseaudio %s", remote_server);
The description should be translatable, and it should be start with an
upper case letter. I think it would also be good to have "at" before the
remote server name. Or drop the "remote pulseaudio" part. And including
the remote sink name in the description might be good too.
> +
> + if (pa_modargs_get_proplist(ma, "sink_properties", sink_data.proplist, PA_UPDATE_REPLACE) < 0) {
> + pa_log("Invalid properties");
> + pa_sink_new_data_done(&sink_data);
> + goto fail;
> + }
> + /* TODO: check PA_SINK_LATENCY + PA_SINK_DYNAMIC_LATENCY */
> + if (!(u->sink = pa_sink_new(m->core, &sink_data, (PA_SINK_LATENCY|PA_SINK_DYNAMIC_LATENCY|PA_SINK_NETWORK)))) {
This patch doesn't implement dynamic latency, so the flag shouldn't be
specified.
> + pa_log("Failed to create sink.");
> + pa_sink_new_data_done(&sink_data);
> + goto fail;
> + }
> +
> + pa_sink_new_data_done(&sink_data);
> + u->sink->userdata = u;
> +
> + /* callbacks */
> + u->sink->parent.process_msg = sink_process_msg_cb;
> +
> + /* set thread queue */
> + pa_sink_set_asyncmsgq(u->sink, u->thread_mq.inq);
> +
> + /* TODO: latency / rewind
> + u->sink->update_requested_latency = sink_update_requested_latency_cb;
> + u->block_usec = BLOCK_USEC;
> + nbytes = pa_usec_to_bytes(u->block_usec, &u->sink->sample_spec);
> + pa_sink_set_max_rewind(u->sink, nbytes);
> + pa_sink_set_max_request(u->sink, nbytes);
> + pa_sink_set_latency_range(u->sink, 0, BLOCK_USEC); */
Please don't include commented out code in patches.
> +
> +
> + if (!(u->thread = pa_thread_new("tunnelstream-sink", thread_func, u))) {
"tunnel-sink" would be a better name.
> + pa_log("Failed to create thread.");
> + goto fail;
> + }
> +
> + pa_sink_put(u->sink);
> + pa_modargs_free(ma);
> +
> + return 0;
> +
> +fail:
> + if (ma)
> + pa_modargs_free(ma);
> +
> + if (proplist)
> + pa_proplist_free(proplist);
> +
> + pa__done(m);
> +
> + return -1;
> +}
> +
> +void pa__done(pa_module*m) {
> + struct userdata *u;
> +
> + pa_assert(m);
> +
> + if (!(u = m->userdata))
> + return;
> +
> + if (u->sink)
> + pa_sink_unlink(u->sink);
> +
> + if (u->thread) {
> + pa_asyncmsgq_send(u->thread_mq.inq, NULL, PA_MESSAGE_SHUTDOWN, NULL, 0, NULL);
> + pa_thread_free(u->thread);
> + }
> +
> + pa_thread_mq_done(&u->thread_mq);
> +
> + if(u->remote_server)
> + free((void *) u->remote_server);
After changing the allocation function to pa_xstrdup(), the freeing
function needs to be changed to pa_xfree() too.
> +
> +
Extra space.
> + if (u->rtpoll)
> + pa_rtpoll_free(u->rtpoll);
> +
> + if (u->memchunk.memblock)
> + pa_memblock_unref(u->memchunk.memblock);
> +
> + if (u->sink)
> + pa_sink_unref(u->sink);
> +
> + pa_xfree(u);
> +}
I think it would be a good idea to mention u->context and u->stream in
pa__done(), like this:
/* The context and the stream are created and freed in the IO
* thread, so after the thread has been shut down, u->context and
* u->stream should always be NULL. */
pa_assert(!u->context);
pa_assert(!u->stream);
--
Tanu
More information about the pulseaudio-discuss
mailing list