-
Notifications
You must be signed in to change notification settings - Fork 115
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
setParameters: Do argument checks in sync section and specify parellel steps #1560
Conversation
42fd551
to
dad40e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comments.
<tr> | ||
<td><dfn><code>hardware-encoder-error</code></dfn></td> | ||
<td>The hardware encoder resources required for the requested | ||
operation are not available.</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"... or the hardware encoder doesn't support the provided set of parameters"? Those could feasibly be two separate errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised a separate issue for this question #1602
webrtc.html
Outdated
<li>Resolve <var>p</var> with <code>undefined</code>.</li> | ||
<li>Let <var>p</var> be a new promise.</li> | ||
<li>In parallel, configure the media stack to use | ||
<var>parameters</var> to transmit <var>sender</var>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It's not sender
that's being transmitted, it's sender
's track.
webrtc.html
Outdated
created <code><a>RTCError</a></code> whose | ||
<code>errorDetail</code> set to | ||
"hardware-encoder-error".</li> | ||
</ol> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a catchall "OperationError" if something non-hardware-related failed in the async steps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add that. Hopefully it wouldn't be used much if we have a rather complete list of specific errors.
webrtc.html
Outdated
<li>If an error occurred due to hardware resources not | ||
being available, reject <var>p</var> with a newly | ||
created <code><a>RTCError</a></code> whose | ||
<code>errorDetail</code> set to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is set to"
Thanks for reviewing @taylor-b. New (and rebased) iteration coming up. |
dad40e5
to
426672e
Compare
426672e
to
9bc2856
Compare
First stab, do not merge yet. We need to figure out what goes into the parallel steps.
fix #1520
Preview | Diff