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

Should VideoEncoderConfig cloning remove parameters that are not useful for a given codec? #681

Open
youennf opened this issue Jun 14, 2023 · 8 comments
Labels
CR Blocking Needs to resolved for Candidate Recommendation Ready for PR

Comments

@youennf
Copy link
Contributor

youennf commented Jun 14, 2023

According https://w3c.github.io/webcodecs/#clone-configuration, the configuration should be cloned, hence all members of VideoEncoderSupport should be copied.

Chrome in https://wpt.live/webcodecs/video-encoder-config.https.any.html assumes that the avc member will not be cloned if the codec is vp8. Safari simply clones all members it knows.

The note in https://w3c.github.io/webcodecs/#dom-videoencoder-isconfigsupported says that only recognised dictionary members will be copied. recognised could be interpreted both ways.
It would be good to clarify the intent here.

@sandersdan
Copy link
Contributor

The intent was to support feature detection. More generally, the returned configuration should be the user agent's interpretation of the supplied configuration.

I would say that the spec as written implies that all known IDL properties should be copied.

For the purpose of a developer debugging WebCodecs operation, I'd say that Chrome's behavior is perhaps more helpful.

Based on that, I would vote for Chrome's behavior, but it might not have an obvious algorithm to specify.

@youennf
Copy link
Contributor Author

youennf commented Jun 22, 2023

The intent was to support feature detection

I would think feature detection is done at WebIDL level, parameters not recognised will not be copied.
The only case would be a feature supported for one codec but not for another codec.
Is that a real use case?

Overall, we might end up in bad interop here between browsers, which might be more hurtful than anything else.
I would vote for either defining an algorithm or keeping it simple like the spec is today and wait for web developer feedback.

@aboba aboba added the agenda Add to Media WG call agenda label Jun 22, 2023
@sandersdan
Copy link
Contributor

The only case would be a feature supported for one codec but not for another codec.
Is that a real use case?

I suppose that's the key question. Right now I don't think we have any options that make sense to support only in some codecs, except for the explicitly codec-specific options. I can imagine bitrate control getting complex enough to have variability, but that hasn't happened yet.

I think I'm comfortable with either fixing Chrome to match the current spec, or changing the spec to exclude coded-specific options that do not apply to codec.

@youennf
Copy link
Contributor Author

youennf commented Jul 3, 2023

I think I'm comfortable with either fixing Chrome to match the current spec

I think I would prefer this at it is simpler to spec and implement.
We can always revisit whenever we see a real need.

@chrisn
Copy link
Member

chrisn commented Jul 3, 2023

In the last Media WG meeting, we discussed adding hints for screen content coding tools, which would be applied in codecs that support them. Would this affect cloning? Maybe not, as it's just a hint?

@Djuffin
Copy link
Contributor

Djuffin commented Jul 6, 2023

I think feature detection won't suffer if we just clone every field known to the user agent. Codec specific fields that belong to a different codec don't have any effect anyway.

As for hints, they need to be copied if the user agent knows about them regardless if they had any real effect on configuration of the underlying encoder.

@Djuffin
Copy link
Contributor

Djuffin commented Jul 11, 2023

Media WG discussion on 7/11/2023:

  1. Make spec more explicit that even fields that don't affect configuration (like codec specific fields) need to be copied even if they don't affect configuration in this particular case.
  2. File a bug against Chromium

@Djuffin Djuffin added Ready for PR and removed agenda Add to Media WG call agenda labels Jul 12, 2023
youennf added a commit to youennf/web-platform-tests that referenced this issue Sep 5, 2023
…dec specific fields

Update test according decision from w3c/webcodecs#681
@youennf
Copy link
Contributor Author

youennf commented Sep 5, 2023

WPT PR up for review.

youennf added a commit to web-platform-tests/wpt that referenced this issue Sep 5, 2023
…dec specific fields (#41824)

Update test according decision from w3c/webcodecs#681
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 13, 2023
…onfiguration needs to copy codec specific fields, a=testonly

Automatic update from web-platform-tests
https://w3c.github.io/webcodecs/#clone-configuration needs to copy codec specific fields (#41824)

Update test according decision from w3c/webcodecs#681
--

wpt-commits: 7d62dd075bbd5535cd6a90028a85b5f6534d083d
wpt-pr: 41824
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this issue Sep 14, 2023
…onfiguration needs to copy codec specific fields, a=testonly

Automatic update from web-platform-tests
https://w3c.github.io/webcodecs/#clone-configuration needs to copy codec specific fields (#41824)

Update test according decision from w3c/webcodecs#681
--

wpt-commits: 7d62dd075bbd5535cd6a90028a85b5f6534d083d
wpt-pr: 41824
Lightning00Blade pushed a commit to Lightning00Blade/wpt that referenced this issue Dec 11, 2023
@aboba aboba added the CR Blocking Needs to resolved for Candidate Recommendation label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR Blocking Needs to resolved for Candidate Recommendation Ready for PR
Projects
None yet
Development

No branches or pull requests

5 participants