feat: update retain helpers #21
Conversation
brycetham
requested review from
szotrabh,
bbaldino,
SomeBody16,
antsukanova,
k-wasniowski and
edvujic
src/munge.ts
Outdated
| * | ||
| * @param sdpOrAv - The {@link Sdp} or {@link AvMediaDescription} from which to filter codecs. | ||
| * @param allowedCodecNames - The names of the codecs that should remain in the SDP. | ||
| * @param predicate - A function used to determine which codecs should be retained. | ||
| * @returns A boolean that indicates if some codecs have been filtered out. | ||
| */ | ||
| export function retainCodecs( | ||
| sdpOrAv: Sdp | AvMediaDescription, |
There was a problem hiding this comment.
I wonder, do we ever use this with an entire SDP? Retaining codecs across media types doesn't make much sense, not sure why I supported that (or if it was even me, I guess)
I guess the predicate version it makes a bit more sense since then it can detect audio vs video.
There was a problem hiding this comment.
That's a good question. I took a look at the history for this file. It seems like removeCodec was the original helper function which took in an SDP (and it's still in this file, though I don't see it being used anywhere). Then we added the current retainCodecs and retainCandidates functions... and I'm not sure why I had it accept both? Probably because it made sense to take in a media description for those, but removeCodec took in an SDP, so I compromised and just had all of them take both.
There was a problem hiding this comment.
Ah ok, yeah I think removeCodec made sense with an SDP because it would remove all instances of those codecs from all mlines, so it was easy to throw both video and audio codecs in there. With 'retain' though, for codecs, I'm not sure it makes sense (transport types still accepting an entire sdp does, though)
There was a problem hiding this comment.
@bbaldino Will it be logical to keep the API consistent between these functions and more flexible? Or is your vision always to retain codecs only from AvMediaDescription when working with SDP?
There was a problem hiding this comment.
It's true that the APIs would differ, but I think in this situation it's accurate: the transport information is consistent across all mlines, while codecs differ between audio and video.
src/munge.spec.ts
Outdated
| describe('retainCodecs', () => { | ||
| it('should retain codecs correctly when passing in an SDP', () => { | ||
| expect.hasAssertions(); | ||
| const offer = fs.readFileSync('./src/sdp-corpus/offer_with_extra_codecs.sdp', 'utf-8'); | ||
| const parsed = parse(offer); | ||
|
|
||
| retainCodecs(parsed, ['h264', 'opus']); | ||
| // eslint-disable-next-line jsdoc/require-jsdoc | ||
| const predicate = (codecInfo: CodecInfo) => |
There was a problem hiding this comment.
If this predicate is the same for multiple tests, consider putting it in a describe to avoid duplicating code.
There was a problem hiding this comment.
It's only being used for half of the tests though. To me, it would make sense to do so if most or all of the tests use it (and in the future, if we add more cases to this describe block, that means most of the test cases will not use it).
src/munge.ts
Outdated
| .filter((codecName) => !allowedLowerCase.includes(codecName.toLowerCase())) | ||
| .forEach((unwantedCodec) => removeCodec(sdpOrAv, unwantedCodec)); | ||
| return retainCodecs(sdpOrAv, (codecInfo) => | ||
| allowedLowerCase.includes(codecInfo.name?.toLowerCase() as string) |
There was a problem hiding this comment.
How can you ensure that this function will always have codecInfo.name defined to cast type to string and not have some callback value like "" for example?
There was a problem hiding this comment.
Ah yeah, it was like this before but I probably should be checking to see if it exists first before checking if it's in the list of allowed codec names.
There was a problem hiding this comment.
@bbaldino, do you remember why CodecInfo.name is optional? Is it just for convenience when building the CodecInfo, so you don't have to define a name until CodecInfo.addLine() is called?
There was a problem hiding this comment.
Discussed offline, but I think your memory is right: it's optional because we fill in that information later. Bryce and I discussed some investigation of using an intermediary type while parsing that could then be finalized into another type which probably models required fields vs optional ones, so we can look into that.
# [1.7.0](v1.6.0...v1.7.0) (2024-07-30) ### Features * update retain helpers ([#21](#21)) ([49b5566](49b5566))
This PR updates the following utility functions:
retainCodecsfunction has been renamed toretainCodecsByCodecName. It now also returns a Boolean to indicate if any codecs were filtered out, similar to the currentretainCandidates. In addition, thesdpOrAvparameter has been updated to only accept anAvMediaDescription(instead of also accepting anSdp) as suggested by @bbaldino.retainCodecsfunction has been created. This function takes a predicate as a parameter that is allows the user to define their own conditions for which codecs will be retained.retainCandidatesfunction has been renamed toretainCandidatesByTransportType.retainCandidatesfunction has been created. Similar to the newretainCodecs, this function takes a predicate as a parameter.The purpose of these changes is to allow users to filter out codecs and candidates using whatever conditions they want (such as only retaining H.264 codecs with a certain profile, or only retaining TLS candidates), while keeping the old utility functions for convenience.