On Tue, Jan 13, 2009 at 10:24 PM, Max Kellermann <m...@duempel.org> wrote:
>
> 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).
Great :)
About CamelCase... do you have any code formatting standards?
May be in your wiki... I didn't found them, so of course
I was trying to copypaste existing functions names and variables.
> 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?
Ok, I'll try to use more complex descriptions.
> I can't find documentation about the protocol you were implementing.
> Looks like you designed a brand new protocol for MPD.
First of all I'd like to implement some stable protocol, and only then
I'll write words about how it works.
> 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.
Yes, if it possible, I'd like to use git.musicpd.org repository
to develop my plugin, so there would be some more pushes before
final submit.
> Some comments about implementation details:
>
> > bool udpserver_kill;
>
> That should be volatile, otherwise gcc may play optimization tricks
> with you.
I've heard about that and of course forgot it :)
> > 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.
My mistake!
> > 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 use MSG_DONTWAIT, about what kind of blocks are you talking about?
May be about very slow sendto()?
So here I will have to use some local buf.data and lock mutex
while memcpy() from ufd->buf.data to local buf.data... is it good at all? :)
I've never saw this before.
> > * 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.
Sorry, but how should I cancel UDP server thread in this case?
For example when the main program terminates...
In pthreads API there was a cancel function,
but in glibc I didn't found it.
> > 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?
I think that error log messages should be somewhere far away from the code flow.
I just forgot to make my functions inlined.
> Why are you using getaddrinfo(), anyway? Looks like a complicated way
> to obtain INADDR_ANY.
I thought that getaddrinfo() is a new-way-main-stream-cool-function
and everybody should use it instead of anything else. This is not true?
> 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.
Why only one client? It works with more than one client :)
(I'm not talking about millions, but more than one)
Yes we can lose some chunks, and if a client will be impatient,
it will receive same chunks more than once. So what?
This is not an audio-streaming server, if you want an audio stream
you can use shout plugin. In visualization tasks I don't worry about
missed chunks,
I will lose some, but who will see it? And don't forgot about UDP lacks at all.
> Also your protocol isn't byte order safe.
Yes, it is true, I will fix it.
> 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.
Looks like you want to create and audio-udp-stream integral output! :)
Any way it is an interesting idea to use new command,
but firstly I'd like to finish what I've started... and then to look round :)
I feel nothing bad with missed chunks,
but life without threads and polls looks cool! :)
> Your code looks very interesting altogether! Doesn't solve any of my
> own problems, but it's a nice hack ;)
Thanks :)
Vasily
------------------------------------------------------------------------------
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
