[MPlayer-dev-eng] [RFC] libnut demuxer API

[Rich Felker]

On Tue, Sep 06, 2005 at 09:42:53PM +0300, Oded Shimon wrote:
> My demuxer API is horrible, I can feel it...
> 
> I need comments, lots of them. What should I do. I certainely can't use 
> MPlayer as an example design :) LAVF is too complicated for me...
> 
> My muxer API probably sucks too.
> 
> Here's what I have so far. Please tell me how much it sucks.

Not too bad..

> typedef struct {
> 	void * priv;
> 	int (*seek)(void * priv, long pos, int dummy);
> 	size_t (*read)(uint8_t * buf, size_t dummy, size_t len, void * priv);
> 	off_t filesize;
> } InputStreamContext;

Don't try to keep compat with stdio functions; instead just use sane
API and call fread/fseek if the function pointers are NULL or
something, or make wrappers.

> typedef struct {
> 	uint8_t * data;
> 	int len;
> } Data;

Useless struct.

> typedef struct {
> 	int width;
> 	int height;
> 	int sample_width;
> 	int sample_height;
> 	int colorspace_type;
> } VideoData;
> 
> typedef struct {
> 	int samplerate_nom;
> 	int samplerate_denom;
> 	int channel_count;
> } AudioData;
> 
> typedef struct {
> 	int type;
> 	Data fourcc;
> 	int time_base_nom;
> 	int time_base_denom;
> 	int fixed_fps;
> 	Data codec_specific;
> 	union {
> 		VideoData vd;
> 		AudioData ad;
> 	} u;
> } StreamData;

Personally I'd do this without a union, putting the generic stream
header at the beginning of the specific-type headers and casting
pointers. I just dislike unions tho. :)

> typedef struct {
> 	int stream_count;
> 	FILE * out;
> 	StreamData * sd;
> } InputData;

Output cannot depend on FILE* and stdio. What if the user wants to
write to a memory buffer, an unbuffered socket, etc. or even use the
lib on an embedded device with no stdio?? IMO all io should be thru
callbacks.

> typedef struct {
> 	int id;
> 	Data type;
> 	Data name;

These can just be null terminated strings in the API.

> 	union {
> 		int i;
> 		Data s;
> 	} val;
> } InfoData;

Again I don't see much advantage to union...

> typedef struct {
> 	int stream;
> 	int pts;
> 	int is_key;
> 	Data f;
> } FrameData;

I just thought of it at this point, but almost all your struct names
end in "Data". This is really rather silly. Personally I'd prefer them
to have names like:

struct nut_frame;
struct nut_info_packet;
struct nut_stream_header;
...

> typedef struct {
> 	enum { e_headers, e_unkown, e_frame, e_info } type;
> 	FrameData fd; // the 'data' part is unused.
> } PacketData;

??

> struct NUTContext_s;
> typedef struct NUTContext_s NUTContext;
> 
> // Muxer
> 
> NUTContext * nut_muxer_init(InputData * id);

This struct is very much nonsense. Putting all the args to a function
in one struct when they're not related is bad API design.

> void nut_muxer_uninit(NUTContext * nut);
> 
> void nut_write_frame(NUTContext * nut, const FrameData * fd);
> void nut_write_info(NUTContext * nut, const InfoData info []);

IMO the API should work something like this:

1. Constuct a NUT muxer context, and fill it with all the global
   header, stream header, and info header data. This can be done
   either with direct access to structs and manual allocations and
   field fill-in by the caller, or with helper functions that allocate
   everything for you and through which you're required to access the
   structs. Some global muxer settings like the degree of header
   repetition, etc. should also be set at this point.

2. Write a sequence of frames. The first frame write should trigger
   all the headers to be written out to the output stream, if the
   caller hasn't already forced them to be written with another
   function. Frame writes can also trigger headers to be written again
   later when a header repetition point is reached, if header
   repetition is enabled. After the first event which triggers headers
   to be written, it's illegal to modify anything in the headers. If
   the header structs are encapsulated with access functions, the API
   should prevent further modification.

3. Call a close function to finalize the output, writing indices if
   the caller wants the file to have an index.

> // Demuxer
> 
> /** Just inits stuff, can never fail. */
> NUTContext * nut_demuxer_init(InputStreamContext isc);

I assume this mallocs something? If so, "can never fail" is
misleading. :)

Actually I'd prefer an API that does no mallocs, but either uses an
allocation callback or requires the caller to pre-allocate things.

> /** Just frees stuff. */
> void nut_demuxer_uninit(NUTContext * nut);
> 
> /** All these functions return:
> 0 success
> 1 unexpected or expected EOF.
> 2 EAGAIN.
> negative number: error, detailed error in nut_error(-1 * ret);
> 
> fatality:
> EAGAIN is never fatal
> EOF is always fatal
> negative error is non-fatal unless it was given by nut_read_next_packet.
> 
> free-ing data malloced by the funtions is only ever needed after a
> successful call. - to protect this, things which should've been malloced will
> turn to NULL.
> 
> nut_read_next_packet should be called after a negative error.

This is very low-level API. I'm not sure it's a bad idea to have such
a thing, but if you do you also need a sane high-level API where the
caller can just..

1. Have the library find and load all necessary headers. Export this
   information to the caller in a sane way.
2. Optionally have the library check for and load an index and/or
   final pts in the file.
3. Seek to a particular frame pts in a particular stream, either
   next/prev keyframe or next/prev frame of any type.
4. Peek at next frame to determine stream/pts/etc.
5. Demux next frame, returning all known data about the frame. Or skip
   next frame if it's not wanted.

ALL of these operations should be able to return an "EAGAIN"-like code
in the case where they run out of data trying to perform their
operation, in which case it should be possible to retry again once the
caller determines that more data is available. This may be somewhat
nontrivial for error recovery and seeking, with multiple seeks and
reads, but it's totally possible.

Also, please ensure that frames are demuxed with "zero copy". It may
be preferable to actually have the demuxer return a pointer to the
internal read buffer, if there's a sane way to lock it to ensure that
it remains valid as long as the caller needs it. For example if the
input stream is an mmapped .nut file, there should be some way to
directly use the mmap for output without ever copying.. But this may
be difficult..?? :(

> nut_read_next_packet also does syncpoint handling.
> It'll start hunting syncpoints if no syncpoint has appeared for a
> long time or one of the functions just return a negative error.

Should be better explained.. There have been LONG discussions on how
error recovery should work but I'm too lazy to dig them up at the
moment.

> Other black magic:
> When nut_read_headers is called, and the stream is seekable, it
> searches for an index

Should be optional, or optionally deferred until the first seek is
requested. It sucks a lot when streaming over http, as ivan keeps
pointing out in his flames.. :)

> nut_skip_packet works differently according to packet type:
> it either reads forward_ptr and skips by that, or it skips by
> frame data. It also mallocs and frees memory to discard the
> packet if non-seekable or method 0 was chosen.

Use alloca or vla. Also there should be a way for the callback to
implement discarding reads, since they're trivial on some input
streams even where seeking isn't possible..

[rest omitted for now]

Rich