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

Is https://w3c.github.io/webcodecs/#valid-audiodecoderconfig complete enough? #714

Closed
youennf opened this issue Sep 1, 2023 · 5 comments · Fixed by #780
Closed

Is https://w3c.github.io/webcodecs/#valid-audiodecoderconfig complete enough? #714

youennf opened this issue Sep 1, 2023 · 5 comments · Fixed by #780
Assignees

Comments

@youennf
Copy link
Contributor

youennf commented Sep 1, 2023

According WPT https://github.com/web-platform-tests/wpt/blob/master/webcodecs/audio-decoder.https.any.js, Chrome seems to do more validation than what the spec currently says in https://w3c.github.io/webcodecs/#valid-audiodecoderconfig.
See also https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webcodecs/audio_decoder.cc;l=132;drc=6f4c64436342c818aa41e6a5c55034e74ec9c6b6

Some of Chrome's checks make sense to me (require non null numeric values for some parameters for instance) but they are not part of the spec.
There are also some codec specific checks (description being required and having specific values) but there, we are doing asynchronous failures for video (and returning not supported for isConfigSupported) so it might be more consistent to do this as well here.

I haven't looked at AudioEncoder but the same thing might happen.

@philn who is looking at that on WebKit side.
@padenot, @dalecurtis.

@dalecurtis
Copy link
Contributor

dalecurtis commented Sep 1, 2023

Ah, I didn't realize the sampleRate/channelCount are non-zero wasn't in the spec. We perform that check on AudioDataInit so I think it makes sense to add it to the config: https://w3c.github.io/webcodecs/#valid-audiodatainit

Being consistent on description presence also sgtm. Maybe @sandersdan recalls some reason why we didn't check video description presence synchronously.

@philn
Copy link

philn commented Sep 1, 2023

Same goes for the description check on flac/vorbis configs, Blink checks those, but the spec doesn't mention it, and I don't think there's a WPT test for this yet either.

@sandersdan
Copy link
Contributor

sandersdan commented Sep 1, 2023

description validity depends on other parameters, but the initial check is only supposed to be about basic sanity. It's not clear exactly where the line should be, but I try to treat it something like "extra requirements that the IDL can't quite capture".

I think non-zero/non-null checks make sense. Codec-specific checks do not.

Edit: I would say that isConfigSupported() promise rejection should imply an error in the application, not just an issue with the media content.

@philn
Copy link

philn commented Sep 2, 2023

I haven't looked at AudioEncoder but the same thing might happen.

It does indeed, webcodecs/audio-encoder-config.https.any.html has validation tests for sampleRate, numberOfChannels and bitrate but the spec doesn't define those.

@padenot
Copy link
Collaborator

padenot commented Jan 31, 2024

On the encode side, this sounds right to me, we should check at least sample-rate and number of channels. This is what the WPT expect. A non-specified bitrate would pick the default for the codec. Codec-specific settings that aren't accepted (e.g. a bitrate of 1kpbs for Opus when implemented with libopus) can fail as usual for codec-specific configuration options.

On the decode side, I just submitted a PR to check that description isn't detached, but the sample-rate, format and channel count is only required for some codecs (depending on the codec).

@padenot padenot self-assigned this Mar 14, 2024
padenot added a commit that referenced this issue Mar 15, 2024
…in a valid AudioEncoderConfig

They are not required for decoding -- this depends on the codec.

This fixes #714.
padenot added a commit that referenced this issue Mar 19, 2024
…in a valid AudioEncoderConfig

They are not required for decoding -- this depends on the codec.

This fixes #714.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants