[FFmpeg-soc] [soc] libavsequencer [PATCH 02/08] Sub-song public API header file.

[Sebastian Vater]

Vitor Sessak a écrit :
> On 08/15/2010 06:41 PM, Sebastian Vater wrote:
>> It is used by the player, i.e. the player handles 6-bit volumes when
>> this flag is set. I also remembered somethings which makes it impossible
>> to replace this without getting trouble (synth sound support).
>> The synth sound assembler also uses 6-bit in that case and since this a
>> full-featured assembly language, it's practically impossible to convert
>> TCM files correctly to 8-bit instead. :(
>
> What about other file formats?

Support usually only 6-bit volumes, though.

But shouldn't be the rule here, if there is one exception that we have
to address that issue?

If we want it format independent, then it should also consider the fact,
that TuComposer requires this.

Sorry that I didn't recognize that already yesterday, I remembered that
after my writing only.

>
>> In TuComposer the user was able to put a parameter for the demuxers
>> which decides to use original 6-bit or 8-bit volume. The current demuxer
>> in FFmpeg, however doesn't allow this now, but it might encounter a file
>> which already has this flag set.
>> [...]
>> player.h is read-only for composers and external users, i.e. anything
>> that will go there isn't user editable anymore and this flag is supposed
>> to be user editable.
>
> So you need something that is both user-editable and not part of the
> BSS. Again, where should be stuff that is not part of the description
> of a song _and_ is set by the user?

Setting the stack size is part of the BSS since it belongs to song
description. Please note a song with a different stack size doesn't
sound necessary the same (and this is not because of the player but pure
logic, the a song with a different starting tempo doesn't sound the same
also), and the composers should really be free in what they need here. I
know it's rather an unconventional feature.

Again, the stack pointers itself (which contain the data) are a
completely different thing and that's why I moved them to player.h
without big questions, since that really belongs to the player and not
to the BSS, but that's not the case for the size.

>
>>
>> The player itself can also play more than 256 channels, in fact it is a
>> limitation of both the player and the BSS (the track limitations and
>> effect limitations are accounting for the BSS, too).
>
> So where the limitation would be if we change the BSS to a int16_t
> (no, I don't think this has any use doing it, but you understand my
> point)?
>
> What it looks like to me you are doing is:
>
> 1- You have some BSS parameter foo.
> 2- Sane values for the parameter foo is between 0 and AA
> 3- You look for the integer type whose maximum value is closest to AA
> 4- You use this type for foo.
>
> I don't think this is the cleaner neither the most future-proof way of
> determining sane values for fields.

I already extended integer types where it made sense. That's also why
the sub-song channels in the BSS (and player!) is uint16_t and not
uint8_t (which would be 0-255).

But more than 65535 host and virtual channels will probably never ever
make sense (imagine just the time to fill that all up by a composer
track to track, row to row).

Also if you noticed that all the public API calls take uint32_t as a
parameter, even for channels, so they can assign larger values (which
are either clipped or return AVERROR_INVALIDDATA.
The same applies, btw, to stack size, it is limited to 65535 entries
(which would mean a loop / gosub stack of 65535 which is way larger than
everyone requires).

Looping a pattern, e.g. 65535 times would make the sound REALLY boring
(you can't extend pattern loop beyond one pattern).

Having an order list range played with a gosub stack size of 65535 would
also make the song very boring.

For the volume / panning stuff we already clarified that, I want to
extend that to full 16-bit someday.

The same also applies to the boundary ranges, they are not as hard as
they look at first, since all of these values are 16 bit, too.

But having e.g. tempo larger than 65535 would delay a row soo much long
that you can wait hours before it increases, BpR && BpM > 65535 is the
same, this is "speed of light" and would increase CPU consumptation such
heavy that even a single channel module couldn't playback with a
Core2Duo and alike.

Making data types way larger than required is sometimes no problem,
unless it is required very, very often (like the track effects), which
would cause a significant increase in memory usage.

Also note, all trackers I know of are in this regarding this still 8-bit
only, since command data is 8-bit. Extending that to 16-bit is way more
than enough and should sastify for the next 50 years.

>
>>
>> The point is that when we don't have a format which can save / restore
>> the BSS in a 100% state, this can be awful.
>>
>> Imagine you're sitting one week on a new song, using an extended BSS
>> which no format can save and/or load 100% because there is still are
>> still no (de-)muxers for it and you notice that 90% of your work is crap
>> because you can't save it.
>
> You are using a crap editor to fill up the BSS. Moreover, your point
> is only valid considering everybody will want to write only TCM files.
> Your GUI should not let you use unsupported values, even if it fit in
> the data types.

The editor can't know which formats are supported in exact detail by the
current FFmpeg version, the editor should simply use the public API
calls which either a) return INVALIDDATA or b) clip to allowed range
where it makes sense (i.e. setting number of maximum channels) and are
already 32-bit.

After all, I invented the API for taking as much burden as possible from
application writers.

>
>> I'm not generally against the idea to extend
>> that, but we should make first sure that it can be 100% exported and
>> imported.
>
> I'm not talking about extending. I mean that converting a field from
> uint8_t to int should not mean every 16-bit value is now valid!

Agree here! For the flags we can really extend them to int (enum).
As long we won't define any new ones across the current boundaries it
won't be any problem.

>
>> Or consider the following, you see that FFmpeg has a nice avsequencer
>> and start using it writing a GUI, since you see that it supports 16-bit
>> effect commands you write the GUI it way, i.e. the users think they can
>> use also the 16-bit range and when they save it, because they think, hmm
>> 16-bit, so I have lots of user effects for my game / demo and then
>> save...BAM!
>
> GUI developers should not need babysitting. And _if_ we want to
> specify this limits somewhere, it should not be in the BSS (limits.h?).

The limits.h idea is great! :)
But give me some time on really think on it, how to implement it the
best way.

>
>> Please note that most tracker editors are more like hex editors, i.e.
>> command, instrument and command data are edited directly in hex (and
>> sometimes decimal) values.
>
> And they don't do any sanity-check?

For effects? No, either they ignore it or do something undocumented,
sometimes they also set to an internal maximum / minimum value which is
handled differently from program to program, which is also one of the
biggest problems having all the different MOD players to get the same
playback for the same file.

>>
>> I'll understand the point better now, but there are still some issues to
>> solve before. My point now is: We should first define a new
>> muxer/demuxer format which can im-/export the new extended features,
>
> I'm not suggesting adding any new features, I suggest not depending on
> the BSS data types to know what is supported or not.

Most data types are long enough already, the point I see is mainly the
flags (where I now finally agree, see below) and volume / panning stuff.

>
>> before actually implementing them (or at least in parallel).
>>
>> Imagine having a word processor where you can set bold/italic/etc. but
>> it can only save ASCII, therefore loosing all that. It simply doesn't
>> make sense to have features which cannot be exported and re-imported
>> at all.
>
> And you would say that the problem in this case would be _the internal
> data structures_ used by the word processor?

Internal? Well, the BSS is public API, right?

>
>>>> That's why I keep the limitations to be consistent, i.e. fitting in
>>>> each
>>>> other.
>>>
>>> I don't really care much at this point about _what_ the limitations
>>> are, I care now about _where_ they are, and IMHO it is not in the BSS.
>>
>> Limitations to be also defineable by the user should be in the BSS,
>> read-only limitations can, of course be moved into the player.
>
> Is this _part of the song description_?

Yes it is. Remember, these limitations are not BSS and player specific.
It's like saying (to take the car example): Yes, I know I can drive 300
km/h, but I prefer just 100 km/h and limit me myself consciousnessly by it.

The actual BSS / player limitations are as said all either 16-bit range
(for the speed stuff) or even 32-bit (sample rate).

BTW, the limits are set by default to the whole integer type range (in
most cases), if you want to restrict that actually, you have to do that
for yourself (see some of my source files in github, to see of it is
actually used).

>
>> Please always note (i.e. never forgot) that you also can create an
>> avsequencer empty file and edit it, in that case you can edit during
>> composing time your limitations to ensure you won't have surprises when
>> you save to your format.
>>
>> For example if I create a new empty file and want to save it on
>> composition finish as S3M, I can set the use S3M-like global volume
>> command flag manually and I know that it's fine when I finally save it
>> in S3M.
>>
>> The same applies if I want to do a module for a friend which only has an
>> Amiga 500 and I want to ensure that it will playback fine there with an
>> ordinary MOD player, so I can again set the Amiga limits manually.
>
> Again, in the BSS? This enforcing this limits is the job of the
> editor, and he should read from somewhere else than the BSS (again,
> the BSS should be _format independent_).

Wouldn't that require that the editor would know the limits of the BSS?

>
>>
>> And therefore lose ability to playback files correctly.
>
> How so? We are talking about a field that we just agreed that won't
> change playback.

Yes that was my fault, I just forgot the synth sound assembler when I
thought that this won't change playback.

>
>> Also again here
>> the same accounts, the min/max pairs are also supposed to be
>> user-editable, moving them to player.h will again make this read-only.
>
> So player_config.h or something else.

limits.h?

>
>> Imagine, I want to start a new song with avsequencer but want to ensure
>> that it will save correctly let's say in IT. Then I can set manually
>> these values to the IT limits and I'm sure that when saving my work, I
>> get a correct file.
>
> Again, I think the BSS should do _one_ _single_ _thing_: describing
> the song in a format-independent format. IT limits are useful to be
> set somewhere, but not in the BSS.

I know and that's why all these fields are here, please note, I didn't
added them, because I was bored, but because I found that to be
necessary. TuComposer's main strength is compatibility playback and
that's because of these fields and flags, if we remove them, we have
just another MOD engine which fails here and there.

Anyway moving actual limits to limits.h is a great idea!

>
>>
>> See above, we can do this, but we should additionally define a new
>> (de-)muxer pair which can save this, otherwise it's pointless.
>
> The whole point was allowing changing an uint8_t to an enum who could
> be no greater than 255 (but could be internally handled by the
> compiler as a 16/32-bit integer). How would it change things?

Oh, well that's right. Agree here now!

One last question, do you agree that we do the structural changes when
everything works fine?

I mean FFmpeg is able to playback TCM modules since today (read my
mail?), but you see there are some issues right now probably mostly
arrived simply by doing a simple change from linked-list storage to **
array.

Anyway, very very much thanks for your constructive critics and ideas!

I'll submit now the current patch set for the BSS as suggested by Ronald
all in one patch.

-- 

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