GNOME Bugzilla – Bug 697463
rtpsbcdebay: incorrect frame length calculation for mono and full stereo modes
Last modified: 2013-04-09 22:26:19 UTC
Created attachment 240865 [details] [review] rtpsbcdebay: incorrect frame length calculation for dual channel. debug output in gst_rtp_sbc_depay_get_params reports this for IPad: blocks:16 channels:2 subbands:8 bitpool:53 mode 0x02/dual channel It seems to be wrong to multiply bitpool by number of channels. I failed to find a proper spec clarifying that. The following RFC has only a table about joint stereo, but not dual channel. The bitpool values from IPad though, match those from stereo w/o joint, and these are not multiplied by number of channels. checked by filling the numbers http://tools.ietf.org/html/draft-ietf-payload-rtp-sbc-04#section-3 +------------------------------------------------------------+ | SBC encoder settings at Medium Quality | +--------------------------------+-------------+-------------+ | | Mono | Joint Stereo| | Sampling frequency (kHz) | 44.1 | 48 | 44.1 | 48 | | Bitpool value | 19 | 18 | 35 | 33 | | Resulting frame length (bytes) | 46 | 44 | 83 | 79 | | Resulting bit rate (kb/s) | 127 | 132 | 229 | 237 | +--------------------------------+------+------+------+------+ | SBC encoder settings at High Quality | +--------------------------------+-------------+-------------+ | | Mono | Joint Stereo| | Sampling frequency (kHz) | 44.1 | 48 | 44.1 | 48 | | Bitpool value | 31 | 29 | 53 | 51 | | Resulting frame length (bytes) | 70 | 66 | 119 | 115 | | Resulting bit rate (kb/s) | 193 | 198 | 328 | 345 | +--------------------------------+------+------+------+------+ + Other settings: Block length = 16, loudness, subbands = 8 | +------------------------------------------------------------+ Also pulseaudio is neither multiplying by number channels: size_t sbc_get_frame_length(sbc_t *sbc) { [snip] subbands = sbc->subbands ? 8 : 4; blocks = 4 + (sbc->blocks * 4); channels = sbc->mode == SBC_MODE_MONO ? 1 : 2; joint = sbc->mode == SBC_MODE_JOINT_STEREO ? 1 : 0; bitpool = sbc->bitpool; ret = 4 + (4 * subbands * channels) / 8; /* This term is not always evenly divide so we round it up */ if (channels == 1) ret += ((blocks * channels * bitpool) + 7) / 8; ^^^^^^^^ unnecessary, since it's 1 else ret += (((joint ? subbands : 0) + blocks * bitpool) + 7) / 8; return ret; } ... and audio plays perfect with the patch.
Created attachment 240866 [details] gst-launch-1.0 avdtpsrc transport=$1 ! gdppay ! filesink location=./avdtpsrc.dump scary, be warned
I *think* I based the calculations in sbcparse and dec on the A2DP spec v10 ("Release Date: 22 May 2003, BLUETOOTH SPECIFICATION, Advanced Audio Distribution Profile"): > 12.9 Calculation of Bit Rate and Frame Length > > Bit Rate (bit_rate) is calculated using the following equation: > bit_rate = 8 * frame_length* fs / nrof_subbands / nrof_blocks, > where fs, nrof_subbands and nrof_blocks denote sampling > frequency, number of subbands and number of blocks, > respectively. Bit Rate is expressed in kb/s, because > fs is expressed in kHz. The Frame Length (frame_length) > is expressed in bytes as > > frame_length = 4 + (4 * nrof_subbands * nrof_channels ) / 8 > + [ nrof_blocks * nrof_channels * bitpool / 8 ]. > > for the MONO and DUAL_CHANNEL channel modes, and > > frame_length = 4 + (4 * nrof_subbands * nrof_channels ) / 8 > + [(join * nrof_subbands + nrof_blocks * bitpool ) / 8] > > for the STEREO and JOINT_ STEREO channel modes. > > Here, nrof_channels and bitpool denote number of channels > and bitpool value, respectively. When joint stereo is used, > join = 1, otherwise 0. For reference, see Section 12.5. This just errors out for me with 'short packet' (bug elsewhere I guess): $GST_DEBUG=*rtp*:9 gst-launch-1.0 filesrc location=697463.gdp ! gdpdepay ! rtpsbcdepay ! sbcdec ! pulsesink -v
Thanks for the pointer, I was wrong. Actually I doubt the IPad uses dual channel at all. It probably sends it as Stereo, but due endianness issues, it is interpret as dual channel. bit0 in spec is bit1 in host machine register, at least that's how it's in used block/subband calculation. Only for channel_mode bit0 is mapped to bit0 of host machine register. From draft-ietf-payload-rtp-sbc: CHANNEL_MODE (2 bits): These two bits indicate with which channel mode the frame has been encoded. The number of channels depends on this information. The channel mode MUST NOT be changed without changing the payload type, too. +--------------+--------------+-----------+ | CHANNEL_MODE | channel mode | number of | | bit 0 1 | | channels | +--------------+--------------+-----------+ | 0 0 | MONO | 1 | | 0 1 | DUAL_CHANNEL | 2 | | 1 0 | STEREO | 2 | | 1 1 | JOINT_STEREO | 2 | +--------------+--------------+-----------+ --- a/gst/rtp/gstrtpsbcdepay.c +++ b/gst/rtp/gstrtpsbcdepay.c @@ -142,7 +142,7 @@ gst_rtp_sbc_depay_get_params (GstRtpSbcDepay * depay, const guint8 * data, length = 4 + ((4 * subbands * channels) / 8); - if (!(channel_mode & 0x1)) { + if (!(channel_mode & 0x2)) { /* Mono || Dual channel */ length += ((blocks * channels * bitpool) + 4 /* round up */ ) / 8;
Pushed, thanks (changed it a bit though): commit 20d3ec881019a10bd669c1bb909817cf45abd3be Author: Andreas Fenkart <andreas.fenkart@streamunlimited.com> Date: Tue Apr 9 23:13:18 2013 +0100 rtpsbcdepay: fix sbc frame length calculation for mono and stereo modes https://bugzilla.gnome.org/show_bug.cgi?id=697463 The +4 /*round up*/ bits are a bit curious - I guess it works because the bit before that will always be a multiple of 4 because blocks is 4, 8, 12 or 16. Don't think the other elements don't do the rounding up part here, fwiw.