GNOME Bugzilla – Bug 742446
sbcparse: Frame coalescing broken for joint stereo
Last modified: 2016-01-16 22:22:20 UTC
sbcparse's frame coalescing is broken for joint stereo SBC streams. We only ever get frames from these streams out of sbcparse with one frame to a buffer. There's a mistake in the SBC frame header size calculation (see the attached patch for a full explanation) that was introduced in the rewrite of the bluez code. Because of this, the frame coalescing code can't find the next frame where it expects it to be, and it gives up on coalescing the frame with any subsequent ones.
Created attachment 293911 [details] [review] Patch to fix the issue.
Thanks for the patch. Could you also make a few kB of such a stream available? You can run ... ! gdppay ! filesink location=sbc.gdp instead of feeding it into sbcparse.
Created attachment 293947 [details] Test vector (4 subbands)
Any further details needed on this?
Thanks for the patch, just one quick question: your new formula doesn't align with the formula given in the spec (12.9 Calculation of Bit Rate and Frame Length). I would like to understand why this is the case, and I think it would be good to keep the code aligned with that formula. Perhaps we should be doing the calculation in floating point and simply round up to the nearest byte, I guess we're truncating/rounding down now when there are extra bits, and that's why you're just adding another byte here?
or just ROUND_UP_8 (blah) / 8
Thanks for reviewing the patch. You're right, the formula in the spec is correct, and rounding up corrects the problem. Patches to follow.
Created attachment 318887 [details] [review] gst-plugins-good sbc frame length patch v2
Created attachment 318888 [details] [review] gst-plugins-bad sbc frame length patch v2
Created attachment 318890 [details] SBC frame length comparison (for all configs) Here's a python script comparing the v2 patch's calculation against bluez's for all valid SBC parameters (it should give no output meaning they both agree on frame length). If you uncomment the line with the XXX, then you'll get output for the SBC parameters for which gstreamer's current implementation and bluez disagree (affects both STEREO and JOINT configs).
Thanks, pushed to master: commit 95a14fd470e7b89a9a5e7476c03e710066ce2b46 Author: Tim Sheridan <tim.sheridan@imgtec.com> Date: Tue Jan 12 14:54:23 2016 +0000 sbc: sbcdec: Fix frame length calculation SBC frame length calculation wasn't being rounded up to the nearest byte (as specified in the A2DP 1.0 specification, section 12.9). This could cause 'stereo' and 'joint stereo' mode SBC streams to have incorrectly calculated frame lengths. https://bugzilla.gnome.org/show_bug.cgi?id=742446 commit 205565ccd9f93968265e76019426d9b048e2d3eb Author: Tim Sheridan <tim.sheridan@imgtec.com> Date: Mon Jan 11 16:29:55 2016 +0000 sbcparse: Fix frame length calculation SBC frame length calculation wasn't being rounded up to the nearest byte (as specified in the A2DP 1.0 specification, section 12.9). This could cause 'stereo' and 'joint stereo' mode SBC streams to have incorrectly calculated frame lengths. Incorrect frame length calculation causes frame coalescing to fail, as subsequent frames in the stream aren't found in the expected locations. https://bugzilla.gnome.org/show_bug.cgi?id=742446
Also cherry-picked into 1.6 branch (for 1.6.3)