[FFmpeg-devel] [PATCH] Add libavsequencer.

[Vitor Sessak]

On 08/19/2010 10:02 PM, Sebastian Vater wrote:
> Vitor Sessak a ?crit :
>> On 08/19/2010 12:04 AM, Ronald S. Bultje wrote:
>>> Hi,
>>>
>>> On Wed, Aug 18, 2010 at 5:58 PM, Stefano Sabatini
>>> <stefano.sabatini-lala at poste.it>   wrote:
>>>> So in the end I suggest this course of action: apply the lavseq patch,
>>>> *or* if we want to add more stuff to it before to commit it, then
>>>> maybe we should focus on the BSS. Once that's ready, we can create
>>>> lavseq *and* immediatly commit the BSS definition.
>>>
>>> I don't want anything committed until I've seen all patches required
>>> for TCM playback posted to ffmpeg-devel so we can judge whether each
>>> component is actually necessary.
>>>
>>> I don't want duplicate code, I don't want overly complicated code and
>>> I don't want overdesigned code. I want exactly that code necessary for
>>> TCM playback. Nothing more. Those patches, we will review together,
>>> removing things that are unneeded/unused, removing code available
>>> already elsewhere in FFmpeg, and simplifying it to the best of our
>>> ability. Once that process is done, we can commit the individual
>>> parts.
>>
>> I've read the whole thread, and I really think this is a great idea.
>> Indeed, let's see a patch (as big as it needs to be, _but not bigger_)
>> to make playing TCM files work. That means that _every_ field in
>> _every_ structure should be:
>>
>> (a) written by some code in the patch
>> (b) read by some code in the patch
>>
>> Everything that is there for the future, for editors, for
>> visualization and for transcoding should be added _later_ (but maybe
>> it is a good idea to keep it in your tree). As I said, simpler patches
>> are approved faster, so let's make this patch as simple as possible
>> (and the simpler way by now is with no new library).
>
> To summarize that for the different parts of avseq:
> 1. The BSS:
> All fields are required (since TCM uses them all).

Not really. There are a lot of "max_xxx" and "min_xxx" that could be 
replaced by a constant without changing the player output.

> 2. The player:
> Most fields are required (some commands are unimplemented right now, due
> to a lack of editor and properly and easy testing of it), I've marked
> today all (hope didn't miss anything out) these parts with a TODO in the
> effect execution path.

Please do not add them in the patch (but you can keep them in your tree).

> This mainly regards the hold / decay stuff which I added for the MED
> demuxer / decoder in TuComposer, which however, never got implemented
> because in the time I wanted to start it was the time where I already
> recognized that it made no sense to continue old TuComposer as Amiga-only.
>
> But MED demuxer / decoder will come into FFmpeg.

One thing at a time.

> 3) The mixer:
> Start with the low quality mixer only. The null mixer is not required
> here, since there's a) no seeking here and b) no hardware mixing support
> now.
>
> Also remove the multiple mixer support until we a) actually need null
> mixer or/and b) have a hardware mixer ready.

Nice!

> Besides this, I'll try to edit my wxTuComposer GUI in parallel, so we
> get the editor advanced as well as the FFmpeg stuff. The editor will
> allow us a) to test everything in the final state and b) to see if our
> current design really fits not only for transcoding / playback but also
> for editing. ;)

I really recommend you do one thing at a time, ie, focus first on 
getting the TCM player committed.

> Please note, that wxTuComposer will be a independent project of FFmpeg,
> it will just be the first project from me that will use the new
> avsequencer API. ;-)
>
> It will probably not only a full editor, but more like a wxWidgets GUI
> class, which you can easily add to your own projects, i.e. create a
> AVSequencer tracker instance with just two lines of C++ code:
>
> wxTuComposer *tucomp = new wxTuComposer ();
> tucomp->Show ();
>
> The rest will be, as usual in wxWidgets just be event handling. ;-)
>
> Maybe there is some day, where you'll find this on wxCode page of
> wxWidgets. ;-)
> Maybe even I start this directly as wxCode project (just disregard that
> I would stick to SVN for that, don't really want to go back to SVN after
> working with git the last three months ;-))
>
>>
>>> Committing one part without knowing how the next part uses it is about
>>> the worst design nightmare possible.
>>
>> I agree also with this. The right way to review the BSS is seeing
>> where and how each field is used.
>
> Great!
>
> One last question here, since my initial patch regarding basic lavseq
> integration added a changelog entry (added libavsequencer). I would
> change that to sth. like: Added libavsequencer (formerly known as
> TuComposer).
>
> The point is here, that some people surely did heard of TuComposer in
> the last 5 years, and in case they search for it, they should find at
> least the FFmpeg page to know that's now named AVSequencer.
>
> I think a small notice regarding this in changelog or is enough. But
> maybe you have other opinions.

Don't worry, when we get something functional in SVN we will mention 
tuComposer in the changelog.

> Just another point, Vitor. You surely remember our last discussions
> regarding BSS data structs in ffmpeg-soc. You know I was afraid that
> extending the fields (uint16_t to enum, int, etc.) was frightening me
> because of no more able to save that 100% to any file format.
>
> I just want to tell you here, that these afraids are actually pointless
> (I talked to Stefano already regarding this some days ago). Since
> IFF-TCM1, as I developed it, is easily extendable regarding that (how I
> should forget that, again 10l to me).
>
> To be short, just an example. Let's say we extend uint8_t flags of
> AVSequencerSong to enum AVSequencerSongFlags (which will be int then,
> thus adding usually more 24 bits).
>
> Storing these in IFF-TCM1 is NO problem! Just to some small magic like:
> first_8_bit_of_flags = get_byte(pb);
>
> Then read the remaining stuff as of old IFF-TCM1, then the first new one
> (let's say we extended to 32-bit):
> flags  = get_byte(pb)<<  8 | first_8_bit_of_flags;
> flags |= get_be16(pb)<<  16;
>
> That should do it. Since IFF-TCM1 also has a version / revision field in
> the MHDR iff tag, we should bump that up simply by one. ;-)
>
> Or to say it simple, forget all my concerns about this! No new format
> required, just some small extension to IFF-TCM1 demuxer / decoder and
> we're fine.

Ok, but I don't really think that IFF-TCM1 should be a kind of DNA of 
the BSS. They should be two different entities.

-Vitor