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

Allow newly created codecs to be reclaimed if the system is out of resources #836

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

Djuffin
Copy link
Contributor

@Djuffin Djuffin commented Sep 12, 2024

This PR addresses #669


Preview | Diff

index.src.html Outdated
@@ -6035,7 +6035,8 @@
received a call to `encode()`, `decode()`, `configure()`, `flush()` or `reset()`
in the past `10 seconds`, or has called its `output()` callback in the past `10
seconds`. Addionally, {{VideoEncoder}}s are considered [=active=] if they are
making progress in encoding queued {{VideoFrame}}s.
making progress in encoding queued {{VideoFrame}}s. Newly created codecs are not
considered [=active=] until they call an output callback for the first time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure? I'd have expected input not output here.

Copy link
Collaborator

@aboba aboba Sep 13, 2024

Choose a reason for hiding this comment

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

I think that "output callback" is intended. A suggestion: change "they call an output callback for the first time" to "the first output callback" to make this more clear.

A decode() or encode() method could error if resources have been reclaimed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, what I'm saying is that this is in contradiction with the previous sentence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, yes. Just calling encode() or decode() is not sufficient if they error (which they can do if resources are not available).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sentence that I added is intended to mark a newly created coded inactive before it actually got some work done. I do realize that it looks like the new condition contradicts the previous one. I expect that readers will realize that the last condition supersedes the previous ones, but if it looks misleading I'm open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we can't have contradiction in normative text. Let's proceed step by step here:

  • Say one of the method mentioned is called. Two things can happen: it can succeed or fail. If it fails, the codec isn't considered active, abort those steps.
  • A message is queued. Two things can happen: it can execute or note. If it cannot execute, it means that other things are executing, and so the codec is already active, abort those steps.
  • If it executes, then we can mark it as active, we'll e.g. configure a decoder, or submit a frame for encode/decode. It can be that there will be no output for some time: decoder and encoder latency can be very large, and encoding time can also be extremely long.
  • When an output callback fires, we should also consider the codec as active, to let script enqueue other tasks, as it is often how the encoding/decoding is driven.

what is important is therefore if one of the methods enqueue a control message successfully OR having an output callback, and that should be our criteria for marking a codec as active.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until the codec successfully processes at least one input it can't be considered fully initialized, because encoding or decoding might use lazy resource allocation.
It means that we can't really be sure that we have sufficient system resources until we have the first output ready.

That's why I think that merely having a control message enqueues shouldn't be enough for the codec to be marked as active. After producing the first output processing control messages should be enough to keep the codec active.

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Accepts", not process, I'd say, but sure. There can be seconds between the two.

index.src.html Outdated
@@ -6035,7 +6035,8 @@
received a call to `encode()`, `decode()`, `configure()`, `flush()` or `reset()`
in the past `10 seconds`, or has called its `output()` callback in the past `10
seconds`. Addionally, {{VideoEncoder}}s are considered [=active=] if they are
making progress in encoding queued {{VideoFrame}}s.
making progress in encoding queued {{VideoFrame}}s. Newly created codecs are not
considered [=active=] until they call an output callback for the first time.
Copy link
Collaborator

@aboba aboba Sep 13, 2024

Choose a reason for hiding this comment

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

I think that "output callback" is intended. A suggestion: change "they call an output callback for the first time" to "the first output callback" to make this more clear.

A decode() or encode() method could error if resources have been reclaimed.

@Djuffin
Copy link
Contributor Author

Djuffin commented Oct 2, 2024

@padenot @aboba
I simplified definition of an active codec. It's a bit more vague but

  • closer to how it actually works in real life
  • we already used this definition of activity for VideoEncoder
  • aligned with what we discussed at TPAC 2024

Copy link
Collaborator

@padenot padenot left a comment

Choose a reason for hiding this comment

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

That should be permissive enough for implementing something useful in practice, thanks!

@Djuffin Djuffin merged commit a2770c1 into w3c:main Oct 8, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Oct 8, 2024
SHA: a2770c1
Reason: push, by Djuffin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to sandersdan/webcodecs that referenced this pull request Oct 14, 2024
SHA: a2770c1
Reason: push, by sandersdan

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to sandersdan/webcodecs that referenced this pull request Oct 14, 2024
SHA: a2770c1
Reason: push, by sandersdan

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to sandersdan/webcodecs that referenced this pull request Oct 14, 2024
SHA: a2770c1
Reason: push, by sandersdan

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to sandersdan/webcodecs that referenced this pull request Oct 14, 2024
SHA: a2770c1
Reason: push, by sandersdan

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to sandersdan/webcodecs that referenced this pull request Oct 14, 2024
SHA: a2770c1
Reason: push, by sandersdan

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants