Buffering API Proposal

Overview

Currently, audio buffering and playback are interconnected, both being handled by playback.c. This has (IMHO) resulted in code which is becoming hard to maintain, with unclear responsibilities.

It would be useful to seperate these functions:

• It should provide a central API/service to handle runtime audio buffer memory management
• It should simplify the playback code and reduce the possibility of race conditions
• It should (ultimately) allow for the audio buffer to be used for any data that should be pre-buffered, e.g. video.

The following is a scratchpad for us to develop the idea of a buffering API that will control all access to the audio buffer.

Definitions

• Audio buffer: The "free" memory area available whilst Rockbox is running, after all fixed allocations have taken place. Despite the name, this can contain many kinds of data (and codecs), not just audio data.
• Buffering: The copying of data from the disk into the audio buffer.
• Handle: An integer that refers to a particular buffered file, not necessarily the file handle as used by standard file i/o.
• Playback: The copying of data from the audio buffer to the output (e.g. to PCM playback).
• Unbuffering: The deallocating of data from the audio buffer, no data is actually copied.

API Functions

The API is a simplified read-only file i/o API. It may be useful to think of it as a Rockbox-ed "SMARTDRV" that aims to pre-buffer all required data so that playback etc. can operate with minimal waiting for disk access.

bufopen()

int bufopen(const char *pathname, const long offset)

Purpose:Requests that a file should be buffered.

Inputs: pathname - Pointer to path to file that will need accessing. offset - offset from start of file to read from (included here so we don't always buffer from the start of the file when not needed)

Returns: handle, or < 0 to indicate failure

bufseek()

int bufseek(const int handle, const long offset)

Purpose: Sets the offset of the next byte to be read

Inputs: handle - handle as returned from bufopen offset - offset from start of file to read from

Returns: 0 to indicate success, or < 0 to indicate failure

bufread()

long bufread(const int handle, const long size, unsigned char *dest)

Purpose: Copies data to the caller, blocking as required

Inputs: handle - handle as returned from bufopen, size - maximum number of bytes to be copied, dest - pointer to buffer to return data

Returns: the number of bytes copied, 0 to mark EOF or < 0 to indicate failure

bufgetdata()

long bufgetdata(const int handle, const long size, unsigned char **destptr)

Purpose: Makes data available to the caller, blocking as required. No data is copied.

Inputs: handle - handle as returned from bufopen, size - minimum number of bytes to be made available, destptr - pointer to pointer to data

Returns: the number of bytes available (with *destptr updated to point to the data), 0 to mark EOF or < 0 to indicate failure Note that there is no guarantee how long that the data will remain valid.

bufclose()

int bufclose(const int handle)

Purpose: Indicates that access to a file is no longer required

Inputs: handle - handle as returned from bufopen

Returns: 0 to indicate success, or < 0 to indicate failure

Others?

It may be useful to incorporate the existing buffer_init() and buffer_alloc() functions into this API. Access to the global variables audiobufend and audiobuf should become read-only via this API (or, later, removed). There are some places (tagcache.c for example) that make temporary use of the audio buffer without using buffer_alloc(); these will need work. Of course, we are not providing a malloc()/free() model here!

Currently there are callback functions supplied by playback.c to indicate when a file is buffered or unbuffered. These are (currently) only used by tagtree.c to update run-time statistics. These should NOT be provided by the buffering API as they are actually related to playback, rather than buffering - some work may be required here.

Implementation

As well as the API, which may be called from various other threads (so may need to consider mutexing or very careful construction), there will be a a buffering thread that aims to make best use of the buffer to contain data for expected calls to bufread().

For simplicity, the initial implementation should be a straight port of the current playback scheme. This uses a ring buffer (including a guard buffer) and allows any file to be buffered in one piece only, with a fixed limit of the number of files that can be buffered. Currently the playback code will buffer up to 32 tracks so we must support at least 64 files (one audio plus one codec per track).

Note that this development is nothing to do with Metadata-On-Buffer, but it should make development of that rather simpler.

Anyone who'd like to help design the implementation is invited to add notes here.

JdGordons attempt.

The code is now at svn://jdgordon.mine.nu/mob if anyone wants to have a look, both mine and Nico's code is there. (email me (JdGordon)) if you want access to help out -- JonathanGordon - 02 Jul 2007

Discussion

Please shoot these proposals to bits, or at least point out weaknesses and suggest improvements.

The current code actually copies the data from the audio buffer into a given pointer instead of just setting data to a pointer in the audio buffer, this is so files can wrap around the end of the ring buffer (probably other reasons also). -- JonathanGordon - 14 May 2007

You mean in codec_request_buffer_callback()? We could have a conventional "long bufread(const int handle, const long size, unsigned char *dst)" but there may be cases when copying isn't required. -- SteveBavin - 15 May 2007

Wouldn't it be better to completly ignore the current implementation (except the codec interface for simplicity) and start the buffering/playback code from scratch using the new API? This way we dont have bugs appearing later on because of possible hacks which need to be added for it to work with the current code? -- JonathanGordon - 14 May 2007

The buffering/playback code will effectively be redone to implement this proposal; there should certainly be no need for "hacks" (unless this API is incorrect). -- SteveBavin - 15 May 2007

And on that note, I think we should even talk about how to make MoB work with this now so its not hacked on later. -- JonathanGordon - 14 May 2007

It would be useful to consider how this will work with MoB, but I see this as a very general (and dumb) service that handles disk reading regardless of the file content. For MoB I assume we would parse all metadata information directly from the buffer, so the "original file format" would be a sensible format to have in memory. -- SteveBavin - 15 May 2007

That said (and remembering we dont want malloc()) would a buf_tempalloc(size_t size) function be acceptable? Depends how its done; I'm scared adding this would cause nasty memory fragmentation... Unless we have this sort of thing for each track in memory (this only works if the whole track fits though, but could work as a base)... |codec (unless codec is in memory or buffer already)|MoB buffer|file buffer ....|...| this shouldn't fragment memory if once the track file is closed the codec and MoB are freed up. I guess the file buffer part would be overwritten if the file doesn't fit in the buffer, but it will need a better way of knowing when to free RAM than bufclose(). -- JonathanGordon - 14 May 2007

The memory layout should (initially, at least) be identical to the current playback code. A caution - don't think of this in terms of buffer allocation. For example, bufclose() doesn't need to "free" anything, and a particular file's data may be unbuffered before the corresponding bufclose() is called if the implementation decides there is other data which will be needed more urgently. -- SteveBavin - 15 May 2007

On the 24th May 2007 there was an interesting conversation on the IRC channel about this proposal and Metadata On Buffer : IRC log of the discussion -- NicolasPennequin - 27 May 2007 another link to the conversation