Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate RTPTransceiver stopped flag #2336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

boks1971
Copy link
Contributor

There is use of t.stopped in peerconnection.go, but that flag is not set/used in RTPTransceiver at all. This PR sets that when a transceiver is stopped and provides an API to check if a transceiver is stopped.

@Sean-Der @davidzhao @cnderrauber The fact that code does not seem to get tripped makes me think I am missing something, but I noticed the ☝️ (i. e. stopped flag in RTPTransceiver was not set/used) and made this PR to address this. Have tested this change with LiveKit and it seems fine.

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Base: 77.52% // Head: 77.59% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (097d94f) compared to base (42dc0d4).
Patch coverage: 91.66% of modified lines in pull request are covered.

❗ Current head 097d94f differs from pull request most recent head 71f4e4b. Consider uploading reports for the commit 71f4e4b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2336      +/-   ##
==========================================
+ Coverage   77.52%   77.59%   +0.07%     
==========================================
  Files          87       87              
  Lines        9262     9266       +4     
==========================================
+ Hits         7180     7190      +10     
+ Misses       1648     1644       -4     
+ Partials      434      432       -2     
Flag Coverage Δ
go 79.37% <91.66%> (+0.08%) ⬆️
wasm 70.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
peerconnection.go 78.23% <83.33%> (+0.16%) ⬆️
rtptransceiver.go 87.71% <100.00%> (+0.36%) ⬆️
icetransport.go 68.42% <0.00%> (-1.22%) ⬇️
dtlstransport.go 64.90% <0.00%> (+1.86%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

There is use of `t.stopped` in `peerconnection.go`, but that
flag is not set/used in `RTPTransceiver` at all. This PR
sets that when a transceiver is stopped and provides an API
to check if a tranceiver is stopped.
@cnderrauber
Copy link
Member

cnderrauber commented Oct 19, 2022

Pion's transceiver don't have a real stopped state now, it call stop method when state go to inactive, so in pion, a transceiver after stop (actually it not set stopped flag, just transit to inactive) can be reused. #2226 also mentioned this. Actually, https://datatracker.ietf.org/doc/html/rfc8829#section-4.2.1 said that a stopped can't be reused, so this PR would comply with the rfc, but changed transceiver reuse behavior, might cause problem if application relied on it.
There are #1943 and #1969 do similar things and revert it, don't know why, @Sean-Der might have suggestion about this.

@boks1971
Copy link
Contributor Author

Thank you @cnderrauber . Did not realise this path had already been done. Maybe, we should just remove the stopped flag to avoid confusion.

@edaniels
Copy link
Member

hey @boks1971, do you still want to work together to get this in?

@boks1971
Copy link
Contributor Author

#1969

@edaniels Would be good to clean up that unused flag, but the discussions surrounding it which @cnderrauber pointed out makes me reluctant to change things. There was also a similar commit by @Sean-Der and a revert. Have not checked exactly why it was reverted.

I am still thinking that getting rid of just that field is good as it is has only one value, i. e. false. The long discussion in #2226 is more around the expected behaviour of stop vs inactive and not around that field value per se.

Getting rid of that field looks straightforward except this one place which is conditioned on if t.stopped which is never true now. Unclear whether that if block should be completely removed if stopped field is removed OR the condition needs to be different -

if t.stopped && t.Mid() != "" {

@edaniels
Copy link
Member

I think it's safe and good to remove the always false bool then. Separately, I think we need to get the behavior of stopped, ended, and friends in line with the W3 spec. I may try to tackle that soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants