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

RTCRtpSender and RTCRtpReceiver state #399

Closed
aboba opened this Issue Feb 29, 2016 · 15 comments

Comments

Projects
None yet
4 participants
@aboba
Contributor

aboba commented Feb 29, 2016

Neither the RTCRtpSender nor the RTCRtpReceiver objects have an associated state attribute or state change events. When the onerror event handler is fired is this equivalent to calling stop() (e.g. always non-recoverable)?

@aboba aboba added the 1.1 label Feb 29, 2016

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Mar 1, 2016

Contributor

At the moment, it is not clear when the onerror event handler will be called.

Contributor

aboba commented Mar 1, 2016

At the moment, it is not clear when the onerror event handler will be called.

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Mar 1, 2016

Contributor

If "onerror" is ever fired, it's not fatal. This was more done because we didn't want send() and receive() to have a promise but some issues may be discovered asynchronously later. If this is the case, the error can be fired and calling send() or receive() again can resolve the issue. I'm open for discussion on this behaviour but that's how it currently is working...

Contributor

robin-raymond commented Mar 1, 2016

If "onerror" is ever fired, it's not fatal. This was more done because we didn't want send() and receive() to have a promise but some issues may be discovered asynchronously later. If this is the case, the error can be fired and calling send() or receive() again can resolve the issue. I'm open for discussion on this behaviour but that's how it currently is working...

@aboba aboba added the question label Mar 4, 2016

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Mar 11, 2016

Contributor

We should have a section defining receiver.receive() and sender.send(). Specifically, how to process errors, and what is fatal, non-fatal. E.g. redefining header extensions to have different ID mappings for different receivers on the same transport would be fatal and bad.

Contributor

robin-raymond commented Mar 11, 2016

We should have a section defining receiver.receive() and sender.send(). Specifically, how to process errors, and what is fatal, non-fatal. E.g. redefining header extensions to have different ID mappings for different receivers on the same transport would be fatal and bad.

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Mar 11, 2016

Contributor

Is a promise needed on a receiver.receive() so that errors on calling receive() are clear as to what happened with what receive() method being called?

E.g. if you called receive() three times in a row for slight adjustments and a method called onerror fired, which receive() call actually failed?

Contributor

robin-raymond commented Mar 11, 2016

Is a promise needed on a receiver.receive() so that errors on calling receive() are clear as to what happened with what receive() method being called?

E.g. if you called receive() three times in a row for slight adjustments and a method called onerror fired, which receive() call actually failed?

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Mar 11, 2016

Contributor

Since some errors might require accessing multiple objects (e.g. checking the header extension mappings for all receivers on the same transport), receive() (and send(), for that matter) might need to be async. If that change is made, I'm not sure what onerror would be used for.

Contributor

aboba commented Mar 11, 2016

Since some errors might require accessing multiple objects (e.g. checking the header extension mappings for all receivers on the same transport), receive() (and send(), for that matter) might need to be async. If that change is made, I'm not sure what onerror would be used for.

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Apr 2, 2016

Contributor

@robin-raymond What do you recommend we do here? Change send() and receive() to async? Clarify the usage of onerror?

Contributor

aboba commented Apr 2, 2016

@robin-raymond What do you recommend we do here? Change send() and receive() to async? Clarify the usage of onerror?

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 2, 2016

Contributor

@aboba I'm thinking send(...) and receive(...) should return promises. If the promise failed, then it can be a rejected promise with a failure.

What does 1.0 return?

Plus then we can have getParameters() method that returns the post-filled information for the sender method too if we so choose to add that and we can know it is safe to call because the promise was resolved...

Contributor

robin-raymond commented Apr 2, 2016

@aboba I'm thinking send(...) and receive(...) should return promises. If the promise failed, then it can be a rejected promise with a failure.

What does 1.0 return?

Plus then we can have getParameters() method that returns the post-filled information for the sender method too if we so choose to add that and we can know it is safe to call because the promise was resolved...

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 7, 2016

Contributor

I think the movement is clearly towards a promise. There's just no other way to know what failed for an asynchronous based operation that fails later after receive() / send() returns. onerror would not resolve this problem.

Contributor

robin-raymond commented Apr 7, 2016

I think the movement is clearly towards a promise. There's just no other way to know what failed for an asynchronous based operation that fails later after receive() / send() returns. onerror would not resolve this problem.

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 15, 2016

Contributor

This is the fix that we need to do:

partial interface RTCRtpSender {
   Promise<void>  send(RTCRtpParameters parameters);
   attribute EventHandler?     onerror;  // REMOVE THIS
}

partial interface RTCRtpReceiver {
    Promise<void>    receive (RTCRtpParameters parameters);
   attribute EventHandler?     onerror;  // REMOVE THIS
}

The event summary also needs to remove those events for onerror.

Contributor

robin-raymond commented Apr 15, 2016

This is the fix that we need to do:

partial interface RTCRtpSender {
   Promise<void>  send(RTCRtpParameters parameters);
   attribute EventHandler?     onerror;  // REMOVE THIS
}

partial interface RTCRtpReceiver {
    Promise<void>    receive (RTCRtpParameters parameters);
   attribute EventHandler?     onerror;  // REMOVE THIS
}

The event summary also needs to remove those events for onerror.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Apr 16, 2016

Contributor

👍

BTW: wouldn't we need the very same for setTransport()?

Contributor

ibc commented Apr 16, 2016

👍

BTW: wouldn't we need the very same for setTransport()?

@robin-raymond robin-raymond removed their assignment Apr 16, 2016

@aboba

This comment has been minimized.

Show comment
Hide comment
@aboba

aboba Apr 20, 2016

Contributor

Yes, setTransport() probably needs to be async as well.

Contributor

aboba commented Apr 20, 2016

Yes, setTransport() probably needs to be async as well.

@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 20, 2016

Contributor

@aboba @ibc I can't think of anything (other than the transport being in an invalid state, i.e. "closed") that would cause adverse affects only detected later. Do you have any ideas? I'm not sure there will be a possible "rejection", more of an exception of the transports are in the wrong state at the time of being passed into the sender/receiver.

However, I think of is the promise would indicate when the change of transport has completed. For example, if you were remapping from one transport to another and you wanted to know when it was "safe" to dump an old transport in favour of a new one then you'd want to know when the promise was resolved and thus it's safe to close out the old transport. That's a justification for making "setTransport" a promise.

I'm already convinced "send / receive" methods require a promise because "onerror" is just confusing when / if it fires.

Thoughts?

Contributor

robin-raymond commented Apr 20, 2016

@aboba @ibc I can't think of anything (other than the transport being in an invalid state, i.e. "closed") that would cause adverse affects only detected later. Do you have any ideas? I'm not sure there will be a possible "rejection", more of an exception of the transports are in the wrong state at the time of being passed into the sender/receiver.

However, I think of is the promise would indicate when the change of transport has completed. For example, if you were remapping from one transport to another and you wanted to know when it was "safe" to dump an old transport in favour of a new one then you'd want to know when the promise was resolved and thus it's safe to close out the old transport. That's a justification for making "setTransport" a promise.

I'm already convinced "send / receive" methods require a promise because "onerror" is just confusing when / if it fires.

Thoughts?

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Apr 20, 2016

Contributor

I'm not aware of which kind of internal complexity could make setTransport() fail asynchronously and, if so, I don't understand why we should wait for a returned Promise to resolve in order to close the old transport. The only I don't want is a onerror event triggered by user initiated actions.

So I'm fine with setTransport() returning void or raising an Error.

Contributor

ibc commented Apr 20, 2016

I'm not aware of which kind of internal complexity could make setTransport() fail asynchronously and, if so, I don't understand why we should wait for a returned Promise to resolve in order to close the old transport. The only I don't want is a onerror event triggered by user initiated actions.

So I'm fine with setTransport() returning void or raising an Error.

@murillo128

This comment has been minimized.

Show comment
Hide comment
@murillo128

murillo128 Apr 21, 2016

setTransport should return a promise as per #467

murillo128 commented Apr 21, 2016

setTransport should return a promise as per #467

@aboba aboba added PR exists and removed publication-blocker labels Apr 25, 2016

aboba added a commit that referenced this issue Apr 25, 2016

aboba added a commit that referenced this issue Apr 26, 2016

Examples: async usage of send(), receive() and setTransport()
Fix for Issues: 
#399
#469

More work to do (please do not merge yet).
@robin-raymond

This comment has been minimized.

Show comment
Hide comment
@robin-raymond

robin-raymond Apr 28, 2016

Contributor

Looks better.

Contributor

robin-raymond commented Apr 28, 2016

Looks better.

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