flac dev - May 2012 - 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

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/

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)?
>
>Erik

Part 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.

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

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!

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.