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

Spec promise<bool> IsConfigSupported() #98

Closed
chcunningham opened this issue Oct 22, 2020 · 3 comments · Fixed by #120
Closed

Spec promise<bool> IsConfigSupported() #98

chcunningham opened this issue Oct 22, 2020 · 3 comments · Fixed by #120
Assignees
Milestone

Comments

@chcunningham
Copy link
Collaborator

chcunningham commented Oct 22, 2020

This tracks spec work to offer a static asynchronous API to check whether a given configuration is supported.

Calling configure() with a valid but unsupported configuration will trigger a fatal error (see #53) via the error callback. Validity can be assessed synchronously and throw an exception. Support is harder because it may require async checks (e.g. if a given codec is only available via hardware acceleration, such acceleration typically happens in a separate process and may require IPC to verify details of support).

Our first thought was to have configure() return a promise, but this has a few drawbacks:

  1. the only other async->promise API is currently flush(), and its promise indicates that the flush has been completed. having configure() return a promise to indicate that its configuration is supported (but not necessarily applied) is inconsistent and hard to reason about.
  2. If we attempt to address 1 by only resolving once the configuration is supported and applied, realtime users may suffer visible side-effects of needing to react to a preventable failure at the last minute

So instead we offer this static async API which users should call prior to calling configure().

@chcunningham chcunningham self-assigned this Oct 23, 2020
@chcunningham chcunningham added this to the Q4 2020 milestone Oct 23, 2020
@chcunningham chcunningham linked a pull request Dec 24, 2020 that will close this issue
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 6, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 8, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 8, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 8, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612684
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841376}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 8, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612684
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841376}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 9, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement VideoDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612684
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841376}

--

wpt-commits: e265ce28838b485f20b11aee723486a035715216
wpt-pr: 27055
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jan 11, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement VideoDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612684
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841376}

--

wpt-commits: e265ce28838b485f20b11aee723486a035715216
wpt-pr: 27055
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 11, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 11, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842153}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 11, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842153}
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 11, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement VideoDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612684
Commit-Queue: Chrome Cunningham <chcunninghamchromium.org>
Reviewed-by: Thomas Guilbert <tguilbertchromium.org>
Cr-Commit-Position: refs/heads/master{#841376}

--

wpt-commits: e265ce28838b485f20b11aee723486a035715216
wpt-pr: 27055

UltraBlame original commit: 4df452a17a2f953d1ab68743d1fd7c03e3c30a7e
pull bot pushed a commit to Mu-L/chromium that referenced this issue Jan 12, 2021
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842153}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 15, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement AudioDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842153}

--

wpt-commits: 87bcad098015a7b983d29b13d2e64f33b93fbc85
wpt-pr: 27135
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 16, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement AudioDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbertchromium.org>
Commit-Queue: Chrome Cunningham <chcunninghamchromium.org>
Cr-Commit-Position: refs/heads/master{#842153}

--

wpt-commits: 87bcad098015a7b983d29b13d2e64f33b93fbc85
wpt-pr: 27135

UltraBlame original commit: e37260abb59bac6743b881820b45f60857c6bab7
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 19, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement AudioDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842153}

--

wpt-commits: 87bcad098015a7b983d29b13d2e64f33b93fbc85
wpt-pr: 27135
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 25, 2021
…(), a=testonly

Automatic update from web-platform-tests
Implement AudioDecoder.isConfigSupported()

API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbertchromium.org>
Commit-Queue: Chrome Cunningham <chcunninghamchromium.org>
Cr-Commit-Position: refs/heads/master{#842153}

--

wpt-commits: 87bcad098015a7b983d29b13d2e64f33b93fbc85
wpt-pr: 27135

UltraBlame original commit: 90e13b80191eac0e913da8f1b39931f37c74b11c
@padenot
Copy link
Collaborator

padenot commented Feb 17, 2021

So instead we offer this static async API which users should call prior to calling configure().

Whether a configuration is supported is a dynamic affair, not a static one. There is a gap of time in between getting the answer and calling configure.

If we have two calls, then it's well possible that the first call is a success and the second one is a failure (say, going in battery saving mode, unplugging a GPU, driver issue, resource contention on hw codec, etc.), or the opposite. We can decide to do it, but this needs to be clear (as an informative note or something).

@sandersdan
Copy link
Contributor

Removing the promise from configure() was a huge improvement to complexity and removed a lot of edge cases for the Chrome implementation. Specifically:

  • If we return a promise, it's only useful (compared to an error callback) if apps wait for it to resolve before enqueuing chunks.
  • This increases latency compared to other fallback strategies (unless we discard chunks queued after the configuration, which is likely to explode the state machine in the implementation and in apps).
  • This prevents us from deferring configuration changes. In some cases we don't have a signal better than 'we tried to decode a chunk and it succeeded/failed', so we want to be able to defer configuration until we have a chunk.

I vote that isConfigSupported() should be best-effort indication.

@chcunningham
Copy link
Collaborator Author

I vote that isConfigSupported() should be best-effort indication.

+1. I'll update the PR accordingly.

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3e53da5961119b4b36ec76666ad1a67cca19b963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612684
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841376}
GitOrigin-RevId: 6e5606f800fbefcc6ed776788846e82ae13e3888
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
API and motivations described here:
w3c/webcodecs#98

Spec PR here:
w3c/webcodecs#120

Bug: 1141707
Test: new layout tests
Change-Id: I3a0f3cbc2bb64a86dcefdc454d6b03e72d3f36bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622421
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842153}
GitOrigin-RevId: 456bdd17f1d32e10406636109071aec9bf1f1f6e
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 a pull request may close this issue.

3 participants