w3c / webcodecs

WebCodecs is a flexible web API for encoding and decoding audio and video.
https://w3c.github.io/webcodecs/
Other
975 stars 137 forks source link

Should encoder.configure fail in case a codec is requested but the configuration contains parameters from another codec #581

Closed youennf closed 9 months ago

youennf commented 1 year ago

WPT /webcodecs/video-encoder-config.https.any.html expect the following configuration to fail, given the codec is vp8 and avc is H264 specific :

      codec: 'vp8',
      width: 640,
      height: 480,
      avc: {
        format: "annexb"
      }

There is little prose in the spec that tells how to handle codec-specific parameters that do not match with the provided codec. It seems the spec should be made clearer there.

I wonder though whether it would make more sense to ignore all codec-specific parameters except the VP8 ones (defined in codec registry entries like https://www.w3.org/TR/webcodecs-vp8-codec-registration/).

dalecurtis commented 1 year ago

Yeah, it does seem like a User Agent which doesn't implement AVC wouldn't know to reject this -- leading to different behavior -- so I think you're right that we instead need to ignore it.

dalecurtis commented 1 year ago

=>dan to clean up chrome logic and test assuming he agrees

dalecurtis commented 1 year ago

Ah, this is encoder actually, so @djuffin instead

padenot commented 1 year ago

I don't think we need to spec anything here, unknown/invalid keys in dictionaries are usually silently ignored. A UA could warn in its developer console for common mistakes.

We can also add an informative Note:.

youennf commented 1 year ago

unknown/invalid keys in dictionaries are usually silently ignored.

They are not unknown or invalid in terms of WebIDL, they just do not apply in that context.

Given the behavior we probably want (ignore) is not what is implemented in Chrome, we probably need to add something to the spec. It does not seem harmful to make it a conformance requirement since this is testable behavior.

padenot commented 1 year ago

In general, we spec what should happen, and we don't spec what shouldn't happen (because when specifying something, most things shouldn't happen).

Here, it is not specified that this should fail, so an implementation cannot have it fail and be conformant. This is already a normative requirement, if that makes sense.

For cases you describe, an informative Note: is the tool I'd reach for, because it seems useful.

Chrome seem to implement something different than the current text, but per @dalecurtis, it's probably for them to sort out.

dalecurtis commented 1 year ago

Yes, we'll clean up the test and Chromium's implementation. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1375781 to track that.

aboba commented 1 year ago

Agree with @padenot that this should not fail. The application can figure out that parts of the config were ignored by checking encoderSupport.config.

Djuffin commented 1 year ago

I fixed the test

https://github.com/web-platform-tests/wpt/commit/bc4e43a9bc8bdd769745bf9bf2c3fcd89ff954c4

padenot commented 1 year ago

Do we want to add a note here? Or we can close this.

aboba commented 1 year ago

Section 7.1 describes how to check configuration support. This section makes clear that including parameters that do not match the codec will not affect whether the configuration is determined to be supported or not. Also, there is an existing note:

"NOTE: This method will trigger a NotSupportedError if the User Agent does not support config. Authors are encouraged to first check support by calling isConfigSupported() with config. User Agents don’t have to support any particular codec type or configuration."

After re-reading Section 7.1, it seems to me that the specification is clear about how implementations should behave, and the revised test checks that. So it seems to me that we can close this issue.