Erik de Castro Lopo wrote:> lvqcl, > > Would you be able to have alook at this one? I think its > Windows related: > > https://sourceforge.net/p/flac/feature-requests/114/ >The relevant changes are <http://git.xiph.org/?p=flac.git;a=commitdiff;h=6a6207b52a86b1d7980a5233e297c0fc948bed7d> and <http://git.xiph.org/?p=flac.git;a=commitdiff;h=e8632477774f56b4fe7ccab525cad2ceab244b8a> the current code: #ifdef _WIN32 /* * Windows can suffer quite badly from disk fragmentation. This can be * reduced significantly by setting the output buffer size to be 10MB. */ setvbuf(file, NULL, _IOFBF, 10*1024*1024); #endif LRN <lrn1986 at gmail.com> wrote:> The commit mentioned in the feature request should not cause such > behaviour, as it only does short-lived operations (opens a file, does > stuff, closes the file immediately after) and is clearly distinguishing > between disk files and pipes (which is how you, presumably, stream data to > other processes for whatever reasons). > > I don't claim that FLAC doesn't do buffering, as the OP described, just > that this commit is unlikely to be the cause.Maybe you mean some other commit? For example, <http://git.xiph.org/?p=flac.git;a=commitdiff;h=d66f6754bf94bc8ba23d3579d0b5650cd380c9f0> ? Because setvbuf() should definitely change libFLAC behaviour regardless of files/pipes/etc. The attached patch *should* resolve the issue. libFLAC will call setvbuf(file, ...) only if GetFileType(...file...) == FILE_TYPE_DISK. -------------- next part -------------- A non-text attachment was scrubbed... Name: buffering.patch Type: application/octet-stream Size: 615 bytes Desc: not available Url : http://lists.xiph.org/pipermail/flac-dev/attachments/20151210/c3bffdd5/attachment.obj
On 12/10/2015 5:58 PM, lvqcl wrote:> Erik de Castro Lopo wrote: >> lvqcl, >> >> Would you be able to have alook at this one? I think its >> Windows related: >> >> https://sourceforge.net/p/flac/feature-requests/114/ >> > > The relevant changes are > <http://git.xiph.org/?p=flac.git;a=commitdiff;h=6a6207b52a86b1d7980a5233e297c0fc948bed7d> > and > <http://git.xiph.org/?p=flac.git;a=commitdiff;h=e8632477774f56b4fe7ccab525cad2ceab244b8a> > > > the current code: > > #ifdef _WIN32 > /* > * Windows can suffer quite badly from disk fragmentation. This > can be > * reduced significantly by setting the output buffer size to be > 10MB. > */ > setvbuf(file, NULL, _IOFBF, 10*1024*1024); > #endif > > > LRN <lrn1986 at gmail.com> wrote: > >> The commit mentioned in the feature request should not cause such >> behaviour, as it only does short-lived operations (opens a file, does >> stuff, closes the file immediately after) and is clearly distinguishing >> between disk files and pipes (which is how you, presumably, stream >> data to >> other processes for whatever reasons). >> >> I don't claim that FLAC doesn't do buffering, as the OP described, just >> that this commit is unlikely to be the cause. > > Maybe you mean some other commit? For example, > <http://git.xiph.org/?p=flac.git;a=commitdiff;h=d66f6754bf94bc8ba23d3579d0b5650cd380c9f0> > ? > Because setvbuf() should definitely change libFLAC behaviour > regardless of files/pipes/etc. > > > The attached patch *should* resolve the issue. libFLAC will call > setvbuf(file, ...) > only if GetFileType(...file...) == FILE_TYPE_DISK.I probably don't know enough about the intricates of Win32, but why not prefer this for clarity: diff --git a/src/libFLAC/stream_encoder.c b/src/libFLAC/stream_encoder.c index 203a271..0394c26 100644 --- a/src/libFLAC/stream_encoder.c +++ b/src/libFLAC/stream_encoder.c @@ -1330,7 +1330,8 @@ static FLAC__StreamEncoderInitStatus init_FILE_internal_( * Windows can suffer quite badly from disk fragmentation. This can be * reduced significantly by setting the output buffer size to be 10MB. */ - setvbuf(file, NULL, _IOFBF, 10*1024*1024); + if(file != stdout) + setvbuf(file, NULL, _IOFBF, 10*1024*1024); #endif encoder->private_->file = file; Does libFLAC support other output types except for pipes/stdout and files? Also, if you want to be really evil you could even just use *else* and have the #ifdef _WIN32 block depend on the relative position to the if above...> > > _______________________________________________ > 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/20151210/183eacb5/attachment.htm
Christoph Terasa wrote:>> The attached patch *should* resolve the issue. libFLAC will call >> setvbuf(file, ...) >> only if GetFileType(...file...) == FILE_TYPE_DISK. > I probably don't know enough about the intricates of Win32, but why not > prefer this for clarity:> - setvbuf(file, NULL, _IOFBF, 10*1024*1024); > + if(file != stdout) > + setvbuf(file, NULL, _IOFBF, 10*1024*1024); > #endif > encoder->private_->file = file; > > Does libFLAC support other output types except for pipes/stdout and files?But stdout is not the only existing pipe. A program that uses libFLAC can create some other pipe (e.g. via popen()) and then call FLAC__stream_encoder_init_FILE().> Also, if you want to be really evil you could even just use *else* and > have the #ifdef _WIN32 block depend on the relative position to the if > above...I don't understand what you mean.
On 10.12.2015 19:58, lvqcl wrote:> LRN wrote: > >> The commit mentioned in the feature request should not cause such >> behaviour, as it only does short-lived operations (opens a file, does >> stuff, closes the file immediately after) and is clearly distinguishing >> between disk files and pipes (which is how you, presumably, stream data to >> other processes for whatever reasons). >> >> I don't claim that FLAC doesn't do buffering, as the OP described, just >> that this commit is unlikely to be the cause. > > Maybe you mean some other commit? For example, > <http://git.xiph.org/?p=flac.git;a=commitdiff;h=d66f6754bf94bc8ba23d3579d0b5650cd380c9f0> ?Yes, that is exactly what i meant.> > The attached patch *should* resolve the issue. libFLAC will call setvbuf(file, ...) > only if GetFileType(...file...) == FILE_TYPE_DISK.That patch looks correct. That said, i'm not sure why there's setvbuf() in the first place. I mean, the code to de-fragment the output file already exists (the aforementioned d66f675), so why buffering? Was it proven empirically that buffering is required? Or am i looking at this backwards and that code was added later than setvbuf()? -- O< ascii ribbon - stop html email! - www.asciiribbon.org -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: OpenPGP digital signature Url : http://lists.xiph.org/pipermail/flac-dev/attachments/20151210/58e8f466/attachment-0001.pgp
LRN wrote:>> Maybe you mean some other commit? For example, >> <http://git.xiph.org/?p=flac.git;a=commitdiff;h=d66f6754bf94bc8ba23d3579d0b5650cd380c9f0> ? > Yes, that is exactly what i meant. > >> >> The attached patch *should* resolve the issue. libFLAC will call setvbuf(file, ...) >> only if GetFileType(...file...) == FILE_TYPE_DISK. > > That patch looks correct. That said, i'm not sure why there's setvbuf() in > the first place. I mean, the code to de-fragment the output file already > exists (the aforementioned d66f675), so why buffering? Was it proven > empirically that buffering is required? Or am i looking at this backwards > and that code was added later than setvbuf()?SetFilePointerEx() for _de_coding (e.g. when flac writes .wav file), setvbuf() for _en_coding (when libFLAC writes .flac file).
lvqcl wrote:> The attached patch *should* resolve the issue. libFLAC will call setvbuf(file, ...) > only if GetFileType(...file...) == FILE_TYPE_DISK.Applied. Thanks. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/