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

[Michael Niedermayer]

On Tue, Sep 07, 2010 at 10:33:08PM +0300, Felipe Contreras wrote:
> 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.

Strictly and in most pedantic way the struct should only have been
introduced once it becomes needed (that is a second field is added)
(and the struct introduction could have been a seperate patch ...)

that way each patch would be free of simplifyable code.

iam not suggesting that things are redone that way though because its
really a waste of time and much more important things could be done with
that time


[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100907/551cda0c/attachment.pgp>