volatile provides absolutely no guarantee thread-safety in SMP
environments. volatile was designed to access memory locations
in peripheral hardware directly; not for SMP. If volatile is
needed to work properly on SMP, then it is only hiding subtle
bugs.
volatile only prevents the /compiler/ from making optimizations
when accessing variables. CPUs do their own optimizations at
runtime so it cannot guarantee registers of CPUs are flushed
to memory cache-coherent access on different CPUs.
Furthermore, the thread-communication via condition variables
between threads sharing audio formats already results in memory
barriers.
The tag pool is a shared global resource that is infrequently
modified. However, it can occasionally be modified by several
threads, especially by the metadata_pipe for streaming metadata
(both reading/writing).
The bulk tag_item pool is NOT locked as currently only the
update thread uses it.
Trying to read or remember
"tag->numOfItems * sizeof(*tag->items)"
requires too much thinking and mental effort on my part.
Also, favor "sizeof(struct mpd_tag)" over "sizeof(*tag->items)"
because the former is easier to read and follow, even though
the latter is easier to modify if the items member changes
to a different type.
The previous patch enabled these warnings. In Eric's branch, they
were worked around with a generic deconst_ptr() function. There are
several places where we can add "const" to pointers, and in others,
libraries want non-const strings. In the latter, convert string
literals to "static char[]" variables - this takes the same space, and
seems safer than deconsting a string literal.
All callers of fdprintf() have been converted to client_printf() or
fprintf(); it is time to remove this clumsy hack now. We can also
remove client_print() which took a file descriptor as parameter.
Now that we have removed all invocations of client_get_fd(), we can
safely remove this transitional function. All access to the file
descriptor is now hidden behind the interface declared in client.h.
The shared code in showPlaylist() isn't worth it, because we aim to
remove fdprintf(). Duplicate this small function, and enable stdio
buffering for saved playlists.
The function loadPlaylist() wants to report incremental errors to the
client, for this reason we cannot remove its protocol dependency right
now. Instead, make it use the client struct instead of the raw file
descriptor.
Don't pass the raw file descriptor around. This migration patch is
rather large, because all of the sources have inter dependencies - we
have to change all of them at the same time.
Pass the client struct to CommandHandlerFunction and
CommandListHandlerFunction. Most commands cannot take real advantage
of that yet, since most of them still work with the raw file
descriptor.
These two functions take a client struct instead of the file
descriptor. We will now begin passing the client struct around
instead of a raw file descriptor (which needed a linear lookup in the
client list to be useful).
This patch continues the work of the previous patch: don't pass a file
descriptor at all to traverseAllIn(). Since this fd was only used to
report "directory not found" errors, we can easily move that check to
the caller. This is a great relief, since it removes the dependency
on a client connection from a lot of enumeration functions.
Database traversal should be generic, and not bound to a client
connection. This is the first step: no file descriptor for the
callback functions forEachSong() and forEachDir(). If a callback
needs the file descriptor, it has to be passed in the void*data
pointer somehow; some callbacks might need a new struct for passing
more than one parameter. This might look a bit cumbersome right now,
but our goal is to have a clean API.
Continuing the effort of removing protocol specific calls from the
core libraries: let the command.c code call commandError() based on
PlaylistInfo's return value.
The playlist library shouldn't talk to the client if possible.
Introduce the "enum playlist_result" type which the caller
(i.e. command.c) may use to generate an error message.
Client's input values should be validated by the command
implementation, and the core libraries shouldn't talk to the client
directly if possible. Thus, setPlaylistRepeatStatus() and
setPlaylistRandomStatus() don't get the file descriptor, and cannot
fail (return void).
The function valid_playlist_name() checks the name, but it insists on
reporting an eventual error to the client. The new function
is_valid_playlist_name() is more generic: it just returns a boolean,
and does not care what the caller will use it for. The old function
valid_playlist_name() will be removed later.
Currently, when the tag cache is being serialized to hard disk, the
stdio buffer is flushed before every song, because tag_print.c
performs unbuffered writes on the raw file descriptor. Unfortunately,
the fdprintf() API allows buffered I/O only for a client connection by
looking up the client pointer owning the file descriptor - for stdio,
this is not possible. To re-enable proper stdio buffering, we have to
duplicate the tag_print.c code without fprintf() instead of our custom
fdprintf() hack. Add this duplicated code to tag_save.c.
Move everything which dumps song information (via tag_print.c) to a
separate source file. song_print.c gets code which writes song data
to the client; song_save.c is responsible for serializing songs from
the tag cache.
Based on client_puts(), client_printf() is the successor of
fdprintf(). As soon as all fdprintf() callers have been rewritten to
use client_printf(), we can remove fdprintf().
client_write() writes a buffer to the client and buffers it if
required. client_puts() does the same for a C string. The next patch
will add more tools which will replace fdprintf() later.
clearMpdTag could be called on a tag that was still in a
tag_begin_add transaction before tag_end_add is called. This
was causing free() to attempt to operate on bulk.items; which is
un-free()-able. Now instead we unmark the bulk.busy to avoid
committing the tags to the heap only to be immediately freed.
Additionally, we need to remember to call tag_end_add() when
a song is updated before we NULL song->tag to avoid tripping
an assertion the next time tag_begin_add() is called.
Since client->fd==-1 has become our "expired" flag, it may already be
-1 when client_close() is called. Don't assert that it is still
non-negative, and call client_set_expired() instead.
During the tag library refactoring, the shout plugin was disabled, and
I forgot about adapting it to the new API. Apply the same fixes to
the oggflac decoder plugin.
While parsing the tag cache, don't allocate the directory name from
the heap, but copy it into a buffer on the stack. This reduces heap
fragmentation by 1%.
If many tag_items are added at once while the tag cache is being
loaded, manage these items in a static fixed list, instead of
reallocating the list with every newly created item. This reduces
heap fragmentation.
Massif results again:
mk before: total 12,837,632; useful 10,626,383; extra 2,211,249
mk now: total 12,736,720; useful 10,626,383; extra 2,110,337
The "useful" value is the same since this patch only changes the way
we allocate the same amount of memory, but heap fragmentation was
reduced by 5%.
Try to detect if the string needs Latin1-UTF8 conversion, or
whitespace cleanup. If not, we don't need to allocate temporary
memory, leading to decreased heap fragmentation.
At several places, we create temporary copies of non-null-terminated
strings, just to use them in functions like validUtf8String(). We can
save this temporary allocation and avoid heap fragmentation if we
add a length parameter instead of expecting a null-terminated string.
Since the inline function cannot modify its caller's variables (which
is a good thing for code readability), the new string pointer is the
return value. The resulting binary should be the same as with the
macro.
The new source tag_pool.c manages a pool of reference counted tag_item
objects. This is used to merge tag items of the same type and value,
saving lots of memory. Formerly, only the value itself was pooled,
wasting memory for all the pointers and tag_item structs.
The following results were measured with massif. Started MPD on
amd64, typed "mpc", no song being played. My music database contains
35k tagged songs. The results are what massif reports as "peak".
0.13.2: total 14,131,392; useful 11,408,972; extra 2,722,420
eric: total 18,370,696; useful 15,648,182; extra 2,722,514
mk f34f694: total 15,833,952; useful 13,111,470; extra 2,722,482
mk now: total 12,837,632; useful 10,626,383; extra 2,211,249
This patch set saves 20% memory, and does a good job in reducing heap
fragmentation.
The value is stored in the same memory allocation as the tag_item
struct; this saves memory because we do not store the value pointer
anymore. Also remove the getTagItemString()/removeTagItemString()
dummies.
This patch makes MPD consume much more memory because string pooling
is disabled, but it prepares the next bunch of patches. Replace the
code in tagTracker.c with naive algorithms without the tree code. For
now, this should do; later we should find better algorithms,
especially for getNumberOfTagItems(), which has become wasteful with
temporary memory.
Unfortunately, the C standard postulates that the argument to free()
must be non-const. This does not makes sense, and virtually prevents
every pointer which must be freed at some time to be non-const. Use
the deconst hack (sorry for that) to allow us to free constant
pointers.
Instead of passing the pointer to the "expired" flag to
processListOfCommands(), this function should use the client API to
check this flag. We can now remove the "global_expired" hack
introduced recently.
Start exporting the client struct as an opaque struct. For now, pass
it only to processCommand() and processListOfCommands(), and provide a
function to extract the socket handle. Later, we will propagate the
pointer to all command implementations, and of course to
client_print() etc.
The old code tried to write a response to the client, without even
checking if it was already closed. Now that we have added more
assertions, these may fail... perform the "expired" check earlier.
Patch bdeb8e14 ("client: moved "expired" accesses into inline
function") was created under the wrong assumption that
processListOfCommands() could modify the expired flag, which is not
the case. Although "expired" is a non-const pointer,
processListOfCommands() just reads it, using it as the break condition
in a "while" loop. I will address this issue with a better overall
solution, but for now provide a pointer to a global "expired" flag.
client_defer_output() was modified so that it can create the
deferred_send list. With this patch, the assertion on
"deferred_send!=NULL" has become invalid. Remove it.
Previously, when select() failed, we assumed that there was an invalid
file descriptor in one of the client structs. Thus we tried select()
one by one. This is bogus, because we should never have invalid file
descriptors. Remove it, and make select() errors fatal.
All of the client's resources are freed in client_close(). It is
enough to set the "expired" flag, no need to duplicate lots of
destruction code again and again.
Due to the large buffers in the client struct, the static client array
eats several megabytes of RAM with a maximum of only 10 clients. Stop
this waste and allocate each client struct from the heap.
Second patch: rename the internal struct name. We will eventually
export this type as an opaque forward-declared struct later, so we
can pass a struct pointer instead of a file descriptor, which would
save us an expensive linear lookup.
I don't believe "interface" is a good name for something like
"connection by a client to MPD", let's call it "client". This is the
first patch in the series which changes the name, beginning with the
file name.
linux/list.h is a nice doubly linked list library - it is lightweight
and powerful at the same time. It will be useful later, when we begin
to allocate client structures dynamically. Import it, and strip out
all the stuff which we are not going to use.
It should be obvious in which thread or context a function is being
executed at runtime. The code which was left in decode.c is for the
decoder thread itself; give the file a better name.
This releases several include file dependencies. As a side effect,
"CHUNK_SIZE" isn't defined by decoder_api.h anymore, so we have to
define it directly in the plugins which need it. It just isn't worth
it to add it to the decoder plugin API.
The decoder plugins need this type, so export it in the public API.
This allows is to remove "decode.h" from "decoder_api.h", uncluttering
the API namespace some more.
Unfortunately, we have to pass the DecoderControl pointer to these
inline functions, because the global variable "dc" may not be
available here. This will be fixed later.
The decoder thread is responsible for resetting dc->command after a
command was executed. As a consequence, we can assume that
dc->command is already NONE after decoder_stop().
There is no unlocked caller of clearPlayerQueue(), and the functions
lockPlaylistInteraction() and unlockPlaylistInteraction() are trivial
- merge them.
Since playerPlay() already calls playerStop(), we can remove its
invocation of playerStop() from playPlaylistOrderNumber().
We can also make playerStop a static function.
All (indirect) callers of queueSong() ensure that the queue state is
BLANK, so there is no need to check it in queueSong() again. As a
side effect, queueSong() cannot fail anymore, and can return void.
Also, playlist_queueError and all its error handling can go away.
playerKill() was marked as deprecated, but it seems like a good idea
to do proper cleanup in all threads (e.g. for usable valgrind
results). Introduce the command "EXIT" which makes the player thread
exit cleanly.
playerWait() stops the player thread (twice!) and closes the output
device. It should be well enough to just send CLOSE_AUDIO, without
STOP.
This requires a tiny change to the player thread code: make it break
when CLOSE_AUDIO is sent.
It was possible for the decoder thread to go into an endless loop
(flac and oggflac decoders): when a "STOP" command arrived, the Read()
callback would return 0, but the EOF() callback returned false. Fix:
when decoder_get_command()!=NONE, return EOF==true.
Storing local configuration in global (static) variables is obviously
a bad idea. Move all those variables into the JackData struct,
including the locks.
There is only one caller of freeJackData() left: jack_finishDriver().
This function is called by the mpd core, and is called exactly once
for every successful jack_initDriver(). We do not need to clear
audioOutput->data, since this variable is invalidated anyway.
Over the lifetime of the jack AudioOutput object, we want a single
valid JackData object, so we can persistently store data there
(configuration etc.). Allocate JackData in jack_initDriver(). After
that, we can safely remove all audioOutput->data==NULL checks (and
replace them with assertions).
No need to destroy the JackData object when an error occurs, since
jack_finishDriver() already frees it. Only deinitialize the jack
library, introduce freeJackClient() for that, and move code from
freeJackData().
Prepare the next patch: make the "!jd" check independent of the
jd->client initialization. This way we can change the "jd"
initialization semantics later.
connect_jack() invokes freeJackData() in every error handler, although
its caller also invokes this function after a failure. We can save a
lot of lines in connect_jack() by removing these redundant
freeJackData() invocations.
When we introduced decoder_read(), we added code which aborts the read
operation when a decoder command arrives. Several plugins however did
not expect that when they were converted to decoder_read(). Add
proper checks to the mp3 and flac decoder plugins.
The code said "decoder_command==STOP" because that was a conversion
from the old "dc->stop" test. As we can now check for all commands in
one test, we can simply rewrite that to decoder_command!=NONE.
This flag is used internally; it is set by decoder_seek_where(), and
indicates that the decoder plugin has begun the seek process. It is
used for the case that the decoder plugin has to read data during the
seek process. Before this patch, that was impossible, because
decoder_read() would refuse to read data unless dc->command is NONE.
This patch is kind of a dirty workaround, and needs to be redesigned
later.
The old code called can_seek() with the uninitialized pointer
"isp.is". Has this ever worked? Anyway, initialize "isp" first, then
call can_seek(&isp).
Move everything related to finding and initializing the WVC stream to
wavpack_open_wvc(). This greatly simplifies its error handling and
the function wavpack_streamdecode().
On our way to stabilize the decoder API, we will one day remove the
input stream functions. The most basic function, read() will be
provided by decoder_api.h with this patch. It already contains a loop
(still with manual polling), error/eof handling and decoder command
checks. This kind of code used to be duplicated in all decoder
plugins.
If the input stream is not seekable, the try_decode() function
consumes valuable data, which is not available to the decode()
function anymore. This means that the decode() function does not
parse the header correctly. Better skip the detection if we cannot
seek. Or implement better buffering, something like unread() or
buffered rewind().
The return value of audio_linear_dither() is always casted to
mpd_sint16. Returning long does not make sense, and consumed 8 bytes
on a 64 bit platform.
The output buffer is always flushed after being appended to, which
allows us to assume it is always empty. Always start writing at
outputBuffer, don't remember outputPtr.
Fill the whole output buffer at a time by using dither_buffer()'s
ability to decode blocks. Calculate how many samples fit into the
output buffer before each invocation.
Simplifying loops for performance: why check dropSamplesAtEnd in every
iteration, when we could modify the loop boundary? The (writable)
variable samplesLeft can be eliminated; add a write-once variable
pcm_length instead, which is used for the loop condition.
The variable samplesPerFrame is used only in one single closure. Make
it local to this closure. The compiler will probably convert it to a
register anyway.
Preparing for simplifying and thus speeding up the dithering code:
moved dithering to a separate function which contains a trivial loop.
With this patch, only one sample is dithered at a time, but the
following patches will allow us to dither a whole block at a time,
without complicated buffer length checks.
Copy some code from aac_decode() to aac_stream_decode() and apply
necessary changes to allow streaming audio data. Both functions might
be merged later.
initAacBuffer() should really only initialize the buffer; currently,
it also reads data from the input stream and parses the header. All
of the AAC buffer code should probably be moved to a separate library
anyway.
Shifting from the buffer queue is a common operation, and should be
provided as a separate function. Move code to aac_buffer_shift() and
add a bunch of assertions.
When checking for EOF, we should not check whether the read request
has been fully satisified. The InputStream API does not guarantee
that readFromInputStream() always fills the whole buffer, if EOF is
not reached. Since there is the function inputStreamAtEOF() dedicated
for this purpose, we should use it for EOF checking after
readFromInputStream()==0.
Fill the AacBuffer even when nothing has been consumed yet. The
function should not check for consumed data, but for free space at the
end of the buffer.
The flag "ready" indicates whether the input stream is ready and it
has parsed all meta data. Previously, it was impossible for
decodeStart() to see the content type of HTTP input streams, because
at that time, the HTTP response wasn't parsed yet.
With the functions decoder_plugin_register() and
decoder_plugin_unregister(), decoder plugins can register a
"secondary" plugin, like the flac input plugin does this for
"oggflac".
"decoder plugin" is a better name than "input plugin", since the
plugin does not actually do the input - InputStream does. Also don't
use typedef, so we can forward-declare it if required.
PlayerControl.command replaces the old attributes play, stop, pause,
closeAudio, lockQueue, unlockQueue, seek. The main thread waits for
each command synchronously, so there can only be one command enabled
at a time anyway.
The wavpack decoder plugin implements a hack, and it needs the song
URL for that. This API (and the hack) should be revised later, but
add that function for now.
Since we want to hide mpd internals from the decoder plugins, the
plugins should not check dc->state whether they have already called
decoder_initialized(). Use a local variable to track that.
