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

[Michael Niedermayer]

On Tue, Sep 07, 2010 at 09:36:38PM +0200, Michael Niedermayer wrote:
> 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.

this of course assumes that the 2 patches adding a field each should
be seperate to begin with ...
[..]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/2958b704/attachment.pgp>