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

[Sebastian Vater]

Vitor Sessak a écrit :
> On 07/07/2010 10:46 PM, Sebastian Vater wrote:
>>
>> /**
>>  * 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.  */
>>     AVMetadata *metadata;
>
>>     /** AVSequencerPlayerChannel pointer to virtual channel data.  */
>>     AVSequencerPlayerChannel *channel_data;
>
> Is there just a virtual channel data per file? And BTW, the "Player"
> part of the field bugs me. Does it really belongs to the BSS?

Yes, you are right, this can be moved to AVSequencerPlayerGlobals and
will fix this (will require some changes to player.c, though).

>
>>     /** Array of pointers containing every sub-song for this
>>        module.  */
>
> "Array (of size songs) of pointers containing every sub-song for this
> module.", same for the others.

Fixed.

>
>>     AVSequencerSong **song_list;
>>
>>     /** Array of pointers containing every instrument for this
>>        module.  */
>>     AVSequencerInstrument **instrument_list;
>>
>>     /** Array of pointers containing every evelope for this
>>        module.  */
>>     AVSequencerEnvelope **envelope_list;
>>
>
>>     /** Array of pointers containing every keyboard definitionb list
>
> typo

Fixed.

>
>>        for this module.  */
>>     AVSequencerKeyboard **keyboard_list;
>>
>>     /** Array of pointers containing every arpeggio envelope
>>        definition list for this module.  */
>>     AVSequencerArpeggio **arpeggio_list;
>
>>     /** Duration of the module, in AV_TIME_BASE fractional
>>        seconds. This is the total sum of all sub-song durations
>>        this module contains.  */
>>     uint64_t duration;
>
> Is this ever written in the files or is it calculated by the player?
> If the later, it does not belong to the BSS.

This is exactly for one module, since AVSequencerContext can contain
multiple modules (if an editor supports editing multiple modules at
once), I decided to move it here, instead.

This can both be calculated by the player but also be preset by the
demuxer. Sometimes for very complex modules, automatic calculation is
almost impossible, so the user can simply playback, look at the total
time and enter it. This field is supposed to be editable by the composer.

>
>>     /** Number of sub-songs attached to this module.  */
>>     uint16_t songs;
>
> Cosmetics: I prefer, for readability, to move this closer to
> AVSequencerSong **song_list. Same for the following.

Fixed.

>
>>     /** Number of instruments attached to this module.  */
>>     uint16_t instruments;
>>
>>     /** Number of envelopes attached to this module.  */
>>     uint16_t envelopes;
>>
>>     /** Number of keyboard definitions attached to this module.  */
>>     uint16_t keyboards;
>>
>>     /** Number of arpeggio definitions attached to this module.  */
>>     uint16_t arpeggios;
>
>>     /** Maximum number of virtual channels, including NNA (New Note
>>        Action) background channels to be allocated and processed by
>>        the mixing engine (defaults to 64).  */
>>     uint16_t channels;
>
> Again, is it read from the file or calculated by the player?

It can be set as maximum by the composer during editing. Most demuxers
with non-NNA modes can set this automatically without problem, but for
NNAs this is not possible, the user can set a maximum value (IT for
example uses this). Not using that will not make the module sound as it
originally was intended.

>
>> #define AVSEQ_MODULE_CHANNELS   64
>>
>>     /** Array of pointers containing every unknown data field where
>>        the last element is indicated by a NULL pointer reference. The
>>        first 64-bit of the unknown data contains an unique identifier
>>        for this chunk and the second 64-bit data is actual unsigned
>>        length of the following raw 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 then won't get lost in that case.  */
>>     uint8_t **unknown_data;
>> } AVSequencerModule;
>>
>> /**
>>  * Opens and registers module to the AVSequencer.
>>  *
>>  * @param avctx the AVSequencerContext to store the opened module into
>>  * @param module the AVSequencerModule which has been opened to be
>> registered
>>  * @return >= 0 on success, a negative 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_open(AVSequencerContext *avctx, AVSequencerModule
>> *module);
>
> Hmm, I think I don't really understand what this function do. Suppose
> you want to write a very short test program to play a MOD file using
> the libraries. Roughly, what functions it will call, in which order
> and with which parameters?

To be honest, I added this prototype only for now to see if the way I
want to add them is compatible with FFmpeg style guide. Wanted just to
avoid adding functions and all follow a wrong guideline. But the suppose
is to load a module from disc and register it to the module list in avctx).

The loading, of course, is not necessary in FFmpeg, as it will be done
by the demuxer itself, but the register process. Also this function can
be called from the user if he loads a module.

-- 

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