[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 :
>>     
>>> Stefano Sabatini <stefano.sabatini-lala at poste.it> writes:
>>>
>>>   
>>>       
>>>> On date Wednesday 2010-08-25 00:55:04 +0200, Sebastian Vater encoded:
>>>> [...]
>>>>     
>>>>         
>>>>> --- /dev/null
>>>>> +++ b/libavsequencer/avsequencer.h
>>>>> @@ -0,0 +1,54 @@
>>>>> +/*
>>>>> + * AVSequencer main header file which connects to AVFormat and AVCodec
>>>>> + * Copyright (c) 2010 Sebastian Vater <cdgs.basty at googlemail.com>
>>>>> + *
>>>>> + * This file is part of FFmpeg.
>>>>> + *
>>>>> + * FFmpeg is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU Lesser General Public
>>>>> + * License as published by the Free Software Foundation; either
>>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>>> + *
>>>>> + * FFmpeg is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>>> + * Lesser General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU Lesser General Public
>>>>> + * License along with FFmpeg; if not, write to the Free Software
>>>>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>>>> + */
>>>>> +
>>>>> +#ifndef AVSEQUENCER_AVSEQUENCER_H
>>>>> +#define AVSEQUENCER_AVSEQUENCER_H
>>>>> +
>>>>> +#define LIBAVSEQUENCER_VERSION_MAJOR 0
>>>>> +#define LIBAVSEQUENCER_VERSION_MINOR 0
>>>>> +#define LIBAVSEQUENCER_VERSION_MICRO 0
>>>>> +
>>>>> +#define LIBAVSEQUENCER_VERSION_INT AV_VERSION_INT(LIBAVSEQUENCER_VERSION_MAJOR, \
>>>>> +                                                  LIBAVSEQUENCER_VERSION_MINOR, \
>>>>> +                                                  LIBAVSEQUENCER_VERSION_MICRO)
>>>>> +#define LIBAVSEQUENCER_VERSION     AV_VERSION(LIBAVSEQUENCER_VERSION_MAJOR,   \
>>>>> +                                              LIBAVSEQUENCER_VERSION_MINOR,   \
>>>>> +                                              LIBAVSEQUENCER_VERSION_MICRO)
>>>>> +#define LIBAVSEQUENCER_BUILD       LIBAVSEQUENCER_VERSION_INT
>>>>> +
>>>>> +#define LIBAVSEQUENCER_IDENT       "Lavsequencer" AV_STRINGIFY(LIBAVSEQUENCER_VERSION)
>>>>> +
>>>>>       
>>>>> +/**
>>>>> + * Returns LIBAVSEQUENCER_VERSION_INT constant.
>>>>> + */
>>>>> +unsigned avsequencer_version(void);
>>>>> +
>>>>> +/**
>>>>> + * Returns the libavsequencer build-time configuration.
>>>>> + */
>>>>> +const char *avsequencer_configuration(void);
>>>>> +
>>>>> +/**
>>>>> + * Returns the libavsequencer license.
>>>>> + */
>>>>> +const char *avsequencer_license(void);
>>>>>       
>>>>>           
>>>> Returns -> Return
>>>>
>>>> Looks OK otherwise, regards.
>>>>     
>>>>         
>>> Pretty please, do not commit _anything_ related to this until a full,
>>> functional set of patches has been presented and reviewed by several
>>> people.
>>>
>>>   
>>>       
>> Hi Mans, I understand your point, but currently a) libavsequencer is
>> disabled by default and b) is tagged as experimental.
>>     
>
> That doesn't matter.  There is no point whatsoever in committing
> something that may not survive even a cursory review once the rest is
> presented.  In fact, doing so would be a VERY BAD THING.
>   

Yes, that's true! But avsequencer has been reviewed more than you might
think at first (see ffmpeg-soc repository for our discussions regarding
this).

>   
>> If I remember correctly, libavfilter got a similar way to get into SVN.
>>     
>
> Nothing from libavfilter was committed before it was functional,
> albeit optional.  By the time the first parts were committed to main
> svn, it had been under development for well over a year, and there had
> been numerous lengthy discussions about various aspects relating to
> it.  In contrast, libavsequencer (or whatever the name of the week is)
> strikes me as a rush job being rammed into svn without semblance of
> proper review.
>   

Hmm, certain parts are submitted here weeks ago, there was some time to
review them and to post complaints, but until now, no one, except
Stephano regarding the nits commented on that, so maybe you missed it
simply during that time?

> >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? Please look at my
github regarding this, it CHANGED HEAVILY during that time! In all
aspects: documentation, code and header stuff.

Also, some parts already have been reviewed (see ffmpeg-soc mainly for
that), so why you are concerning me, that I'm dodging reviews?

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?

>   
>> 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, there's still plenty of code left, of course. Since all that
stuff is large, 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.

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.

Anyway, please checkout my latest github and see for yourself. I'll have
to submit the IFF-TCM1 test files to some central place, though.

> 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.

-- 

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