This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
dexter gerrit-no-reply at lists.osmocom.orgdexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/17095 )
Change subject: dtx: add decoding for AMR-DTX frames
......................................................................
Patch Set 10:
(23 comments)
I think we have it now. We can detect DTX frames and get an n_errors/n_bits_total rating for detected frames. If we do not detect a DTX frame the code assumes that the frame is a normal speech frame and everything continues as normal.
https://gerrit.osmocom.org/c/libosmocore/+/17095/2//COMMIT_MSG
Commit Message:
https://gerrit.osmocom.org/c/libosmocore/+/17095/2//COMMIT_MSG@12
PS2, Line 12: caller can be informed about this
> about this? "this" means that the decoder is not able to handle dtx frames? or that it now supports […]
I am not there yet, it is correct that the decoder can not decode the DTX frames yet. However, we only need to decode SID_UPDATE frames, all other frames do not contain audio information, they just tell us if DTX is switched on or off. However, the upper layers must know that those frames are present to compute the measurement results correctly. I will get back to this commit message when the patch is done.
https://gerrit.osmocom.org/c/libosmocore/+/17095/5/include/osmocom/coding/gsm0503_amr_dtx.h
File include/osmocom/coding/gsm0503_amr_dtx.h:
https://gerrit.osmocom.org/c/libosmocore/+/17095/5/include/osmocom/coding/gsm0503_amr_dtx.h@30
PS5, Line 30: static inline const char *gsm0503_amr_dtx_frame_name(uint8_t frame)
> not uint8_t, but enum gsm0503_amr_dtx_frames
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/include/osmocom/coding/gsm0503_amr_dtx.h
File include/osmocom/coding/gsm0503_amr_dtx.h:
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/include/osmocom/coding/gsm0503_amr_dtx.h@35
PS2, Line 35: uint8_t gsm0503_detect_afs_dtx_frame(ubit_t * ubits);
> we usually use "*ubits", not "* ubits"
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/include/osmocom/coding/gsm0503_coding.h
File include/osmocom/coding/gsm0503_coding.h:
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/include/osmocom/coding/gsm0503_coding.h@70
PS2, Line 70: int gsm0503_tch_ahs_decode_dtx(uint8_t *tch_data, const sbit_t *bursts, int odd,
> int odd probably means true/false, so it can be a bool?
gsm0503_tch_ahs_decode_dtx is a new variant of gsm0503_tch_ahs_decode, The int odd variable is part of the existing API, changing this would be out of the scope of this patch.
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c
File src/coding/gsm0503_amr_dtx.c:
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@44
PS2, Line 44: ubit_t
> const
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@47
PS2, Line 47: ubit_t id_marker[] = { 0, 1, 0, 0, 1, 1, 1, 1, 0 };
> static const
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@60
PS2, Line 60: ubits++;
> you are ncreasing ubits 4 times +1 here, and later on you increase it 4 times afterwards. […]
Yes it is correct. The id_marker appears for 4 bits, then there are 4 bits uninteresting bits which need to be jumped over and then there are again 4 bits of id_marker and so on. So it is correct.
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@68
PS2, Line 68: if (errors < count * 4 / 8)
> return (errors < count * 4 / 8);
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@73
PS2, Line 73: static bool detect_ahs_id_marker(ubit_t * ubits, ubit_t * id_marker)
> Same kind of comments apply for other functions in this file.
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@73
PS2, Line 73: ubit_t
> const
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@96
PS2, Line 96: static bool detect_interleaved_ahs_id_marker(ubit_t * ubits, ubit_t * id_marker)
> const uibt_t *id_marker.
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@122
PS2, Line 122: ubit_t
> const
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@125
PS2, Line 125: { 0, 0, 0, 0 };
> could be just ... […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@147
PS2, Line 147: ubit_t
> const
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@151
PS2, Line 151: { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
> could be just ... […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@211
PS2, Line 211: ubit_t id_marker[] = { 0, 1, 0, 0, 1, 1, 1, 1, 0 };
> Ack
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@252
PS2, Line 252: uint8_t gsm0503_detect_afs_dtx_frame(ubit_t * ubits)
> return is of type enum gsm0503_amr_dtx_frames
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_amr_dtx.c@266
PS2, Line 266: uint8_t gsm0503_detect_ahs_dtx_frame(ubit_t * ubits)
> return is of type enum gsm0503_amr_dtx_frames
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/5/src/coding/gsm0503_amr_dtx.c
File src/coding/gsm0503_amr_dtx.c:
https://gerrit.osmocom.org/c/libosmocore/+/17095/5/src/coding/gsm0503_amr_dtx.c@34
PS5, Line 34: { AFS_SID_FIRST, "AFS_SID_FIRST" },
> If possible please align the strings.
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/5/src/coding/gsm0503_amr_dtx.c@138
PS5, Line 138: 14 - ones[k]
> each ones item (onces[k]) is increased at most 14 times by previous loops, and each time += ubits[k] […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/5/src/coding/gsm0503_amr_dtx.c@157
PS5, Line 157: ones[k] += ubits[0];
> shouldn't this be ubits[k] perhaps?
This is indeed a bit irretating. I need to access every 2nd ubit, this is done by incrementing the ubits pointer by two every time and accessing always the first element. I have now changed it a bit to make it more clear.
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_coding.c
File src/coding/gsm0503_coding.c:
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_coding.c@2098
PS2, Line 2098: * \param[out] dtx DTX frame type
> This looks wrong, gsm0503_tch_afs_decode() has no such parameter.
Done
https://gerrit.osmocom.org/c/libosmocore/+/17095/2/src/coding/gsm0503_coding.c@2166
PS2, Line 2166: printf("%u", cBd[i]);
> I'm sure that shouldn't be there.
Done
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/17095
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I2bbdb39ea20461ca08b2e6f1a33532cb55cd5195
Gerrit-Change-Number: 17095
Gerrit-PatchSet: 10
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 17 Mar 2020 12:36:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: fixeria <axilirator at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200317/7692edcf/attachment.htm>