Josh Coalson
2012-Apr-25 23:26 UTC
[flac-dev] [Flac-dev] Git branch with compiling fixes for win32
(Jumping in again, maybe at the wrong point since this doesn't seem to involve encoding, but here goes.) Miroslav's patches have always been high-quality for sure.? But regardless of submitter, any patch that affects encoding must be reviewed very carefully, preferably by several other people and definitely me.? If there were ever a libFLAC release that had a bug and was not always lossless, that would be very damaging to the format. Miroslav, sorry for dropping the ball on your patches; if I wasn't able to review properly then I didn't put it in, just out of caution.? Somewhere in my mailbox they're still there waiting to be looked at :) ? Also, please keep an eye if you can on critical commits because you always were a good reviewer too.>________________________________ > From: Erik de Castro Lopo <mle+la at mega-nerd.com> >To: flac-dev at xiph.org >Sent: Friday, February 10, 2012 1:39 AM >Subject: Re: [flac-dev] [Flac-dev] Git branch with compiling fixes for win32 > >Miroslav Lichvar wrote: > >> I'd like to see the following patch included. It's not trivial, but I >> think it's very well tested after those years. >> http://www.mail-archive.com/flac-dev at xiph.org/msg00914.html >> >> An updated version of the patch which includes some other >> optimizations is here. >> http://pkgs.fedoraproject.org/gitweb/?p=flac.git;a=tree > >I've had a look at that, but it doesn't apply to current git >head and I'd like to have a bit of an explanation of what it >does and why. > >> There are other patches which I think you might find useful: -asm, >> -gcc43 and -hidesyms. > >Likewise for those. > >> I can prepare proper git patches, if interested. > >Patch with an explanation of what and why would be awesome. > >Current git head is here: > >? ? https://git.xiph.org/?p=flac.git;a=summary > >Cheers, >Erik >-- >---------------------------------------------------------------------- >Erik de Castro Lopo >http://www.mega-nerd.com/ >_______________________________________________ >flac-dev mailing list >flac-dev at xiph.org >http://lists.xiph.org/mailman/listinfo/flac-dev > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: http://lists.xiph.org/pipermail/flac-dev/attachments/20120425/2bdaea8c/attachment-0001.htm
Erik de Castro Lopo
2012-Apr-25 23:42 UTC
[flac-dev] [Flac-dev] Git branch with compiling fixes for win32
Josh Coalson wrote:> But regardless of submitter, any patch that affects encoding must be > reviewed very carefully, preferably by several other people and definitely > me.Is there any way of encoding this manual review process in the test suite so that people hacking on FLAC can immediately see that soemthing is wrong before even attempting to submit a patch (assuming they run the test suite)? Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Josh Coalson
2012-Apr-26 00:11 UTC
[flac-dev] [Flac-dev] Git branch with compiling fixes for win32
From: Erik de Castro Lopo <mle+la at mega-nerd.com>>To: flac-dev at xiph.org >Cc: Josh Coalson <xflac at yahoo.com> >Sent: Wednesday, April 25, 2012 4:42 PM >Subject: Re: [flac-dev] [Flac-dev] Git branch with compiling fixes for win32 > >Josh Coalson wrote: > >> But regardless of submitter, any patch that affects encoding must be >> reviewed very carefully, preferably by several other people and definitely >> me. > >Is there any way of encoding this manual review process in the test suite >so that people hacking on FLAC can immediately see that soemthing is wrong >before even attempting to submit a patch (assuming they run the test suite)? > >ErikPart of the reason the current test suite is so long is to try and discover those problems automatically.? But it's not possible to be exhaustive simply because new code may not be covered by the test suite. Encoder bugs can be very subtle and sometimes it takes someone with good knowledge of the format to notice.
Miroslav Lichvar
2012-May-03 16:19 UTC
[flac-dev] [Flac-dev] Git branch with compiling fixes for win32
Hi Josh, nice to see you here again. On Wed, Apr 25, 2012 at 04:26:05PM -0700, Josh Coalson wrote:> (Jumping in again, maybe at the wrong point since this doesn't seem > to involve encoding, but here goes.) > > Miroslav's patches have always been high-quality for sure.? But > regardless of submitter, any patch that affects encoding must be > reviewed very carefully, preferably by several other people and > definitely me.? If there were ever a libFLAC release that had a bug > and was not always lossless, that would be very damaging to the > format.The bitreader patch touches only the rice decoding code which I believe is very well covered by the test suite and any bugs would be quickly seen. Also, it has also been included in the Fedora packages for several years, no bug reports about MD5 mismatch were received yet :). It makes the C function faster than the corresponding asm routine, so if it's included I'd suggest to just drop the asm function to not keep around more asm code than is necessary. I'm not sure if anyone is planning to port the asm code to x86_64, I think that it will be quite a lot of work, perhaps it would be a good time to reconsider using inline assembly instead of nasm to minimize the amount of asm code? It would be useful to know how much are the individual asm routines actually faster, it has been a long time since I played with it. -- Miroslav Lichvar
Cristian RodrÃguez
2012-May-04 15:13 UTC
[flac-dev] [Flac-dev] Git branch with compiling fixes for win32
El 03/05/12 12:19, Miroslav Lichvar escribi?:> Hi Josh, > > nice to see you here again. > > On Wed, Apr 25, 2012 at 04:26:05PM -0700, Josh Coalson wrote: >> (Jumping in again, maybe at the wrong point since this doesn't seem >> to involve encoding, but here goes.) >> >> Miroslav's patches have always been high-quality for sure. But >> regardless of submitter, any patch that affects encoding must be >> reviewed very carefully, preferably by several other people and >> definitely me. If there were ever a libFLAC release that had a bug >> and was not always lossless, that would be very damaging to the >> format. > > The bitreader patch touches only the rice decoding code which I > believe is very well covered by the test suite and any bugs would be > quickly seen. Also, it has also been included in the Fedora packages > for several years, no bug reports about MD5 mismatch were received > yet :). > > It makes the C function faster than the corresponding asm routine, so > if it's included I'd suggest to just drop the asm function to not keep > around more asm code than is necessary. > > I'm not sure if anyone is planning to port the asm code to x86_64, I > think that it will be quite a lot of work, perhaps it would be a good > time to reconsider using inline assembly instead of nasm to minimize > the amount of asm code? It would be useful to know how much are the > individual asm routines actually faster, it has been a long time > since I played with it. >Hi: Both Erick and I did already submitted patches to the tree that do just exactly what your flac-1.2.1-bitreader.patch intended.. please checkout current GIT tree. Cheers!
Cristian RodrÃguez
2012-May-04 15:22 UTC
[flac-dev] [Flac-dev] Git branch with compiling fixes for win32
El 03/05/12 12:19, Miroslav Lichvar escribi?:> It makes the C function faster than the corresponding asm routine, so > if it's included I'd suggest to just drop the asm function to not keep > around more asm code than is necessary.With current compilers it is very likely that those routines are already superflous.