make generateKeyFrame take a list of rids and return a promise<undefined> #165
Conversation
fippo
force-pushed
the
trim-generatekeyframe
branch
from
788951a
to
6a4a3cb
Compare
fippo
changed the title
making it request a key frame for each rid or all of them if the list is empty. Returning a promise resolving with a timestamp is not necessary since requesting a key frame is something the encoder might not be able or willing to satisfy.
fippo
force-pushed
the
trim-generatekeyframe
branch
from
6a4a3cb
to
e43c486
Compare
|
discussed at the October 2022 meeting: https://www.w3.org/2022/10/18-webrtc-minutes.html#t03 |
|
Discussed at the December meeting. Needs update to return promise. |
|
updated. @dontcallmedom will need to slap the IPR bot who seems to have forgotten we're friends again ;-) |
|
the IPR bot seems to be happy without any slapping involved |
fippo
changed the title
| 1. Instruct the encoder associated with |sender| to generate a key frame for |rids| or all layers when |rids| is empty. | ||
| 1. Resolve |promise| with `undefined`. |
There was a problem hiding this comment.
I think we want to resolve the promise when the key frame is being queued, this is what the current spec is asking and is a nice addition in the case of script transform.
There was a problem hiding this comment.
This will introduce unnecessary complexity to what was supposed to be as a simple key frame generation mechanism. And will make scenarios involving multiple streams (active, inactive, restricted) not only hard to define in specification, but even more harder to implement in user agents.
There was a problem hiding this comment.
This complexity is fairly limited in the case of script transform, please have a look at how this is already implemented in WebKit.
For the sender API, I agree the usefulness is reduced, we could consider relaxing the resolution timing if it proves difficult to implement.
not only hard to define in specification
This is already defined in the spec for the script transform code path.
There was a problem hiding this comment.
I don't think anyone considered awaiting useful at TPAC, to quote the minutes:
this can be done by waiting to a keyframe in the stream before doing the key change"
With multiple rids when do you resolve?
On the first keyframe (regardless of layer) or do you wait for all of them? Return an array of promises?
What about cases like turning off the lowest simulcast layer and asking for a keyframe? Surprise (thankfully not returning an RTP timestamp you don't expect anymore).
What if the highest layer is disabled due to bandwidth restrictions?
There was a problem hiding this comment.
On the first keyframe (regardless of layer) or do you wait for all of them? Return an array of promises?
On the first keyframe of any layer would make sense.
There was a problem hiding this comment.
What usecase does resolving on a "random" keyframe solve? Also how do you avoid this keyframe being triggered by another operation such as changing the size?
| 1. If |rids| is defined, for each |rid| in rids, | ||
| 1. if |rid| is not associated with |sender|, reject Promise with an {{InvalidAccessError}} and abort these steps. | ||
| 1. Instruct the encoder associated with |sender| to generate a key frame for |rids| or all layers when |rids| is empty. | ||
| 1. Resolve |promise| with `undefined`. |
There was a problem hiding this comment.
- this should happen in a "queue a task" or "in parallel", so that it's obviously async
- we should guarantee that the promise resolves before the first of the keyframes arrives at the transform.
index.bs
Outdated
| @@ -658,7 +633,7 @@ partial interface RTCRtpSender { | |||
| The <dfn method for="RTCRtpSender">generateKeyFrame(|rids|)</dfn> method steps are: | |||
|
|
|||
| 1. Let |promise| be a new promise. | |||
| 1. [=In parallel=], run the [=generate key frame algorithm=] with |promise|, |this|'s encoder and |rids|. | |||
| 1. Run the [=generate key frame algorithm=] with |promise|, |this| and |rids|. | |||
There was a problem hiding this comment.
This patch removes in-parallel, making this algorithm synchronous while still returning a promise, which is an anti-pattern.
We'd need to either restore in-parallel or drop the promise, for this to make sense.
There was a problem hiding this comment.
Done, three fell in one swoop ;-)
|
I'll have a deeper look before next week editor's meeting. |
|
I had some more thoughts about this. We do not want to resolve the promise until this check is done, but this check will be done in parallel, close to the encoder for both requestKeyFrame variants. That removes the possibility for resolving the promise as soon as getting a key frame for one of the given rid in the transformer case. Or we would need to wait for the check result to actually start looking at key frames, which is probably not worth the extra work. |
| the resolution callbacks of the promises are always executed just before the corresponding key frame is exposed. | ||
| If the promise is associated to several rid values, it will be resolved when the first key frame corresponding to one the rid value is enqueued. | ||
| The <dfn>generate key frame algorithm</dfn>, given |promise|, |sender| and |rids|, is defined by running these steps: | ||
| 1. If the sender's transceiver kind is not `video`, reject |promise| with an {{OperationError}} and abort these steps. |
There was a problem hiding this comment.
NoSupportedError would be more adequate maybe?
| The <dfn>generate key frame algorithm</dfn>, given |promise|, |sender| and |rids|, is defined by running these steps: | ||
| 1. If the sender's transceiver kind is not `video`, reject |promise| with an {{OperationError}} and abort these steps. | ||
| 1. If |rids| is defined, for each |rid| in rids, | ||
| 1. if |rid| is not associated with |sender|, reject Promise with an {{InvalidAccessError}} and abort these steps. |
There was a problem hiding this comment.
associated with is not really clear to me. Maybe we could reuse webrtc-pc terminology. something like if sender's media stack is not configured with |rid| as active,
I also think we should reject if the sender is not actively sending (its track is null or current direction does not include send). I am not exactly sure of which wording to use, maybe something like if sender's media stack is not actively sending media.
It might also more accurate to queue a task to reject the Promise in the Promise realm event task loop, since all of those checks are done close to the encoder.
| 1. If the sender's transceiver kind is not `video`, reject |promise| with an {{OperationError}} and abort these steps. | ||
| 1. If |rids| is defined, for each |rid| in rids, | ||
| 1. if |rid| is not associated with |sender|, reject Promise with an {{InvalidAccessError}} and abort these steps. | ||
| 1. Instruct the encoder associated with |sender| to generate a key frame for |rids| or all layers when |rids| is empty. |
There was a problem hiding this comment.
We should make it clear that the promise gets resolved in the transformer context before any key frame triggered by this step is enqueued in the transformer readable stream. This PR is removing line 633-635 which was a similar statement with stronger-but-no-longer-valid guarantees.
|
This issue was mentioned in WEBRTCWG-2023-05-16 (Page 27) |
making it request a key frame for each rid or all of them if the list is empty.
Returning a promise resolving with a timestamp is not necessary since requesting a key frame is something the encoder might not be able or willing to satisfy.
馃挜 Error: connect ETIMEDOUT 173.230.149.95:443 馃挜
PR Preview failed to build. (Last tried on Jan 10, 2023, 10:54 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
馃毃 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.
馃敆 Related URL
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.