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

[Vitor Sessak]

On 07/14/2010 12:16 AM, Sebastian Vater wrote:
> Vitor Sessak a écrit :
>> On 07/13/2010 11:55 PM, Sebastian Vater wrote:
>>> Vitor Sessak a écrit :
>>>> On 07/13/2010 10:11 PM, Sebastian Vater wrote:
>>>>> Vitor Sessak a écrit :
>>>>>> On 07/07/2010 10:46 PM, Sebastian Vater wrote:
>>>>
>>>> [...]
>>>>
>>>>>>> /**
>>>>>>>     * 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).
>>>>
>>>> Guideline-wise it is fine, but I don't think functions should be added
>>>> to the BSS structs without a very good reason.
>>>
>>> That's clear! Just want to mention the point, that the user today
>>> expects the feature of having multiple (in this case modules) files
>>> opened at once and thus editing them. We're not anymore in the eighties
>>> were it was common just to have one file opened at the same time.
>>
>> And what does it change? Both lavf/ and lavc/ can handle several files
>> at the same time (and I mean _really_ at the same time, it is fully
>> thread-safe).
>
> Great! Wasn't really aware of this! I have now to find a way to
> integrate the lavs base structs in lavs/module.h, as I understand it?
>
>>
>>> So I thought of a way of allowing the user exactly that, that's why I
>>> plan to add multiple modules to AVSequencerContext. The programmer
>>> developing the GUI has to take care of this list though mostly by
>>> itself.
>>
>> In the BSS, it does not make much sense an AVSequencerContext.
>>
>> Besides that, I agree with Ronald: why not a different player context
>> for each file and let the application store them as it wish?
>>
>
> This is clearly a until not-yet-known feature of FFmpeg which I didn't
> account to until now. Sorry for that! Now I have to guess the best way,
> to integrate lavs/avsequencer.h into lavs/module.h instead!?

No! avsequencer.h is part of the player and module.h is part of the BSS! 
They should be really well separated to have a clean architecture. Just 
do everything as if there were just a single file and, if you don't use 
global vars, everything will work well if you have two files with two 
different contexts.

Changing a little bit the subject, I suggest that after you take into 
account all the feedback you had, you send a new patchset, this time 
containing just the headers for the BSS (and _only_ the BSS, without a 
single code line or define that belongs to the player!), so we can give 
a second round of reviewing.

> But let's file that tomorrow, otherwise you will read sth. like that the
> next mail:
> fjs9teuwJSDUHPuhfp
>
> Because falling on the keyboard for sake of tireness...;-)

Of course, take your time and have a good night!

-Vitor