On 2009/01/10 23:57, Vasily Stepanov <vasily.stepa...@gmail.com> wrote:
> Currently I'm working on MPD visualization stuff.
Hi Vasily,
here's a rough review of the "udpfeed" plugin (commit f0fd78a911d6).
First: I like your coding style, you're using inline functions a lot.
You're writing a lot of source code comments. However don't use
CamelCase for new code, we're trying to get rid of it (probably you
have copy'n'pasted the names from other output plugins).
I'd like to see better patch descriptions. "udpfeed_plugin.c added"
just doesn't seem enough - what kind of plugin is it? What does it
do?
I can't find documentation about the protocol you were implementing.
Looks like you designed a brand new protocol for MPD.
As long as your patches are not submitted, you may fold corrections
into the first patch. e.g. the GLib conversion patch may just be
folded into the first patch.
Some comments about implementation details:
> bool udpserver_kill;
That should be volatile, otherwise gcc may play optimization tricks
with you.
> size_t udpfeed_client_size = sizeof(udpfeed_client);
This variable is initialized only once, but used several times. This
breaks if a IPv4 client is first used, and then another client using
IPv6.
> g_mutex_lock(ufd->playChunkMutex);
[...]
> if (sendto(ufd->sock, ufd->buf.data,
> ufd->buf.offset, MSG_DONTWAIT, &udpfeed_client, udpfeed_client_size) == -1) {
[...]
> g_mutex_unlock(ufd->playChunkMutex);
Don't call blocking functions while holding a mutex!
> * I will wait for incoming connection just one second
[...]
> * And if there is no incoming data I will check kill state
I don't like fixed sleep times. poll() for an event with unlimited
timeout, and let the kernel wake you up.
> new_addrinfo(const char *service, const struct addrinfo *hints, struct
> addrinfo **res)
> free_addrinfo(struct addrinfo **res)
> new_socket(int domain, int type, int protocol, int *sock)
> free_socket(int *sock)
Why so many wrapper functions?
Why are you using getaddrinfo(), anyway? Looks like a complicated way
to obtain INADDR_ANY.
I also don't like the protocol you designed. It allows only one
single client, and requires the client to request each chunk. If a
client doesn't send the "PCM" request quickly enough, chunks are lost.
Also your protocol isn't byte order safe.
My idea: introduce a new MPD command "udpfeed". A MPD client would
send:
"udpfeed 6543"
MPD would then send all PCM chunks to this client on port 6543 for,
say, 60 seconds, after which the client has to refresh the
subscription. Any number of clients can be subscribed at a time.
The same code could be used to send PCM data to a multicast
destination.
Another huge advantage: we could do that without threading and without
polling and without buffering!
Of course, we'd have to make sure that we get the byte order thing
right. We need to define (and document) a proper packet header with a
well-defined byte order.
Your code looks very interesting altogether! Doesn't solve any of my
own problems, but it's a nice hack ;)
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
