[FFmpeg-devel] [RFC/PATCH 5/8] tidsp: add init/close

Felipe Contreras felipe.contreras
Tue Sep 7 21:33:08 CEST 2010


On Tue, Sep 7, 2010 at 10:17 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Tue, Sep 07, 2010 at 05:42:10PM +0300, Felipe Contreras wrote:
>> On Tue, Sep 7, 2010 at 3:28 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>> > On Tue, Sep 7, 2010 at 6:33 AM, Felipe Contreras
>> > <felipe.contreras at gmail.com> wrote:
>> >> On Tue, Sep 7, 2010 at 1:09 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >>> On Mon, Sep 06, 2010 at 01:15:32AM +0300, Felipe Contreras wrote:
>> >>>> Signed-off-by: Felipe Contreras <felipe.contreras at gmail.com>
>> >>>> ---
>> >>>> ?libavcodec/tidsp_mpeg4.c | ? 28 ++++++++++++++++++++++++++++
>> >>>> ?1 files changed, 28 insertions(+), 0 deletions(-)
>> >>>>
>> >>>> diff --git a/libavcodec/tidsp_mpeg4.c b/libavcodec/tidsp_mpeg4.c
>> >>>> index 3c3190d..1ed2e90 100644
>> >>>> --- a/libavcodec/tidsp_mpeg4.c
>> >>>> +++ b/libavcodec/tidsp_mpeg4.c
>> >>>> @@ -8,8 +8,34 @@
>> >>>> ? * in the packaging of this file.
>> >>>> ? */
>> >>>>
>> >>>> +#include "tidsp/tidsp.h"
>> >>>> +
>> >>>> ?#include "avcodec.h"
>> >>>>
>> >>>> +struct td_av_context {
>> >>>> + ? ? struct td_context *td_ctx;
>> >>>> +};
>> >>>
>> >>> why this wraper struct?
>> >>
>> >> See patch 8/8: tidsp: process buffers.
>> >>
>> >> The code in the 'tidsp' directory (and thus struct td_context) is
>> >> supposed to be independent of FFmpeg, and according to other people's
>> >> comments I should strip out and make an independent library.
>> >
>> > So drop td_av_context and use td_context directly. :-). Saves you a
>> > new extra struct.
>>
>> If you see patch #8, you would see that I add an output_buffer there;
>> that doesn't belong into 'struct td_context' -- it's specific to the
>> design for FFmpeg.
>
> If you need to refer to a different patch to explain something in
> another one that usually is a strong sign you split the patches in
> a bad way.

I disagree.

> A separate patch should contain exactly that amount of changes
> that you can reason about and argue for independently, not
> more and not less.

If possible yes, but not if it breaks the logic independence.

At least in git and linux I've seen many people mentioning "this would
help further patches".

By your logic a skeleton patch wouldn't make sense, because you cannot
argue that such patch makes sense on its own.

If you have a different policy, then I can merge all the changes in
tidsp_mpeg4.c into one patch, although I think that would be slightly
harder to review.

-- 
Felipe Contreras



More information about the ffmpeg-devel mailing list