<p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5//COMMIT_MSG@7">Patch Set #5, Line 7:</a> <code style="font-family:monospace,monospace">add full SDP codec information to the MNCC socket</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So there's 2 main topics here which seem would be in different commits, splitting this huge commit: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">That is true, but it is also true that it is realitvely hard to do one without the other. I actually started out separating it once, and I gave up, because it seemed to be too much effort.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The reasons: to only move the CRCX implies changing the order of the callbacks between msc_a and gsm_04_08_cc. That semantically separates in different places than before, and would require keeping state across those calls. Keeping that state is in this patch implemented by cc_sdp.c, which accumulates all information about what parts imply what codec choices. So, "just for the sake of review", I would invent a different patch version, that keeps state in a way that needs to be made to work and make sense, yet is again completely discarded one patch later.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I would argue that reviewers should in fact take a test setup, be it in hardware or in ttcn3, and examine what this code does and how it acts and reacts. This kind of profound change should anyway see such scrutiny, IMHO. To put it in blunt words, I want people to understand this patch and what it is doing.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Now, OTOH, that may be naive. I am not sure what benefit it would bring to do the extra work and separate patches, just to get two half uninformed reviews instead of no informed review at all. But if it means getting the patch merged (and if sysmocom agrees in the form of time available) then I can also invest more time in separating this paradigm shift into one weird patch that is then followed by still a paradigm shift. I know I'm sounding sarcastic, but whatever the community needs is fine with me.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe it does need to sit here a bit longer, maybe needs to be test-run at congress first, gain more audience before it can be merged.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I personally think this change is really exciting, also with the other change that I still have in mind for after this patch, because it allows a truly informed codec choice for the first time. So we can let this sit here for a bit, I'm fairly confident that someone or other will be drawn to it at some point and give it deep attention.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5/include/osmocom/msc/gsm_04_08.h">File include/osmocom/msc/gsm_04_08.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5/include/osmocom/msc/gsm_04_08.h@51">Patch Set #5, Line 51:</a> <code style="font-family:monospace,monospace">int cc_assignment_done(struct gsm_trans *trans);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(talking about these callbacks above)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5/src/libmsc/gsm_04_08_cc.c">File src/libmsc/gsm_04_08_cc.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5/src/libmsc/gsm_04_08_cc.c@581">Patch Set #5, Line 581:</a> <code style="font-family:monospace,monospace">                &trans->msc_a->cc.codec_list_bss_supported);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(talking about this state above)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5/src/libmsc/gsm_04_08_cc.c@640">Patch Set #5, Line 640:</a> <code style="font-family:monospace,monospace">      sdp->rtp = *rtp_cn_local;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(and this state)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/5/src/libmsc/gsm_04_08_cc.c@719">Patch Set #5, Line 719:</a> <code style="font-family:monospace,monospace">    sdp_audio_codecs_to_bearer_cap(&trans->bearer_cap, &trans->cc.sdp.result.audio_codecs);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(this state ... and so on)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953">change 15953</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-msc/+/15953"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I8c3b2de53ffae4ec3a66b9dabf308c290a2c999f </div>
<div style="display:none"> Gerrit-Change-Number: 15953 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 12 Dec 2019 15:04:16 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>