Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Robert Kausch wrote: Hi all, I tried 1.3.0pre4 with ICL on Windows and found some issues. Not sure if this is the right place to submit patches, but someone suggested this on the apparently dead SourceForge patch tracker. The first two are quite straight forward: - The ICL patch fixes a typo in bitmath.h and adds FLAC__bitwriter_write_zeroes to the external declarations in bitwriter.c. - The Ogg patch replaces the check for FLAC_API_SUPPORTS_OGG_FLAC in stream_decoder.c with FLAC__HAS_OGG to fix compilation with Ogg support. The _lseeki64 patch probably is a little more controversial. The problem is that fseeki64 and ftelli64 are not available in Windows XP - at least not without installing extra MSVC runtime libraries. I changed compat.h and replaced them with calls to _lseeki64, which was available at least back to Windows 98 and thus doesn't impose such compatibility issues. However, the patch only represents my quick and dirty solution and you'll probably like to find a cleaner one. Maybe all calls to fseeko and ftello should be put in OS specific wrapper functions. Would love to see those patches in the 1.3.0 release. Sorry, I've read through this thread and can't figure out what was actually decided and which patch I should be looking at. Clues? Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
On 25.5.2013 10:54, Erik de Castro Lopo wrote: Robert Kausch wrote: Hi all, I tried 1.3.0pre4 with ICL on Windows and found some issues. Not sure if this is the right place to submit patches, but someone suggested this on the apparently dead SourceForge patch tracker. The first two are quite straight forward: - The ICL patch fixes a typo in bitmath.h and adds FLAC__bitwriter_write_zeroes to the external declarations in bitwriter.c. - The Ogg patch replaces the check for FLAC_API_SUPPORTS_OGG_FLAC in stream_decoder.c with FLAC__HAS_OGG to fix compilation with Ogg support. The _lseeki64 patch probably is a little more controversial. The problem is that fseeki64 and ftelli64 are not available in Windows XP - at least not without installing extra MSVC runtime libraries. I changed compat.h and replaced them with calls to _lseeki64, which was available at least back to Windows 98 and thus doesn't impose such compatibility issues. However, the patch only represents my quick and dirty solution and you'll probably like to find a cleaner one. Maybe all calls to fseeko and ftello should be put in OS specific wrapper functions. Would love to see those patches in the 1.3.0 release. Sorry, I've read through this thread and can't figure out what was actually decided and which patch I should be looking at. Clues? Erik None. The functions in use do not prevent the program from working on any operating system. Their usage only requires compiler to have the functions, and all Microsoft Visual C compilers since version 2005 have them. If there is will to support older compilers it should be in some MSVC version dependant macro, so that new compilers don't need hacks. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Janne Hyvärinen wrote: None. The functions in use do not prevent the program from working on any operating system. Their usage only requires compiler to have the functions, and all Microsoft Visual C compilers since version 2005 have them. If there is will to support older compilers it should be in some MSVC version dependant macro, so that new compilers don't need hacks. Awesome, thanks. Cheers, Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Janne Hyvärinen wrote:
On 25.5.2013 10:54, Erik de Castro Lopo wrote:
Robert Kausch wrote:
I tried 1.3.0pre4 with ICL on Windows and found some issues.
The first two are quite straight forward:
- The ICL patch fixes a typo in bitmath.h and adds
FLAC__bitwriter_write_zeroes to the external declarations in bitwriter.c.
- The Ogg patch replaces the check for FLAC_API_SUPPORTS_OGG_FLAC in
stream_decoder.c with FLAC__HAS_OGG to fix compilation with Ogg support.
The _lseeki64 patch probably is a little more controversial.
Sorry, I've read through this thread and can't figure out what was
actually decided and which patch I should be looking at.
None.
I think the first two patches should actually go in.
- The second patch absolutely, because it fixes an all-platform
regression. (Actually, slightly modified perhaps; or even better, we
revert this part to how it was in 1.2.1. Attaching a patch to do that.)
- The first patch addresses two different issues. I've split it into
two patches (also attached). One of them fixes a mistyped variable
name that will obviously cause the Intel compiler to fail. Regarding
the other, well, it is certainly consistent with the other external
declarations, and if it helps the Intel compiler, why not?
Ulrich
From 846e4d349062014eb6b960b0e16e80f63d8c691c Mon Sep 17 00:00:00 2001
From: Ulrich Klauer ulr...@chirlu.de
Date: Sat, 25 May 2013 14:14:08 +0200
Subject: [PATCH] Correctly initialize FLAC_API_SUPPORTS_OGG_FLAC
Commits a7e3705d051bafd1cae90f6605287cc1d9f2a18d and
a4c321e492748db0a7e38240699ba420ba88e01c, while trying to simplify how
the FLAC_API_SUPPORTS_OGG_FLAC global variable was initialized,
inadvertently caused it to be always set to false, whether Ogg support
was compiled in or not.
This commit reverts the relevant part to how it looked in the 1.2.1
release, which is verbose but correct.
The problem was found by Robert Kausch robert.kau...@freac.org.
---
src/libFLAC/stream_decoder.c |8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/libFLAC/stream_decoder.c b/src/libFLAC/stream_decoder.c
index 33be1e8..f4d2d39 100644
--- a/src/libFLAC/stream_decoder.c
+++ b/src/libFLAC/stream_decoder.c
@@ -55,11 +55,13 @@
/* technically this should be in an export.c but this is convenient enough */
-#ifdef FLAC_API_SUPPORTS_OGG_FLAC
-FLAC_API int FLAC_API_SUPPORTS_OGG_FLAC = FLAC__HAS_OGG ;
+FLAC_API int FLAC_API_SUPPORTS_OGG_FLAC =
+#if FLAC__HAS_OGG
+ 1
#else
-FLAC_API int FLAC_API_SUPPORTS_OGG_FLAC = 0 ;
+ 0
#endif
+;
/***
--
1.7.10.4
From 39ad5e721c715c1c2e9427ea128d7079912dc10d Mon Sep 17 00:00:00 2001
From: Robert Kausch robert.kau...@freac.org
Date: Sat, 25 May 2013 14:34:18 +0200
Subject: [PATCH 1/2] Fix mistyped variable name
---
src/libFLAC/include/private/bitmath.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libFLAC/include/private/bitmath.h b/src/libFLAC/include/private/bitmath.h
index 42ce639..e5c7695 100644
--- a/src/libFLAC/include/private/bitmath.h
+++ b/src/libFLAC/include/private/bitmath.h
@@ -74,7 +74,7 @@ static inline unsigned int FLAC__clz_uint32(FLAC__uint32 v)
{
/* Never used with input 0 */
#if defined(__INTEL_COMPILER)
-return _bit_scan_reverse(n) ^ 31U;
+return _bit_scan_reverse(v) ^ 31U;
#elif defined(__GNUC__) (__GNUC__ = 4 || (__GNUC__ == 3 __GNUC_MINOR__ = 4))
/* This will translate either to (bsr ^ 31U), clz , ctlz, cntlz, lzcnt depending on
* -march= setting or to a software rutine in exotic machines. */
--
1.7.10.4
From 98fd66be667cc5557ba18fbf83fb8bbc016b0095 Mon Sep 17 00:00:00 2001
From: Robert Kausch robert.kau...@freac.org
Date: Sat, 25 May 2013 14:37:44 +0200
Subject: [PATCH 2/2] bitwriter.c : Add missing extern declaration
---
src/libFLAC/bitwriter.c |1 +
1 file changed, 1 insertion(+)
diff --git a/src/libFLAC/bitwriter.c b/src/libFLAC/bitwriter.c
index 9b15aff..dd66e6d 100644
--- a/src/libFLAC/bitwriter.c
+++ b/src/libFLAC/bitwriter.c
@@ -857,6 +857,7 @@ FLAC__bool FLAC__bitwriter_zero_pad_to_byte_boundary(FLAC__BitWriter *bw)
* Unfortunately, the Microsoft VS compiler doesn't pick them up externally. To
* fix that we add extern declarations here.
*/
+extern FLAC__bool FLAC__bitwriter_write_zeroes(FLAC__BitWriter *bw, unsigned bits);
extern FLAC__bool FLAC__bitwriter_write_raw_int32(FLAC__BitWriter *bw, FLAC__int32 val, unsigned bits);
extern FLAC__bool FLAC__bitwriter_write_raw_uint64(FLAC__BitWriter *bw, FLAC__uint64 val, unsigned bits);
extern FLAC__bool FLAC__bitwriter_write_raw_uint32_little_endian(FLAC__BitWriter *bw, FLAC__uint32 val);
--
1.7.10.4
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
On 25.5.2013 15:42, Ulrich Klauer wrote: Janne Hyvärinen wrote: On 25.5.2013 10:54, Erik de Castro Lopo wrote: Robert Kausch wrote: I tried 1.3.0pre4 with ICL on Windows and found some issues. The first two are quite straight forward: - The ICL patch fixes a typo in bitmath.h and adds FLAC__bitwriter_write_zeroes to the external declarations in bitwriter.c. - The Ogg patch replaces the check for FLAC_API_SUPPORTS_OGG_FLAC in stream_decoder.c with FLAC__HAS_OGG to fix compilation with Ogg support. The _lseeki64 patch probably is a little more controversial. Sorry, I've read through this thread and can't figure out what was actually decided and which patch I should be looking at. None. I think the first two patches should actually go in. - The second patch absolutely, because it fixes an all-platform regression. (Actually, slightly modified perhaps; or even better, we revert this part to how it was in 1.2.1. Attaching a patch to do that.) - The first patch addresses two different issues. I've split it into two patches (also attached). One of them fixes a mistyped variable name that will obviously cause the Intel compiler to fail. Regarding the other, well, it is certainly consistent with the other external declarations, and if it helps the Intel compiler, why not? Ulrich ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev Ah indeed, I have nothing against those. I should have read all the quoted text. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Ulrich Klauer wrote: I think the first two patches should actually go in. - The second patch absolutely, because it fixes an all-platform regression. (Actually, slightly modified perhaps; or even better, we revert this part to how it was in 1.2.1. Attaching a patch to do that.) - The first patch addresses two different issues. I've split it into two patches (also attached). One of them fixes a mistyped variable name that will obviously cause the Intel compiler to fail. Regarding the other, well, it is certainly consistent with the other external declarations, and if it helps the Intel compiler, why not? All three applied. Thanks. Erik -- -- Erik de Castro Lopo http://www.mega-nerd.com/ ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Timothy B. Terriberry wrote: The pos value is stored in an internal format and is intended for use only by *fgetpos* and *fsetpos*. FWIW, I verified that this is the approach used by mingw32 to implement fseeko/ftello. Yes, they do - I also had a look at the libmingwex source. But still, with that sentence Microsoft basically reserves the right to change the behaviour of fgetpos/fsetpos at any time without notice... I wrote a small test program in the meantime and verified that fgetpos/fsetpos indeed work, even back to Windows 98. _lseeki64 on the other hand totally messes up with buffered streams and cannot be used here. Calling fflush beforehand doesn't help and even fflush alone breaks the stream (i.e. with 4kB buffers, fopen()-fread()-fflush()-fread() will start at byte #4096 with the second read, so don't use fflush for reading). However, as Janne already pointed out, libFLAC is statically linked by default, so there's no real problem with _fseeki64/_ftelli64 indeed. I'll still have to decide whether to link statically or use your fgetpos/fsetpos patch for my build, but no need to change anything in the official FLAC code. In any case, please disregard my _lseeki64 patch. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Janne Hyvärinen wrote: Oldest Visual Studio supported by FLAC 1.3 is Visual Studio 2005. FLAC is also configured to be compiled with static linking, so no external dependencies hinder its function. Ok. Thanks for pointing this out! I use a custom build setup and link dynamically against msvcrt.dll. That's why I came across that problem and didn't realize that it would not affect most other people. Sorry for causing such a stir over this! There's one problem with static linkage and some of the FLAC API functions, though, that actually was the reason I linked FLAC against msvcrt.dll in the first place. The metadata object functions can be used in a memory ownership transferring manner. Doing so will cause problems when the calling EXE and the FLAC DLL use different CRTs as the DLL's CRT will not know about memory allocated by the EXE's CRT. However, the same happens when you mix msvcrt.dll and msvcr??.dll, so it's not limited to static linkage only. It's getting a bit off topic here, though... ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Robert Kausch wrote: msvcrt.dll in the first place. The metadata object functions can be used in a memory ownership transferring manner. Doing so will cause problems when the calling EXE and the FLAC DLL use different CRTs as the DLL's Sigh, I thought we'd finally gotten rid of most of this kind of API brokenness from the Xiph libraries. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
A few comments. 1) MSVCRT.DLL hasn't been used in ages. Each version of visual studio has its own C Runtime Library version, with a naming convention of msvcr##.dll. It's safe to use _fseeki64 and _ftelli64. Leave it up to the developer-user of FLAC to determine whether to statically link or dynamically link. 2) Yes, the copy flag in the Metadata API is broken. The APIs that take FILE * objects as parameter are similarly broken (e.g. FLAC__stream_decoder_init_FILE). There's absolutely no guarantee that a developer is using the same C library that was used to compile libFLAC. 3) For Windows C runtime libraries that are missing 64-bit seek/tell, we can fall back to ftell and fseek and encourage those developers to use FLAC__stream_decoder_init_stream instead. This is also the correct way for developers to use platform-specific I/O (CreateFile, etc). -Ben Allison Janne Hyvärinen wrote: Oldest Visual Studio supported by FLAC 1.3 is Visual Studio 2005. FLAC is also configured to be compiled with static linking, so no external dependencies hinder its function. Ok. Thanks for pointing this out! I use a custom build setup and link dynamically against msvcrt.dll. That's why I came across that problem and didn't realize that it would not affect most other people. Sorry for causing such a stir over this! There's one problem with static linkage and some of the FLAC API functions, though, that actually was the reason I linked FLAC against msvcrt.dll in the first place. The metadata object functions can be used in a memory ownership transferring manner. Doing so will cause problems when the calling EXE and the FLAC DLL use different CRTs as the DLL's CRT will not know about memory allocated by the EXE's CRT. However, the same happens when you mix msvcrt.dll and msvcr??.dll, so it's not limited to static linkage only. It's getting a bit off topic here, though... ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
On 5.5.2013 18:02, Timothy B. Terriberry wrote: Instead I've attached a patch that uses fgetpos/fsetpos. This is totally untested (I haven't even checked it compiles), but the idea should work. You people do realize these hacks would only be required for 10+ year old obsolete compilers? ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
JonY wrote: How about just forgetting about base XP and require at least SP2 or some such? Alternatively, use win32api underneath instead, eg CreateFileW/SetFilePointer. Even SP2 and SP3 do not have fseeki64/ftelli64. Using Windows API functions would probably be the cleanest solution, but on the other hand require wrappers for all file IO functions. I guess that would be too big of a change to make it into 1.3.0 at this point. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
On 6.5.2013 0:43, Timothy B. Terriberry wrote: Janne Hyvärinen wrote: You people do realize these hacks would only be required for 10+ year old obsolete compilers? No, they're required for easy distribution on 12 year old OSes (which, last I saw, make up almost 40% of Firefox's desktop userbase, and likely will continue to for some time). What kind of nonsense is this? You should know that the last Microsoft compiler to create dynamically linked code that used msvcrt.dll was Visual Studio 6.0 from 1998. Oldest Visual Studio supported by FLAC 1.3 is Visual Studio 2005. FLAC is also configured to be compiled with static linking, so no external dependencies hinder its function. If you take a look at the following MSDN pages for Visual Studio 2005, you will see that _fseeki64 and _ftelli64 are supported all the way back to Windows 95: http://msdn.microsoft.com/en-us/library/75yw9bf3%28v=vs.80%29.aspx and http://msdn.microsoft.com/en-US/library/0ys3hc0b%28v=vs.80%29.aspx ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
Robert Kausch wrote: The _lseeki64 patch probably is a little more controversial. The problem is that fseeki64 and ftelli64 are not available in Windows XP - at least not without installing extra MSVC runtime libraries. I changed compat.h and replaced them with calls to _lseeki64, which was available at least back to Windows 98 and thus doesn't impose such compatibility issues. _lseeki64 operates on the underlying file handle, and does not interact well with the buffering in stdio streams. I _think_ you can use this successfully if you call fflush() beforehand (as this sets FILE::_cnt to 0 and FILE::_ptr to FILE::_base). By which I mean I read the MSVCRT source from MSVC6.0 and it appears this is how things work. That source also includes an fseeki64()/ftelli64(), but they are not defined in stdio.h. I wonder if just declaring it yourself is good enough? If not, they get called by fsetpos()/fgetpos() (which _do_ interact correctly with the buffering in stdio streams), except when _MAC is defined (which I presume is not common). I don't actually have XP to test against. ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
Re: [flac-dev] Bug fix and compatibility patches for 1.3.0pre4
On 5/5/2013 09:03, Timothy B. Terriberry wrote: Robert Kausch wrote: The _lseeki64 patch probably is a little more controversial. The problem is that fseeki64 and ftelli64 are not available in Windows XP - at least not without installing extra MSVC runtime libraries. I changed compat.h and replaced them with calls to _lseeki64, which was available at least back to Windows 98 and thus doesn't impose such compatibility issues. _lseeki64 operates on the underlying file handle, and does not interact well with the buffering in stdio streams. I _think_ you can use this successfully if you call fflush() beforehand (as this sets FILE::_cnt to 0 and FILE::_ptr to FILE::_base). By which I mean I read the MSVCRT source from MSVC6.0 and it appears this is how things work. How about just forgetting about base XP and require at least SP2 or some such? Alternatively, use win32api underneath instead, eg CreateFileW/SetFilePointer. That source also includes an fseeki64()/ftelli64(), but they are not defined in stdio.h. I wonder if just declaring it yourself is good enough? If not, they get called by fsetpos()/fgetpos() (which _do_ interact correctly with the buffering in stdio streams), except when _MAC is defined (which I presume is not common). I don't actually have XP to test against. Bad, do no declare manually, I had to cleanup some bad code recently that make assumptions about your header declarations. You can try using GetModuleHandle/GetProcAddress instead, but msvcr* versions are a whole different can of worms. signature.asc Description: OpenPGP digital signature ___ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
