Validation functions at `ortc.ts` file are modifying input data

[piranna]

Validation functions at https://github.com/versatica/mediasoup/blob/43edb3acd467045fc557ad7ba1fd87c11072e24b/node/src/ortc.ts are modifying the input data, in particular the createRouter() function options.mediaCodecs field. It's ok to normalice it internally, but input data should not be modified as a side effect. That validation functions has a comment about they modify the input data, but that docstrings are not getting into the Mediasoup documentation, so solutions are

1. add a note about that in the docs
2. export the normalization functions
3. (preferably) create a copy of the input data with utils.clone(), so original provided data gets untouched

[ibc]

Fixed here: PR https://github.com/versatica/mediasoup/pull/1285

[piranna]

Great, thank you so much :-)

[piranna]

I've seen at https://github.com/versatica/mediasoup/blob/d17a9374055e5ea8fb44d1bbf661a01661f6270e/node/src/ortc.ts#L987C2-L987C59 that this is still happening, validateRtpCodecCapability() function is modifying the provided codec parameter. In fact, according to git blame, that assignment was added just in the PR #1285 that was trying to fix this issue... I've done manual inspection of the code, and found more places over all the file where validation functions modify the input data. How can we proceed with that?

[ibc]

Those are private functions. They do modify given input. But the caller side in mediasoup code clones the I out before passing it so we are good. Tests verify it via Object.freeze().

[piranna]

Those are private functions. They do modify given input. But the caller side in mediasoup code clones the I out before passing it so we are good. Tests verify it via Object.freeze().

Yeah, I know, I have already reviewed the code, and it's ok, but I think it makes more sense doing the cloning inside the validation functions, if any, or better than that, DON'T do any modification, since validations should work with inmutable data, and just only at most do them in another normalization functions. I'm a bit purist with this things, but in the long run that make code more maintainable and easier to understand since there's no surprises.

[ibc]

Input given by the user app in all public methods is never modified, so mission accomplished. Those validateXxxx() functions are internal and they modify input data on purpose. Imagine they are called normalizeXxxx() instead of validate(). We are not gonna make it less performant by cloning the data in every validation function. The inline documentation of all those API private functions in ortc.ts clearly say that they may modify data.

[piranna]

I know, my question is, is it really needed they get modified in the first place?

[ibc]

Yes. Some codecs are removed, RTX codecs are added or removed depending on capabilities of Router and endpoint, codec.payloadType and codec.preferredPayloadType get a proper value, etc. I don't remember all the details but all these changes are needed and are very sensitive.

[piranna]

In that case, i would split the function in two, one to validate, and another to enhance.

[ibc]

In that case, i would split the function in two, one to validate, and another to enhance.

And make it unnecessary inefficient by having to iterate arrays twice. ortc.ts is private API. No need to make it fancy. It does what it must do and there is clear inline doc in each function.