Re: [PATCH 01/13] dsound: New sample rate converter core functions.

[Krzysztof Nikiel]

2011/2/11 Dmitry Timoshkov :
> Krzysztof Nikiel  wrote:
> > > You can't send Makefile changes separately from added/removed
> > > files, a patch should not add dead code.
> >
> > Could you explain "dead code", all 13 parts need to be applied,
> > otherwise the code will be broken.
>
> Wine should compile and be able to pass 'make test' after each separate patch.
> You can't add for instance dlls/dsound/resample.c in one patch, and add it to
> Makefile or use interfaces provided by it in some later patches. Every patch
> should be finished and self-containing.

Well, previous version of this patch was rejected as "needs
splitting", it's basically too big to be send as a single patch. It
can be applied as several smaller chunks or rejected as a whole. I
don't think there is any other option.

[PATCH 01/13] dsound: New sample rate converter core functions.

[Ricardo Filipe]

2011/2/11 Krzysztof Nikiel
> 2011/2/11 Dmitry Timoshkov :
> > Krzysztof Nikiel  wrote:
> > > > You can't send Makefile changes separately from added/removed
> > > > files, a patch should not add dead code.
> > >
> > > Could you explain "dead code", all 13 parts need to be applied,
> > > otherwise the code will be broken.
> >
> > Wine should compile and be able to pass 'make test' after each separate
>
> patch.
> > You can't add for instance dlls/dsound/resample.c in one patch, and add
>
> it to
> > Makefile or use interfaces provided by it in some later patches. Every
>
> patch
> > should be finished and self-containing.
>
> Well, previous version of this patch was rejected as "needs
> splitting", it's basically too big to be send as a single patch. It
> can be applied as several smaller chunks or rejected as a whole. I
> don't think there is any other option.

still, what dmitry told you still holds. wine needs to be able to compile
and pass the tests after each patch is applied. at least the makefile
changes should be merged with the previous patches that needs them (1 and
2). the rest seems like fine splitting, although i don't know the code.
what i'm not sure is, if resampler is still not called after the first
patches, if it is fine to send those first patches... i think you also need
to call resampler for the patch to be accepted, even if it is just as a
stub, incomplete version.

[PATCH 01/13] dsound: New sample rate converter core functions.

[Krzysztof Nikiel]

2011/2/11 Ricardo Filipe :
> 2011/2/11 Krzysztof Nikiel
> > 2011/2/11 Dmitry Timoshkov :
> > > Krzysztof Nikiel  wrote:
> > > > > You can't send Makefile changes separately from added/removed
> > > > > files, a patch should not add dead code.
> > > >
> > > > Could you explain "dead code", all 13 parts need to be applied,
> > > > otherwise the code will be broken.
> > >
> > > Wine should compile and be able to pass 'make test' after each separate
> > > patch.
> > > You can't add for instance dlls/dsound/resample.c in one patch, and add
> > > it to
> > > Makefile or use interfaces provided by it in some later patches. Every
> > > patch
> > > should be finished and self-containing.
> >
> > Well, previous version of this patch was rejected as "needs
> > splitting", it's basically too big to be send as a single patch. It
> > can be applied as several smaller chunks or rejected as a whole. I
> > don't think there is any other option.
>
> still, what dmitry told you still holds. wine needs to be able to compile
> and pass the tests after each patch is applied. at least the makefile
> changes should be merged with the previous patches that needs them (1 and
> 2). the rest seems like fine splitting, although i don't know the code.
> what i'm not sure is, if resampler is still not called after the first
> patches, if it is fine to send those first patches... i think you also need
> to call resampler for the patch to be accepted, even if it is just as a
> stub, incomplete version.

It's not that easy, I don't think it would be just incomplete, it
would be broken.
The original code is rather messy, especially mixer.c, no wonder no
one wanted to touch it to fix the really poor sample rate conversion.

[PATCH 01/13] dsound: New sample rate converter core functions.

[Maarten Lankhorst]

Hi Krzysztof,

Op 11-02-11 12:16, Krzysztof Nikiel schreef:
> 2011/2/11 Dmitry Timoshkov:
> > Krzysztof Nikiel  wrote:
> > > > You can't send Makefile changes separately from added/removed
> > > > files, a patch should not add dead code.
> > >
> > > Could you explain "dead code", all 13 parts need to be applied,
> > > otherwise the code will be broken.
> >
> > Wine should compile and be able to pass 'make test' after each separate patch.
> > You can't add for instance dlls/dsound/resample.c in one patch, and add it to
> > Makefile or use interfaces provided by it in some later patches. Every patch
> > should be finished and self-containing.
>
> Well, previous version of this patch was rejected as "needs
> splitting", it's basically too big to be send as a single patch. It
> can be applied as several smaller chunks or rejected as a whole. I
> don't think there is any other option.

Thanks for looking at the format conversion functions. The splitting can 
probably be done in 4 parts:
1. If you have any generic bugfixes, put them in the first few patches. 
Once those are in you can look at the other parts. Each bugfix should 
probably be its own separate patch.
2. Single patch to remove the old behavior. At this point you probably 
only want the mixing to work without requiring reconversion, and have a 
FIXME or something to say rate conversion is unavailable.
3. A bunch of patches implementing new rate conversion, one logical step 
at a time. The only requirement is that you want to be able to build 
wine without it failing, and if possible even running directsound, even 
if the result is insane..

Having mkfir in the way you put it there will probably not work, since 
some people will build wine out of tree. My guess is that you should put 
that in tools. Since the contents of the file won't change often it may 
just be enough to have the .c file there and have a note in the mkfir.h 
header that it was generated by tools/mkfir.c, but I'm not AJ so he 
would have to decide.

The reason for this kind of splitting is that even if the code is broken 
between the removal of the old code and the last patch, it's a lot 
easier to see bugs. Usually in a patch like this you will spot 2 or 3 
bugs you wouldn't have otherwise spotted because you look at the code in 
logical parts. This also makes bisecting a lot easier. Otherwise you 
know what commit caused the regression, but you have no idea what part 
of the code is.

Cheers,
Maarten

[PATCH 01/13] dsound: New sample rate converter core functions.

[Krzysztof Nikiel]

2011/2/11 Maarten Lankhorst :
> > Well, previous version of this patch was rejected as "needs
> > splitting", it's basically too big to be send as a single patch. It
> > can be applied as several smaller chunks or rejected as a whole. I
> > don't think there is any other option.
>
> Thanks for looking at the format conversion functions. The splitting can
> probably be done in 4 parts:
> - If you have any generic bugfixes, put them in the first few patches. Once
>
> those are in you can look at the other parts. Each bugfix should probably be
> its own separate patch.
> 2. Single patch to remove the old behavior. At this point you probably only
> want the mixing to work without requiring reconversion, and have a FIXME or
> something to say rate conversion is unavailable.
> 3. A bunch of patches implementing new rate conversion, one logical step at
> a time. The only requirement is that you want to be able to build wine
> without it failing, and if possible even running directsound, even if the
> result is insane..

Thanks, now I have at least two reasonable advices how to proceed with
the split.
It looks like the actual split may be more difficult to make than the
patch itself :P
> Having mkfir in the way you put it there will probably not work, since some
> people will build wine out of tree. My guess is that you should put that in
> tools. Since the contents of the file won't change often it may just be
> enough to have the .c file there and have a note in the mkfir.h header that
> it was generated by tools/mkfir.c, but I'm not AJ so he would have to
> decide.

Yes, 'tools' seems a good place to put it. I think if someone is
really inclined to know where 'firtab.h' comes from it won't be a real
problem to find it.
I'm not sure what "out of tree" means but I think it does work out of
tree. Anyway, if you like the unwrapped version, just visit the
bugzilla and download the .zip pack.

http://bugs.winehq.org/show_bug.cgi?id=14717
> The reason for this kind of splitting is that even if the code is broken
> between the removal of the old code and the last patch, it's a lot easier to
> see bugs. Usually in a patch like this you will spot 2 or 3 bugs you
> wouldn't have otherwise spotted because you look at the code in logical
> parts. This also makes bisecting a lot easier. Otherwise you know what
> commit caused the regression, but you have no idea what part of the code is.

I expected it will be a problematic patch. It's quite big and the
original code is rather messy, especially mixer.c.
I really doubt the patch will be easy to read even when split as you say.
> Cheers,
> Maarten