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

Felipe Contreras felipe.contreras
Wed Sep 8 00:50:20 CEST 2010


On Tue, Sep 7, 2010 at 10:49 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> 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:
>> > 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".
>
> Usually those are design changes, where the "help further patches" is just
> a shorthand for "our design was not future-safe enough, so we need to
> replace it"

Not really, sometimes it's just a matter of code reorganization; the
second patch looks too convoluted without the first patch that
cleanups/reorganizes the code, or even sometimes remove code (if the
changes are too drastic). The first patch might look like unnecessary,
or even detrimental if you consider it in itself, but it has a reason
to exist: to make the second patch easy to review.

>> By your logic a skeleton patch wouldn't make sense, because you cannot
>> argue that such patch makes sense on its own.
>
> Well, I think it not only makes no sense but is likely to be confusing
> to anyone doing regression testing. Having intentionally broken code
> in SVN is not so great.

I can set the id to CODEC_ID_NONE if that's an issue, and only change
it on the last patch.

>> 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.
>
> I haven't looked at it enough to know which is more reviewable, so
> I don't intend to request such a change.
> I consider it highly likely there's a better way to split it, however
> I guess there are more important, higher-level changes necessary anyway
> (most importantly how to handle the shared code), so no point quibbling
> about that now.

All right. I have no problem changing the patches in a way that's more
friendly to the eyes of FFmpeg developers, but yeah, it would be nice
to have more substantial changes first.

-- 
Felipe Contreras



More information about the ffmpeg-devel mailing list