GNOME Bugzilla – Bug 584455
[flacenc] sometimes writes broken flac files
Last modified: 2009-07-31 22:35:30 UTC
I've been able to consistently reproduce this on master (0.10.15.1) with: gst-launch -v filesrc location=/home/thomas/gst/media/medium/sugar.ogg ! oggdemux ! vorbisdec ! audioconvert ! audio/x-raw-int,width=16,depth=16 ! flacenc ! filesink location=test.flac; flac -d test.flac -f The output from flac -d is: test.flac: ERROR: bits per sample is 32, must be 4-24 Tim has been able to reproduce this, but only using filesrc; when he tried with httpsrc it didn't work. After bisecting, I found the following patch responsible: df707c666433a78d3878af6f055698d5756226c4 is first bad commit commit df707c666433a78d3878af6f055698d5756226c4 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Sun Aug 3 12:23:49 2008 +0000 ext/flac/gstflacenc.c: Set an estimate for the total number of samples that will be encoded if possible to help decod... Original commit message from CVS: * ext/flac/gstflacenc.c: (gst_flac_enc_query_peer_total_samples), (gst_flac_enc_sink_setcaps), (gst_flac_enc_write_callback): Set an estimate for the total number of samples that will be encoded if possible to help decoders if the streaminfo can't be rewritten later (like when muxing into Ogg containers). Add a warning if we get header packets after data packets as those will get lost when muxing into Ogg, i.e. rewriting the headers doesn't work. :100644 100644 cc564bd510602fb58d99c05a304b3ca588385821 890ca15411e027cdac7f026cce6b4762ca7ad806 M ChangeLog :040000 040000 6e346457b1c979c4f3b2dcc72bf4e7eba84415f2 20d0a1ec8792223b02d31c1ebf6e3d8b4b4f47d5 M ext
Generating the test file, the difference seems to be exactly one byte in the header: test.df707c666433a78d3878af6f055698d5756226c4.flac 0000 0000: 66 4C 61 43 00 00 00 22 12 00 12 00 00 20 95 00 fLaC..." ..... .. 0000 0010: 3C 9C 05 62 3B F0 00 38 DF C0 CD A8 B5 66 6F B6 <..b;..8 .....fo. 0000 0020: CC 54 26 DC 89 F0 D1 52 0E 21 84 00 00 28 20 00 .T&....R .!...( . 0000 0030: 00 00 72 65 66 65 72 65 6E 63 65 20 6C 69 62 46 ..refere nce libF 0000 0040: 4C 41 43 20 31 2E 32 2E 31 20 32 30 30 37 30 39 LAC 1.2. 1 200709 0000 0050: 31 37 00 00 00 00 FF F8 56 18 00 2C 4E 00 1A 00 17...... V..,N... 0000 0060: 4F 00 22 FF F1 FF FE 00 0B 00 2B 00 44 B5 3F 55 O."..... ..+.D.?U 0000 0070: 81 0A CF 26 99 4E 52 10 37 B2 06 DB D0 32 73 A5 ...&.NR. 7....2s. 0000 0080: 21 17 32 D2 0D D1 3A 80 A6 A0 AC 45 F4 8C 8A DD !.2...:. ...E.... test.e3e4257ac93a5a7926650ed9898b547ba19a3fd3.flac 0000 0000: 66 4C 61 43 00 00 00 22 12 00 12 00 00 20 95 00 fLaC..." ..... .. 0000 0010: 3C 9C 05 62 22 F0 00 38 DF C0 CD A8 B5 66 6F B6 <..b"..8 .....fo. 0000 0020: CC 54 26 DC 89 F0 D1 52 0E 21 84 00 00 28 20 00 .T&....R .!...( . 0000 0030: 00 00 72 65 66 65 72 65 6E 63 65 20 6C 69 62 46 ..refere nce libF 0000 0040: 4C 41 43 20 31 2E 32 2E 31 20 32 30 30 37 30 39 LAC 1.2. 1 200709 0000 0050: 31 37 00 00 00 00 FF F8 56 18 00 2C 4E 00 1A 00 17...... V..,N... 0000 0060: 4F 00 22 FF F1 FF FE 00 0B 00 2B 00 44 B5 3F 55 O."..... ..+.D.?U 0000 0070: 81 0A CF 26 99 4E 52 10 37 B2 06 DB D0 32 73 A5 ...&.NR. 7....2s. 0000 0080: 21 17 32 D2 0D D1 3A 80 A6 A0 AC 45 F4 8C 8A DD !.2...:. ...E.... Look at offset 0x14, 3B vs 22
Created attachment 135738 [details] a git bisect run test script for testing whether the bug works edit it and change the location to sugar.ogg
Created attachment 135739 [details] script to do the bisection run this in your gst-plugins-good checkout to do the bisection
Hm, this sounds more like a bug in libflac then as the only thing that this commit changes, is an additional call to the flac API to set the total number of samples. The flac encoder then creates invalid headers for some reason...
Well, setting the number of samples wrong doesn't sound like a good idea to me.
Your fix seems to make things work fine for me, so I guess we can close this now, can't we? It looks to me like there's an admittedly somewhat obscure bug in libFLAC here. The bitwriter doesn't seem to zero out bits it's not meant to write in all cases, and _set_total_samples_estimate() doesn't seem to ensure that the input isn't too large either, which leads to the possibility of a bit being set in the wrong place where it shouldn't be set (viz. in the bits_per_sample field in the stream info header). Filed a bug against FLAC, in case they want to fix this. Committed this as well now, to guard against bogus values from upstream (don't really want that to mess up the file, do we?): diff --git a/ext/flac/gstflacenc.c b/ext/flac/gstflacenc.c index cf5afcd..1adcdb0 100644 --- a/ext/flac/gstflacenc.c +++ b/ext/flac/gstflacenc.c @@ -633,7 +633,7 @@ gst_flac_enc_sink_setcaps (GstPad * pad, GstCaps * caps) if (total_samples != GST_CLOCK_TIME_NONE) FLAC__stream_encoder_set_total_samples_estimate (flacenc->encoder, - total_samples); + MIN (total_samples, G_GUINT64_CONSTANT (0x0FFFFFFFFF))); gst_flac_enc_set_metadata (flacenc);