[FFmpeg-devel] [PATCH] Add libavsequencer.

[Sebastian Vater]

M?ns Rullg?rd a ?crit :
> Sebastian Vater <cdgs.basty at googlemail.com> writes:
>
>   
>> M?ns Rullg?rd a ?crit :
>>     
>>>   
>>>       
>>>>>> Also, some parts already have been reviewed (see ffmpeg-soc mainly for
>>>>>> that), so why you are concerning me, that I'm dodging reviews?
>>>>>>         
>>>>>>             
>>>>> A while ago you proposed that, to expedite the process, only a part of
>>>>> a huge blob of code be reviewed, and the remainder accepted on faith.
>>>>> I call that dodging review.
>>>>>       
>>>>>           
>>>> I think you mean the part where we were talking about the mixer stuff.
>>>>
>>>> The point here is that the mixing functions are to 99% very similar,
>>>> i.e. 16-bit mixing in differs in int16_t vs. int8_t for 8-bit mixing and
>>>> shifting. It's a total of 23k lines which is pretty large.
>>>>
>>>> My idea here was, that just 2-3 of these functions are reviewed, then we
>>>> discuss of creating #define macros for these and therefore get rid of
>>>> 20k lines, making the whole stuff much more easilier to maintain and
>>>> also to review, saving us all huge amounts of time.
>>>>
>>>> So you simply misunderstood that part, sorry for this!
>>>>     
>>>>         
>>> I would appreciate if you dropped that belittling attitude.  You speak
>>> as though to a small child who believes he has seen an elf.  Most
>>> people, little children included, take offence at being addressed in
>>> such a manner.
>>>   
>>>       
>> Could you clarify here, what you exactly mean? I really don't understand.
>> Maybe with some examples showing the issue on a strong emphasis and what
>> you expected and got? Thanks in advance!
>>     
>
> "you simply misunderstood that", "you must have missed that", "it is
> quite complex, really", etc, etc.
>   

Sorry, if this offended you, but I neither wanted nor will threat you
(or anybody else) as a small child. I just considered it as a
description as I had seen the issue.

Please note, that I also use these terms at myself (I just missed that,
I misunderstood this), and I'm surely not considering me as a small
child in these cases, also. ;-)

Anyway, for sake of peace and respect, I'll try to avoid this in the
future, when I talk to you.

>   
>>>>>> Regarding the minimal patch...this is just what you have in front of
>>>>>> you! A minimal patch, which does simply nothing else than adding the
>>>>>> library...or did you mean sth. else here?
>>>>>>         
>>>>>>             
>>>>> That patch does NOTHING useful.  Ronald asked you repeatedly to submit
>>>>> a full set of patches allowing SOMETHING to be done with the simplest
>>>>> file format.  You continue to (rather clumsily) attempt to evade this
>>>>> request.
>>>>>       
>>>>>           
>>>> I already told them that I want to do first a port which has the same
>>>> compatibility as TuComposer regarding playback,
>>>>     
>>>>         
>>> Not in my svn.
>>>   
>>>       
>> What's the problem you see here with this approach? After all, this is
>> quite complex, and small changes can have side effects (sometimes not
>> easy to track by audible means).
>>     
>
> It is a huge piece of unfinished code.  It doesn't belong there.
>   

Yes, the part being most advanced is the player right now, as well as
the lq mixer (except the large code line problem).

>   
>> As said, I want to use that as a base of a automatic regression test only.
>>     
>
> You can run tests on your own copy.
>   

First I have to write the tests, though. ;-)

My plan to implement the tests is, that I write, while playback &
mixing, to a text file containing the internal dump of the player / BSS
/ mixer structures. This has to be done twice, one for original
TuComposer, and one for AVSequencer.

Then after testing, I just compare the text files with a simple diff, I
don't only see if it passed the tests (diff empty) but also if there's a
regression, where it occured and what's changed.

I yesterday was busy getting the AVSequencer to pass other tests, after
having finished the deallocation stuff, though:
All tests with valgrind, i.e. allocation / deallocation of complete BSS,
basic avsequencer, mixer and player does not cause a single hit anymore
in valgrind strictest memcheck / memleak mode.

There are, however, some show-stopper bugs in the playback engine, which
causes some TCM-files to crash and valgrind also has some minor
complains there.

>   
>>>> Anyway, why do you think that I'm against reviewing it? After all, the
>>>> review process makes a) code quality better b) improves readibility
>>>> (documentation and code) and c) fixes bugs. So again, why do you think I
>>>> try to avoid this?
>>>>         
>>> How else should I interpret your refusal to send reviewable patches?
>>>   
>>>       
>> Some files still change too frequently and too much in a short period,
>> i.e. sending patches has for some parts a pretty high probability that
>> once review is done (which can quite a bit of time) the code does not
>> match the current progress anymore.
>>     
>
> Then clearly, the code is not yet ready for review.
>   

I expect it at least be one more month before it's ready for a
full-featured review.
But this shows another issue, though, I should generally be more
communicative on that to avoid wild speculations. So thank you very much
for addressing this issue and solving that finally. ;-)

>   
>> I am still thinking of the best way to post the patches actually, i.e.
>> which parts should it all contain and which not? I got suggested to
>> remove anything from the patches which is currently unreachable, etc.
>> Figuring that out, is despite the large structures not always that easy.
>>     
>
> Maybe once it's actually ready to be reviewed this will be easier.
>   

Yes, after all, removal of unused stuff can also affect the API, so
it'll go hand in hand.

>   
>>>>>> Since all that stuff is large,
>>>>>>         
>>>>>>             
>>>>> That is a problem.  You, however, appear to be quite proud of the
>>>>> bloat you have produced.  That is not a good sign.
>>>>>       
>>>>>           
>>>> Why do you think it being bloat?
>>>>         
>>> An excess of 20k lines for a simple audio mixer is bloat, whichever
>>> way you look at it.
>>>   
>>>       
>> Yes, solving that with define's as you showed me with the IFF-ILBM
>> decoder optimization lut's, would be way nicer, for the mixer I strongly
>> was preferring speed over size.
>>     
>
> Even with separate code for 8-bit, 16-bit, 32-bit, and float, 20k
> lines seems very much.  20k is plenty to write an entire operating
> system.
>   

For 8-bit samples: Separate code also for mono, left, right, center,
surround, any other panning value

Also for 16/32/n-bit samples: Same separation for same panning values as
well as separation for real 16 bit mode (which uses mul/div loop instead
of 8-bit volume lut having more accuracy in terms of quality).

Also non-interpolated and linear interpolation (full and adaptive) modes
are supported.

Luckily, in many cases one C line is one asm instruction (at least for
m68k).

-- 

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