Skip to content

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Aug 5, 2025

@jan-ivar jan-ivar self-assigned this Aug 5, 2025
@fippo
Copy link
Collaborator

fippo commented Aug 5, 2025

What is the purpose of blocking the return until all requested key frames have been generated? This was previously required/done to get "a" timestamp but no longer serves any purpose?

See also #145 on the overall broken assumptions

@dontcallmedom-bot
Copy link

This issue had an associated resolution in WebRTC September 2025 meeting – 16 September 2025 (PR #276: Default the generate key frame algorithm to all layers):

RESOLUTION: proceed with merging PR

@henbos
Copy link
Collaborator

henbos commented Sep 18, 2025

Note that RIDs are optional, we only know that either all encodings have RIDs or none of them have RIDs, but we don't know that no RIDs equals singlecast - you could have multiple encodings without RIDs. The alternative would be encodingIndex but we can say that rids are a prerequisite

@jan-ivar
Copy link
Member Author

I've now updated the algorithm to work sans rids. PTAL @henbos @youennf

@jan-ivar
Copy link
Member Author

Note I also cleaned up the language a bit:

"encoder" was being used at two different conceptual levels: A) as the owner of all layers, and B) per layer. I reworked/replaced the latter with "layer" consistent with existing text in 4.10: "When transform’s [[encoder]] receives a keyframe request, ... Set rid to the RID of the appropriate layer, or undefined if the request is not for a specific layer."

1. If |encoder|.`[[pendingKeyFrameTasks]]` is undefined, initialize |encoder|.`[[pendingKeyFrameTasks]]` to an empty set.
1. Let |shouldTriggerKeyFrame| be <code>false</code> if |encoder|.`[[pendingKeyFrameTasks]]` contains a task whose `[[rid]]`
value is equal to |rid|, and <code>true</code> otherwise.
1. Remove from |layers| all layers already [=list/contained|found=] in any `[[layers]]` of any tasks in |encoder|.`[[pendingKeyFrameTasks]]`.
Copy link
Collaborator

@youennf youennf Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can do that step first before creating the pending key frame task.

1. If |frame| was generated by a video encoder identified by |task|.`[[rid]]`, run the following steps:
1. If |frame| was generated for a layer
[=list/contained=] in |task|.`[[layers]]`, remove that layer from |task|.`[[layers]]`.
If this causes |task|.`[[layers]]` to become empty, then run the following steps:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's resolve the promise as soon as we have a frame, that way, whether there are multiple key frames or just one, it is consistent.

Copy link
Collaborator

@youennf youennf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll file a separate issue about the promise resolution timing, maybe we are too smart and we should simplify further the algorithm.

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

Successfully merging this pull request may close these issues.

6 participants