On Wed, 14 Aug 2013 14:42:16 +0300
Tanu Kaskinen <tanu.kaskinen at linux.intel.com> wrote:
> On Wed, 2013-08-07 at 18:05 +0200, Alexander Couzens wrote:
> > Signed-off-by: Alexander Couzens <lynxis at fe80.eu>
> > ---
>
> The whole commit message, apart from the signed-off-by line, is now all
> on one line. The first line is supposed to be a very short summary of
> the changes, suitable for e.g. email subject line. Further explanation
> should go in a separate paragraph.
sorry. this was not my intention.
>
> > +
> > +/* libpulse callbacks */
> > +static void stream_state_callback(pa_stream *stream, void *userdata);
>
> Cosmetic: I'd prefer _cb suffix for all callbacks.
is fixed.
>
> > +static void context_state_callback(pa_context *c, void *userdata);
> > +static void sink_update_requested_latency_cb(pa_sink *s);
> > +
> > +struct userdata {
> > + pa_module *module;
> > + pa_sink *sink;
> > + pa_thread *thread;
> > + pa_thread_mq thread_mq;
> > + pa_mainloop *thread_mainloop;
> > + pa_mainloop_api *thread_mainloop_api;
> > +
> > + /* libpulse context */
>
> In the previous review I said that this comment doesn't have any useful
> information. I meant to imply that the comment should be removed.
removed.
> > + pa_context *context;
> > + pa_stream *stream;
> > +
> > + pa_buffer_attr bufferattr;
> > +
> > + bool connected;
> > +
> > + char *remote_server;
> > + char *remote_sink_name;
> > +};
> > +
> > +static const char* const valid_modargs[] = {
> > + "sink_name",
> > + "sink_properties",
> > + "server",
> > + "sink",
> > + "format",
> > + "channels",
> > + "rate",
> > + "channel_map",
> > + "cookie", /* unimplemented */
> > + "reconnect", /* reconnect if server comes back again - unimplemented*/
>
> "cookie" and "reconnect" aren't included in the usage string. I guess
> the reason is because the features haven't been implemented, but I think
> it would be good to keep PA_MODULE_USAGE and valid_modargs in sync,
> because when you implement the cookie support, for example, it's very
> easy to forget to update PA_MODULE_USAGE.
ok. I'll extend the comment.
>
> What's your plan for implementing "reconnect"? I argued in my previous
> review that module-tunnel-sink shouldn't support reconnecting at all,
> and should instead leave that job for module-zeroconf-discover.
imagine you have a network with 10 small boxes (let's say raspberry) with usbsound devices.
some of them disappear from time to time, because somebody cut their power off ;). they are so small, that avahi does not fit on it.
today there is a crontab which runs every minute and loads a lot of mod-tunnel module with their specific ips.
for that i would like to have a opt-in feature.
> > + proplist = tunnel_new_proplist(u);
> > + /* init libpulse */
> > + u->context = pa_context_new_with_proplist(pa_mainloop_get_api(u->thread_mainloop),
>
> Nitpick: using u->thread_mainloop_api instead of pa_mainloop_get_api()
> would be slightly simpler/shorter.
But you can not change the mainloop implementation sometime.
>
> > + "PulseAudio",
> > + proplist);
> > + pa_proplist_free(proplist);
> > +
> > + if (!u->context) {
> > + 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,
>
> As I explained in the previous review, the NOFAIL flag serves no purpose
> here.
You did it. I didn't removed it, because I thought first fix the docu and
remove it afterwards.
> > + if (u->connected &&
> > + PA_STREAM_IS_GOOD(pa_stream_get_state(u->stream)) &&
> > + PA_SINK_IS_LINKED(u->sink->thread_info.state)) {
> > + /* TODO: use IS_RUNNING + cork stream */
>
> What do you mean by this comment?
Implement corking instead of sending silence
>
> > +
> > + if (pa_stream_is_corked(u->stream)) {
> > + pa_stream_cork(u->stream, 0, NULL, NULL);
>
> I already pointed out in the previous review that you need to unref the
> pa_operation object that this function returns.
This doesn't work, because it doesn't return an operation and if I'm doing it crashed because of
dereferencing a nullpointer.
>
> > + } else {
> > + writable = pa_stream_writable_size(u->stream);
> > + if (writable > 0) {
> > + if (memchunk.length <= 0)
>
> I already pointed out in the previous review that the length is always
> zero here. That means that checking the value is pointless.
>
> > + pa_sink_render(u->sink, writable, &memchunk);
>
> I think pa_sink_render_full() would be better. It's nicer to send one
> big packet over the network than several smaller ones (or that's at
> least my gut feeling)
ok.
> > +
> > + pa_assert(memchunk.length > 0);
> > +
> > + /* we have new data to write */
> > + p = (const uint8_t *) pa_memblock_acquire(memchunk.memblock);
> > + /* TODO: ZERO COPY! */
>
> How do you plan to achieve zero copy? If the plan is to use
> pa_stream_write_begin(), that won't result in zero copy, because the
> data needs to be copied to the buffer that pa_stream_write_begin() gives
> (but it's still a good idea to use pa_stream_write_begin()).
I thought that would be the solution because we can render direct in the buffer from libpulse.
> > + ret = pa_stream_write(u->stream,
> > + ((uint8_t*) p + memchunk.index),
> > + memchunk.length,
> > + NULL, /**< A cleanup routine for the data or NULL to request an internal copy */
> > + 0, /** offset */
> > + PA_SEEK_RELATIVE
> > + );
>
> The indentation is still off by two spaces here.
>
> > + pa_memblock_release(memchunk.memblock);
> > + pa_memblock_unref(memchunk.memblock);
> > + pa_memchunk_reset(&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);
>
> I already answered this question in the previous review: "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 I quit here if ret != 0.
> > + }
> > + }
> > + }
> > + }
> > + }
> > +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?
>
> I already answered this question in the previous review. No, it's not a
> race condition. This code will work just fine if PA_MESSAGE_SHUTDOWN has
> been sent. If PA_MESSAGE_SHUTDOWN has been sent, it means that
> pa__done() is being run from pa_module_unload(), and in that case the
> PA_CORE_MESSAGE_UNLOAD_MODULE handler won't do anything, because the
> module was already removed from core->modules before pa_module_unload()
> was called.
forgot to remove this coment.
> > +
> > +static void stream_state_callback(pa_stream *stream, void *userdata) {
> > + struct userdata *u = userdata;
> > +
> > + pa_assert(u);
> > +
> > + switch (pa_stream_get_state(stream)) {
> > + case PA_STREAM_FAILED:
> > + pa_log_error("Stream failed.");
> > + u->connected = false;
> > + u->thread_mainloop_api->quit(u->thread_mainloop_api, TUNNEL_THREAD_FAILED_MAINLOOP);
> > + break;
> > + case PA_STREAM_TERMINATED:
> > + pa_log_debug("Stream terminated.");
> > + break;
> > + default:
> > + break;
> > + }
>
> When the stream becomes ready, you should check what the actual tlength
> is, and set the sink latency accordingly. Also, if the server changes
> the tlength at runtime, the sink latency needs to be updated also in
> that situation. You can use pa_stream_set_buffer_attr_callback() for
> getting notifications about changes in the buffer attributes.
> > +}
> > +
> > +static void context_state_callback(pa_context *c, void *userdata) {
> > + struct userdata *u = userdata;
> > + int c_errno;
> > +
> > + pa_assert(u);
> > +
> > + switch (pa_context_get_state(c)) {
> > + case PA_CONTEXT_UNCONNECTED:
> > + case PA_CONTEXT_CONNECTING:
> > + case PA_CONTEXT_AUTHORIZING:
> > + case PA_CONTEXT_SETTING_NAME:
> > + break;
> > + case PA_CONTEXT_READY: {
> > + pa_proplist *proplist;
> > + const char *username = pa_get_user_name_malloc();
> > + const char *hostname = pa_get_host_name_malloc();
> > + /* TODO: old tunnel say 'Null-Output' */
>
> What do you mean by this comment?
That the title/description is fetched from the remote side. I have to update this after the stream is ready.
> > + char *stream_name = pa_sprintf_malloc("%s for %s@%s", "Tunnel", username, hostname);
>
> Why is "Tunnel" inserted to the string via substitution instead of
> directly?
>
> Also, I think the string should be translatable.
Because I should fix
> > +
> > + pa_log_debug("Connection successful. Creating stream.");
> > + pa_assert(!u->stream);
> > +
> > + proplist = tunnel_new_proplist(u);
> > + pa_proplist_sets(proplist, PA_PROP_MEDIA_ROLE, "sound");
>
> "sound" is not a standard media role, and therefore it's not useful.
> It's probably best to not set any role, because we don't know what the
> tunnel will be used for. (The old tunnel module sets role "abstract",
> but I don't think that's really useful.)
but abstract wasn't documented. Should I update the documentation?
>
> > + pa_assert(proplist);
>
> If you want to have this assertion, it should be before
> pa_proplist_sets().
I moved it into tunnel_new_proplist
> > +
> > + u->stream = pa_stream_new_with_proplist(u->context,
> > + stream_name,
> > + &u->sink->sample_spec,
> > + &u->sink->channel_map,
> > + proplist);
> > + pa_proplist_free(proplist);
> > + pa_xfree(stream_name);
> > +
> > + if(!u->stream) {
>
> Missing space after "if".
>
> > + pa_log_error("Could not create a stream.");
> > + u->thread_mainloop_api->quit(u->thread_mainloop_api, TUNNEL_THREAD_FAILED_MAINLOOP);
> > + return;
> > + }
> > +
> > +
>
> Extra empty line.
>
> > + pa_context_subscribe(u->context, PA_SUBSCRIPTION_MASK_SINK_INPUT, NULL, NULL);
>
> Subscribing without providing a callback is pointless. Also, you don't
> unref the pa_operation object that the function returns.
oops :).
> > +
> > + pa_stream_set_state_callback(u->stream, stream_state_callback, userdata);
> > + if (pa_stream_connect_playback(u->stream,
> > + u->remote_sink_name,
> > + &u->bufferattr,
> > + PA_STREAM_START_CORKED | PA_STREAM_AUTO_TIMING_UPDATE,
>
> I asked you to add DONT_MOVE to the flags.
>
> Also, I now realized that PA_STREAM_INTERPOLATE_TIMING should be added
> to the flags too.
Why do I really need DONT_MOVE? I like to move the streams on the remove side.
> > + NULL,
> > + NULL) < 0) {
> > + /* TODO fail */
>
> Why is this not implemented? It should be a simple case of logging an
> error and calling quit() on the mainloop.
fixed
> > + }
> > + u->connected = true;
> > + break;
> > + }
> > + case PA_CONTEXT_FAILED:
> > + c_errno = pa_context_errno(u->context);
> > + pa_log_debug("Context failed with err %d.", c_errno);
>
> pa_strerror() is more informative than the plain error code.
>
> > + u->connected = false;
> > + u->thread_mainloop_api->quit(u->thread_mainloop_api, TUNNEL_THREAD_FAILED_MAINLOOP);
>
> There could be a "quit" section, so that you could write just "goto
> quit;" instead of setting connected = false and calling quit() multiple
> times in this function.
I'm not sure if I really need connected anymore.
Will be refactored.
> > +
> > + nbytes = pa_usec_to_bytes(block_usec, &s->sample_spec);
> > + pa_sink_set_max_rewind_within_thread(s, nbytes);
>
> max_rewind should be 0, since you don't support rewinding.
ok.
> > + pa_sink_set_max_request_within_thread(s, nbytes);
> > +
> > + if (block_usec != (pa_usec_t) -1) {
> > + u->bufferattr.tlength = nbytes;
>
> block_usec is never -1, because if
> pa_sink_get_requested_latency_within_thread() returns -1, then you set
> block_usec to s->thread_info.max_latency.
>
> To be consistent with the latency all the time, I think you should set
> the initial tlength to s->thread_info.max_latency too, instead of
> initializing it to -1 like you do now.
> > + }
>
> Unnecessary braces.
>
> > +
> > + if (u->stream && PA_STREAM_IS_GOOD(pa_stream_get_state(u->stream))) {
> > + pa_stream_set_buffer_attr(u->stream, &u->bufferattr, NULL, NULL);
>
> You don't unref the pa_operation object that the function returns.
>
> It's a bit hairy issue what to do if the stream is not yet ready when
> the sink requested latency changes. You use PA_STREAM_IS_GOOD() here,
> but it seems that pa_stream_set_buffer_attr() fails if the state is not
> READY. PA_STREAM_IS_GOOD(), on the other hand, returns true also if the
> state is CREATING. So what to do? I think it would best to postpone the
> pa_stream_set_buffer_attr() call until the stream becomes ready. When
> the stream becomes ready, check the requested latency, and if it's
> something else than max_latency or the tlength that the server assigned
> to the stream, call pa_stream_set_buffer_attr().
>
> > + }
>
> Unnecessary braces.
>
> > +}
> > +
> > +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) {
> > + *((pa_usec_t*) data) = 0;
> > + return 0;
> > + }
> > +
> > + if (!PA_STREAM_IS_GOOD(pa_stream_get_state(u->stream))) {
>
> You should check for PA_STREAM_READY instead of PA_STREAM_IS_GOOD, or
> alternatively not have have this check at all and let
> pa_stream_get_latency() return an error when the stream state is
> something else than READY.
>
> > + *((pa_usec_t*) data) = 0;
> > + return 0;
> > + }
> > +
> > + if (pa_stream_get_latency(u->stream, &remote_latency, &negative) < 0) {
> > + *((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 ? */
>
> I already answered this question in the previous review. You don't need
> to add more latency.
removed
> > + return 0;
> > + }
> > + }
> > + return pa_sink_process_msg(o, code, data, offset, chunk);
> > +}
> > +
> > +int pa__init(pa_module *m) {
> > + struct userdata *u = NULL;
> > + pa_modargs *ma = NULL;
> > + pa_sink_new_data sink_data;
> > + pa_sample_spec ss;
> > + pa_channel_map map;
> > + const char *remote_server = NULL;
> > + const char *sink_name = NULL;
> > + char *default_sink_name = 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) {
> > + pa_log("Invalid sample format specification or channel map");
> > + goto fail;
> > + }
> > +
> > + remote_server = pa_modargs_get_value(ma, "server", NULL);
> > + if (!remote_server) {
> > + pa_log("No server given!");
> > + goto fail;
> > + }
> > +
> > + u = pa_xnew0(struct userdata, 1);
> > + u->module = m;
> > + m->userdata = u;
> > + u->remote_server = pa_xstrdup(remote_server);
> > + u->thread_mainloop = pa_mainloop_new();
> > + if (u->thread_mainloop == NULL) {
> > + pa_log("Failed to create mainloop");
> > + goto fail;
> > + }
> > + u->thread_mainloop_api = pa_mainloop_get_api(u->thread_mainloop);
> > +
> > + u->remote_sink_name = pa_xstrdup(pa_modargs_get_value(ma, "sink", NULL));
> > +
> > + u->bufferattr.maxlength = (uint32_t) -1;
> > + u->bufferattr.minreq = (uint32_t) -1;
> > + u->bufferattr.prebuf = (uint32_t) -1;
> > + u->bufferattr.tlength = (uint32_t) -1;
> > +
> > + pa_thread_mq_init_thread_mainloop(&u->thread_mq, m->core->mainloop, pa_mainloop_get_api(u->thread_mainloop));
> > +
> > + /* Create sink */
> > + pa_sink_new_data_init(&sink_data);
> > + sink_data.driver = __FILE__;
> > + sink_data.module = m;
> > +
> > + default_sink_name = pa_sprintf_malloc("tunnel-sink-new.%s", remote_server);
>
> I would prefer "tunnel-sink" as the prefix instead of "tunnel-sink-new".
> The reason is that the module name is supposed to be
> "module-tunnel-sink-new" only temporarily, until it's ready to replace
> the original module. It's easy to forget to change this string when
> doing the module renaming, and I don't think it's important to see from
> the sink name which version of the module is being used.
I can not load the old tunnel as long the new is loaded when both have the same name.
>
> > + sink_name = pa_modargs_get_value(ma, "sink_name", default_sink_name);
> > +
> > + pa_sink_new_data_set_name(&sink_data, sink_name);
> > + pa_sink_new_data_set_sample_spec(&sink_data, &ss);
> > + pa_sink_new_data_set_channel_map(&sink_data, &map);
> > +
> > + pa_proplist_sets(sink_data.proplist, PA_PROP_DEVICE_CLASS, "sound");
> > + pa_proplist_setf(sink_data.proplist,
> > + PA_PROP_DEVICE_DESCRIPTION,
> > + _("Tunnel to %s/%s"),
> > + remote_server,
> > + pa_strempty(u->remote_sink_name));
> > +
> > + 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 */
>
> What do you mean by this comment?
removed. I tested it already.
I will look over all pa_operations if there are some left which needs unrefed. Tomorrow evening I should fixed all your suggestions and send an update to the list.
Best,
Alex
--
Alexander Couzens
mail: lynxis at fe80.eu
sip/jabber: lynxis at c-base.org
mobile: +4915123277221
