flac dev - May 2013 - Bug fix and compatibility patches for 1.3.0pre4

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.

Robert
-------------- next part --------------
diff -Naur flac-1.3.0pre4/include/share/compat.h
flac-1.3.0pre4-patched/include/share/compat.h
--- flac-1.3.0pre4/include/share/compat.h	2013-04-07 11:02:38 +0000
+++ flac-1.3.0pre4-patched/include/share/compat.h	2013-05-04 11:59:54 +0000
@@ -50,8 +50,8 @@
 #include <sys/types.h> /* for off_t */
 #define FLAC__off_t __int64 /* use this instead of off_t to fix the 2 GB limit
*/
 #if !defined __MINGW32__
-#define fseeko _fseeki64
-#define ftello _ftelli64
+#define fseeko(f, p, o) _lseeki64((f)->_file, (p), (o))
+#define ftello(f) _lseeki64((f)->_file, 0, SEEK_CUR)
 #else /* MinGW */
 #if !defined(HAVE_FSEEKO)
 #define fseeko fseeko64
-------------- next part --------------
diff -Naur flac-1.3.0pre4/src/libFLAC/bitwriter.c
flac-1.3.0pre4-patched/src/libFLAC/bitwriter.c
--- flac-1.3.0pre4/src/libFLAC/bitwriter.c	2013-04-07 02:39:36 +0000
+++ flac-1.3.0pre4-patched/src/libFLAC/bitwriter.c	2013-05-03 13:54:21 +0000
@@ -857,6 +857,7 @@
  * 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);
diff -Naur flac-1.3.0pre4/src/libFLAC/include/private/bitmath.h
flac-1.3.0pre4-patched/src/libFLAC/include/private/bitmath.h
--- flac-1.3.0pre4/src/libFLAC/include/private/bitmath.h	2013-04-24 05:19:16
+0000
+++ flac-1.3.0pre4-patched/src/libFLAC/include/private/bitmath.h	2013-05-03
12:07:43 +0000
@@ -74,7 +74,7 @@
 {
 /* 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. */
-------------- next part --------------
diff -Naur flac-1.3.0pre4/src/libFLAC/stream_decoder.c
flac-1.3.0pre4-patched/src/libFLAC/stream_decoder.c
--- flac-1.3.0pre4/src/libFLAC/stream_decoder.c	2013-04-07 11:02:38 +0000
+++ flac-1.3.0pre4-patched/src/libFLAC/stream_decoder.c	2013-05-03 13:45:32
+0000
@@ -55,7 +55,7 @@
 
 
 /* technically this should be in an "export.c" but this is convenient
enough */
-#ifdef FLAC_API_SUPPORTS_OGG_FLAC
+#ifdef FLAC__HAS_OGG
 FLAC_API int FLAC_API_SUPPORTS_OGG_FLAC = FLAC__HAS_OGG ;
 #else
 FLAC_API int FLAC_API_SUPPORTS_OGG_FLAC = 0 ;

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.

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: OpenPGP digital signature
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20130505/e7754bf3/attachment.pgp

Timothy B. Terriberry wrote:
> _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.
Ok, I didn't take that into account. Thanks for pointing it out! Using 
fflush to clear the buffers before calling _lseeki64 sounds reasonable, 
so I think I'll try that.

I noticed another problem with _lseeki64 though. It returns the new file 
pointer position on success instead of 0, so the macro will have to take 
that into account.
> 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?
Those functions are not exported by XPs msvcrt.dll, so declaring them 
won't help.

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/

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.