[FFmpeg-devel] [PATCH] Add libavsequencer.

[Sebastian Vater]

M?ns Rullg?rd a ?crit :
> That list is read by only a few people.  Something as intrusive as
> this needs a broader review.
>   

Yes, that's true! No panic, we will start with this very soon.

>>> >From what I saw looking at the tucomposer source earlier, I say it is
>>> humanly impossible to turn it into ffmpeg-worthy code in the time
>>> which has been available.  Your attempts at dodging reviews and
>>> refusal to submit a minimal patch set reinforces this feeling of mine.
>>>       
>> Why do you look still at TuComposer's original source?
>>     
>
> I looked at it a couple of months ago after you posted a link to it.
> It was total garbage.
>   

I still want to hear from you, what's the point considering you it being
garbarge? When I asked you this during that time, you simply complained
about an unnecessary cast.

I mean, you surely neither compiled nor tested it. I know that the
coding style I was using during that time, is somewhat weird, the
backport from 100% Asm doesn't make it any better, probably.

>   
>> Please look at my github regarding this, it CHANGED HEAVILY during
>> that time! In all aspects: documentation, code and header stuff.
>>     
>
> It is impossible to turn what I saw back then into anything even half
> decent in the time which has passed since.
>   

Well, take a look into github then, and see for yourself...the link to
it, is below.

>   
>> 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!
So the remainder should NOT be accepted on faith, since it's simply gone
(replaced by macros).

>   
>> 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, which is quite close
now. The point is, once we review it on a larger scale here and also the
whole stuff, we'll probably do some API changes, internal code logic
changes.

This will require regression tests as well (which I want to write once
it's on par with TuComposer regarding playback compatibility). From then
on, we can easily see when some files change playback (regression test
failures).

That's the reason I want to wait a bit with the patches, hope you
understand that better.

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?

>   
>>>> So I don't really see any big issues not commiting that part already.
>>>> But maybe I'm missing sth. out?
>>>>         
>>> You're missing the part where nobody has seen the minimal, functional
>>> patch set which has been repeatedly requested.
>>>       
>> As said, some of the stuff already got reviewed in the ffmpeg-soc
>> thread,
>>     

>
>   
>> 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? Despite the features it has, it isn't
much. Or don't you mean executable size but instead the spaces all
around in the source code, i.e. writing foo ( .., .. ) instead foo(.., ..)?

>   
>> but we should start at some point, and the way I got suggested it
>> was to post minimal patches here which to get reviewed and
>> integrated.
>>     
>
> Reviewing the first step is impossible when the final goal is unknown.
>
>   
>> Please note, that I did a lot of private communication between Vitor,
>> Stefano, Benjamin, Ronald and Michael. They also have a huge set of
>> IFF-TCM1 files to see how the current player / demuxer / decoder works.
>>     
>
> So why are you trying to push ahead against Ronald's request now?
>   

Sorry, here are you right, of course. Did totally forget it yesterday
evening, that Ronald also wanted to hold back the patches.

>   
>> Anyway, please checkout my latest github and see for yourself.
>>     
>
> That would be much easier if you provided a link.
>   

Stefano already posted it some time ago, but here it is again:
http://github.com/BastyCDGS/ffmpeg-soc


>   
>> I'll have to submit the IFF-TCM1 test files to some central place,
>> though.
>>     
>
> That would be a start.
>   

Could you check what's wrong with anonymous FTP here? Some weeks ago I
tried to upload some files, could create a directory but not upload
anything. Maybe it's also possible to create a developer account for me.

>   
>>> Again, I request that nothing be committed before a proper review
>>> has taken place.
>>>       
>> Of course, everything should be tested fine and work properly before
>> commited. So I prefer (as some dev's recommended me) to do that piece by
>> piece, i.e. small and easy to review patches, if they work for it's own,
>> they're ok.
>>     
>
> Pieces cannot be reviewed in isolation.  The reviewer must have the
> full picture in order to make an assessment of any single piece.
>   

I mean the BSS, player and demuxer/decoder stuff. I was pleased to
submit BSS patches separately to the others.

-- 

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