[FFmpeg-soc] [soc] libavsequencer [PATCH 01/08] Music module public API header file.

[Vitor Sessak]

On 07/10/2010 11:31 PM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 07/10/2010 07:57 PM, Sebastian Vater wrote:
>>> Vitor Sessak a écrit :
>>>>
>>>>> +#include "libavsequencer/avsequencer.h"
>>>>> +#include "libavsequencer/player.h"
>>>>> +#include "libavutil/tree.h"
>>>>> +
>>>>> +/**
>>>>> + * Sequencer module structure.
>>>>> + * New fields can be added to the end with minor version bumps.
>>>>> + * Removal, reordering and changes to existing fields require a major
>>>>> + * version bump.
>>>>> + */
>>>>> +typedef struct AVSequencerModule {
>>>>> +    /** Metadata information: Original module file name, module name,
>>>>> +     *  module message, artist, genre, album, begin and finish
>>>>> date of
>>>>> +     * composition and comment.  */
>>>>> +    AVSequencerMetadata *metadata;
>>>>
>>>> I don't think you need any struct for the metadata: AVFormatContext
>>>> has one already. Note that our metadata API allows you to add any name
>>>> for metadata by adding for ex.
>>>>
>>>> av_metadata_set2(&s->metadata, "module message", module_message,
>>>> AV_METADATA_DONT_STRDUP_VAL);
>>>
>>> Already fixed in github.
>>
>> It is still not good in github. Why duplicating the
>> AVFormatContext->metadata as AVSenquencerSynth->metadata?
>
> You surely meant AVSequencerModule->metadata not synth, right?

Oops, there is a file metadata inside the synth data? I hope some 
demuxer expert can tell how to set metadata in these cases...

>> Since I imagine that adding new instruments, songs, etc should be
>> pretty rare and their number should be pretty small, I imagine that
>> the extra simplicity of using an array is worth the O(n) cost to add
>> an item.
>
> I agree totally with that! O(1) for access is way more important than
> inserting or deleting. What do you think about:
> AVSequencerInstrument **instrument_list?
>
> So we have just a pointer list to move around instead the huge structures?

I don't know what typical values are, but is the folowing doable?

#define MAX_INSTRUMENTS [... some value here ...]
AVSequencerInstrument *instrument_list[MAX_INSTRUMENTS];

Everything access and insertion O(1) and both done in one line of code...

>> If it is currently unused, I think the variable should be removed
>> altogether. We can add it later when needed.
>
> Fixed locally right now (will commit soon) by removing both compat_flags
> and flags, since compat_flags is also unused in the module right now.

Nice.

>>>>> +    /** 64-bit integer indexed unique key tree root of unknown data
>>>>> +       fields for input file reading with AVTreeNode->elem being
>>>>> +       unsigned 8-bit integer data. Some formats are chunk based
>>>>> +       and can store information, which can't be handled by some
>>>>> +       other, in case of a transition the unknown data is kept as
>>>>> +       is. Some programs write editor settings for module in those
>>>>> +       chunks, which won't get lost then. The first 8 bytes of this
>>>>> +       data is an unsigned 64-bit integer length in bytes of
>>>>> +       the unknown data.  */
>>>>> +    AVTreeNode *unknown_data;
>>>>
>>>> Might make sense storing it somewhere, but why not a plain buffer?
>
> You mean like:
> uint8_t **unknown_data?
>
>
>> Why not then a "void *"?
>
> void ** or uint8_t ** would be better I think...each chunk is a pointer
> then. That way single chunks can be changed without recalculating
> everything else.

Why do you think in chunks? They might be organized in a tree inside the 
file or a circularly linked list, or etc. void * do not exclude int8_t **...

>>> Fixed in github by removing user_data in all header files.
>>>
>>> However, these structures are all to be supposed to be writable by the
>>> client, how you otherwise could write a tracker using FFmpeg, when the
>>> stuff the user edits can't be applied to this structure?
>>
>> I agree.
>
> user_data is removed though, since rethinking this took me to the
> conclusion that it's better to let the application developer decide this
> and if he needs extra data for this, he can still do:
> typedef struct MyTrackerModule {
>      AVSequencerModule module;
>      [...] custom stuff follows here with exact declaration
> } MyTrackerModule;

Nice.

-Vitor