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

[Sebastian Vater]

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?

>
> 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?

>
> 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.

>
>>>> +    /** 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.

>> 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;

>
>>>
>>>> +} AVSequencerModule;
>>>> +
>>>> +/**
>>>> + * Registers a module to the AVSequencer.
>>>> + *
>>>> + * @param module the AVSequencerModule to be registered
>>>> + * @return>= 0 on success, error code otherwise
>>>> + *
>>>> + * @NOTE This is part of the new sequencer API which is still under
>>>> construction.
>>>> + *       Thus do not use this yet. It may change at any time, do not
>>>> expect
>>>> + *       ABI compatibility yet!
>>>> + */
>>>> +int avseq_module_register(AVSequencerModule *module);
>>>
>>> ?
>>> Should this be created only when the file is opened? Normally, we have
>>> xxx_register() only for things that are statically initialized before
>>> opening any file (codecs, demuxers, etc).
>>
>> Yes, when the file is opened and added to AVSequencerContext->modules
>> list. I just see that there is a parameter missing, AVSequencerContext
>> *avctx...should I change the same to avseq_module_open instead?
>
> Yes.

Fixed locally, will commit that either tomorrow or before going to bed.

-- 

Best regards,
                   :-) Basty/CDGS (-: