feat: audio video stream missing data detector #30
Conversation
eugeny-dementev
force-pushed
the
TT-269-audio-video-missing
branch
2 times, most recently
from
7416eea to
6520a75
Compare
eugeny-dementev
changed the title
eugeny-dementev
changed the title
eugeny-dementev
force-pushed
the
TT-269-audio-video-missing
branch
from
1a39ff9 to
da3b55f
Compare
eugeny-dementev
changed the title
eugeny-dementev
changed the title
| } | ||
|
|
||
| export default class MissingStreamDataDetector extends BaseIssueDetector { | ||
| readonly #lastMarkedAt = new Map<string, number>(); |
There was a problem hiding this comment.
The name of this property does not explicitly says what it is used for. I'd suggest adding context to the name about track (e.g. tracksToLastMarkedAt / trackToLastMarkedAtMap / )
There was a problem hiding this comment.
That value mainly used to limit how often issue reported. Not sure how to name it properly. I've took it mostly intact from FrozenVideoTrackDetector
| )); | ||
| } | ||
|
|
||
| const unvisitedTrackIds = new Set(this.#lastMarkedAt.keys()); |
There was a problem hiding this comment.
The name seems to be confusing. What if to skip using a variable and inline
(new Set(this.#lastMarkedAt.keys())).forEach(...)
or to rename the variable
|
|
||
| constructor(params: MissingStreamDetectorParams = {}) { | ||
| super(); | ||
| this.#timeoutMs = params.timeoutMs ?? 10_000; |
There was a problem hiding this comment.
timeoutMs is used for marked tracks cleanup and for marking tracks (in conditions), don't we need to add 2 separate props for cleanup and for mark tracks ttl?
| const { audio: { inbound: newAudioInbound } } = data; | ||
| const prevVideoInbound = prevData?.video.inbound; | ||
| const prevAudioInbound = prevData?.audio.inbound; | ||
|
|
| ): IssueDetectorResult { | ||
| const issues: IssuePayload[] = []; | ||
|
|
||
| const mapStatsByTrackId = (items: CommonParsedInboundStreamStats[]) => new Map<string, CommonParsedInboundStreamStats>( |
There was a problem hiding this comment.
Can we move this function to a separate static method / helper?
|
|
||
|
|
||
| if (bytesReceivedDelta === 0 && !inboundItem.track.detached && !inboundItem.track.ended) { | ||
| const hasIssue = this.markIssue(trackId); |
There was a problem hiding this comment.
markIssue() seems to only mark the issue, but actually it checks if it should mark it. What about separating of the logic into 2 methods: checkShouldMarkIssue / markIssue?
| const statsSample = { | ||
| bytesReceivedDelta, | ||
| bytesReceived: inboundItem.bytesReceived, | ||
| trackDetached: inboundItem.track.detached, |
There was a problem hiding this comment.
this props are gonna be constants (always true), do we need to add them to the stats sample?
There was a problem hiding this comment.
I wanted to show in statsSample that these values are not true. So it's visible that stream is in fact alive and active, just has no data going through it
There was a problem hiding this comment.
I've no converted the undefined to false. Not sure if it's correct
eugeny-dementev
force-pushed
the
TT-269-audio-video-missing
branch
from
50a35ee to
de8e19d
Compare
No description provided.