GNOME Bugzilla – Bug 650960
flacparse makes decoded flac files start at sample offset 9215
Last modified: 2011-10-29 15:24:48 UTC
Create test file, note total length gst-launch -v filesrc location=pink.flac ! flacdec ! filesink silent=False Decode with flacdec, note that first offset is 0 gst-launch -v filesrc location=pink.flac ! flacdec ! fakesink silent=False Decode with flacparse, note that first offset is 9215 gst-launch -v filesrc location=pink.flac ! flacparse ! flacdec ! fakesink silent=False Additionally I noticed that offset_end isn't being set when it's easy to set it.
Indeed, see the 6th buffer, which should have offset 0: $ gst-launch-0.10 filesrc location=566769.flac ! flacparse ! fakesink -v | grep chain | head -n10 (snip 5 header buffers) chain 5318 bytes, timestamp: 0:00:00.000000000, duration: 0:00:00.104489795, offset: 104489795, offset_end: 4608
Created attachment 193972 [details] [review] flacdec: warn if we see a variable block size where unsupported
Created attachment 193973 [details] [review] flacdec: fix bit twiddling Right shifting a 8 bit value by 8 bits is twice too much to get the high 4 bits.
Created attachment 193974 [details] [review] flacdec: bail on reserved value Now that we look at the right bits, we can test against the reserved value as we do for other fields.
Created attachment 193975 [details] [review] flacdec: avoid timestamp/offset tracking going out of sync The libFLAC API is callback based, and we must only call it to output data when we know we have enough input data. For this reason, a single processing step is done when receiving a buffer. However, if there were metadata buffers still pending, a step intended for the first audio frame might end up writing that leftover metadata. Since a single step is done per buffer, this will cause every buffer to be written one step late. This would add some latency (a bufferfull's worth), possibly lose a buffer when seeking or the like, and also cause timestamp and offset to be applied to the wrong buffer, as updates to the "current" segment last_stop (from incoming buffer timestamp) will be applied to an output buffer originating from the previous incoming buffer. This fixes the issue by ensuring that, upon receiving the first audio frame, processing is done till all metadata is processed, so the next "single step" done will be for the audio frame. After this, we should keep to 1 input buffer -> 1 output buffer and so avoid getting out of sync.
Created attachment 193982 [details] [review] flacdec: error out if we see a variable block size where unsupported Alternate version that throws an element error instead of logging a warning, at tpm's suggestion.
Pushed these: commit 3e0134f51fe174a1c68422b55147b2f2a2b07373 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Aug 16 17:27:13 2011 +0100 flacdec: avoid timestamp/offset tracking going out of sync The libFLAC API is callback based, and we must only call it to output data when we know we have enough input data. For this reason, a single processing step is done when receiving a buffer. However, if there were metadata buffers still pending, a step intended for the first audio frame might end up writing that leftover metadata. Since a single step is done per buffer, this will cause every buffer to be written one step late. This would add some latency (a bufferfull's worth), possibly lose a buffer when seeking or the like, and also cause timestamp and offset to be applied to the wrong buffer, as updates to the "current" segment last_stop (from incoming buffer timestamp) will be applied to an output buffer originating from the previous incoming buffer. This fixes the issue by ensuring that, upon receiving the first audio frame, processing is done till all metadata is processed, so the next "single step" done will be for the audio frame. After this, we should keep to 1 input buffer -> 1 output buffer and so avoid getting out of sync. https://bugzilla.gnome.org/show_bug.cgi?id=650960 commit e09eb95a5fd46bcbf3088dbdff780a4adebb0a75 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Aug 16 15:32:07 2011 +0100 flacdec: bail on reserved value Now that we look at the right bits, we can test against the reserved value as we do for other fields. https://bugzilla.gnome.org/show_bug.cgi?id=650960 commit 64beef46101b023ca4f052c769ce9b0280ca9dcc Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Aug 16 15:27:43 2011 +0100 flacdec: fix bit twiddling Right shifting a 8 bit value by 8 bits is twice too much to get the high 4 bits. https://bugzilla.gnome.org/show_bug.cgi?id=650960 commit 1549aaba2749f9b8a2fc9e56c849c69a8107098f Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Aug 16 15:22:46 2011 +0100 flacdec: warn if we see a variable block size where unsupported https://bugzilla.gnome.org/show_bug.cgi?id=650960 Went for the non-GST_ELEMENT_ERROR variant after all, because we may just be dealing with a bad syncpoint, where we don't want to error out. There's still an off-by-one error though, check the offset of the second buffer flacdec outputs, it's 1 too small. This may be because of an off-by-one or because we do calculations with GST_CLOCK_TIME_NONE somewhere.
I'd missed your extra comments at the end of the patches list, just saw this now. This off by one is fixed by Monty's flac patch in 651615, just tracked it to that and checked it fixes this test case too. Marking as dependency.
With respect to the original observation regarding offsets; the latter are media specific and in case of flacparse it arranges those to have meaning as used by oggmux (in older days at least), which are likely to be confusing if you don't happen to be oggmux.
Patches committed, and the off by 1 is now fixed in 651615. Closing.