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

RTCRtpSender.getCapabilities() may not return correct information in sync #49

Closed
alvestrand opened this issue Jun 15, 2020 · 25 comments
Closed
Assignees
Labels
wontfix This will not be worked on

Comments

@alvestrand
Copy link
Collaborator

Chrome has found a case where getCapabilities() is going to have issues with delivering correct capability information about codec availability synchronously.

The two immediately obvious workarounds are:

  • Fail the call with a LATER error code (but since it's a static method, there's no place to fire a "later has arrived" event at)
  • Change the signature to async

The latter would be preferable from an architectural viewpoint, but has some compatibility issues.

@henbos
Copy link
Collaborator

henbos commented Jun 15, 2020

IMO this API should have been made asynchronous in the first place; codec capabilities means querying HW which means "not synchronous".

While you could still implement this with blocking-invokes I am not in favor of that. Besides, since this reveals information about the system, having it be promise-based would make sense for privacy reasons as well in case a browser wants to implement a prompt (different discussion but worth bringing up if we address this at the same time).

Suggestion:

  • Make a promise-based version of the API with a different name like queryCapabilities() or something.
  • Deprecate the synchronous version of the API.

@fippo
Copy link
Contributor

fippo commented Jun 15, 2020

@aboba you might remember the original intention to make it sync in ORTC...

@aboba
Copy link
Contributor

aboba commented Jun 15, 2020

@fippo. We made it a synchronous static method because:

  • No permission prompts were contemplated for getCapabilities().
  • The capabilities were static, not dynamic (e.g. did not depend on the particular RtpSender or RtpReceiver object), so it could be a static method.
  • The capabilities were time invariant. It was assumed that a codec implemented in hardware would always also be available in software (possibly with reduced performance). As a result, the same response would be returned for every RtpSender.getCapabilites() method call and each RtpReceiver.getCapabilites() call would return the same response. As a result, the response to a getCapabilites() call could be pre-determined, cached and returned immediately.

@fippo
Copy link
Contributor

fippo commented Jun 15, 2020

@aboba thank you! The problem probably boils down to chrome on android not supporting h264 software encoding as a fallback.

https://bugs.chromium.org/p/chromium/issues/detail?id=719023 -- Hotlist-Recharge-Cold is another way of saying "this is gonna bite", right?

@alvestrand
Copy link
Collaborator Author

The reason the Chrome bug was an issue is that the process on Android to figure out whether or not a H.264 encoder is available takes time. It will return the same answer every time on the same hardware, but until WebRTC asks about it, it is not going to be known to the underlying system.

So we have a short interval in which the reply to "do you support H.264 encode" is "I don't know yet". Depending on the answer resolving one way or the other is not a Good Thing.

@aboba
Copy link
Contributor

aboba commented Jun 16, 2020

Since the same information (support for encode/decode) is needed when createOffer() is called, why not cache the answer then? That would allow subsequent getCapabilities() calls to return immediately, without querying the hardware again.

@lgrahl
Copy link

lgrahl commented Jun 17, 2020

Seems like a rabbit hole to me to leave it synchronous. If there's one bit where the information is not available immediately, there's usually another bit with the same problem just around the corner. @henbos' suggestions sound like a good compromise to me.

@henbos
Copy link
Collaborator

henbos commented Jun 17, 2020

@aboba Caching after this has already been obtained asynchronously by other APIs seems like a sensible optimization (and nothing would prevent you from returning a cached result in a promise immediately), but considering there is no requirement that the app invoke and await createOffer() prior to calling getCapabilities(), having this be synchronous doesn't make sense. I am not in favor of having one API throw an exception unless another API on a different interface is called first.

@dontcallmedom
Copy link
Member

@henbos, when you say "deprecate" getCapabilities(), what do you mean exactly? is it removing it? throwing an exception on usage? just showing a warning in the console?

If the latter, I'm still unclear what happens when the method is called with no synchronous answer available. If one of the others, is it Web compatible?

@henbos
Copy link
Collaborator

henbos commented Jun 17, 2020

Once we have an asynchronous alternative we can simply remove getCapabilities() as far as the spec is concerned unless we want to document its behavior in legacy interface extensions section.

For backwards compatibility, until it is safe to remove it from implementations without breaking the web platform, its probably best to let implementations support getCapabilities() the way it is implemented today. I.e. add a deprecation warning but continue to return whatever the implementation is able to know synchronously.

@henbos
Copy link
Collaborator

henbos commented Jun 17, 2020

Alternatively we fix this in an extension spec and let getCapabilities() be whatever it is today in webrtc-pc

@lgrahl
Copy link

lgrahl commented Jun 17, 2020

I think @dontcallmedom was more asking for what you're going to return right now in the existing getCapabilities if the information is not yet available. If it enters a deprecation phase (or is left as is), it still needs to do something:

  • Block until the information is available
  • Return incomplete information
  • Throw an exception
  • ...

@alvestrand
Copy link
Collaborator Author

OperationError with an error message indicating "call the async interface if you really want info now" would be my preference.

@alvestrand alvestrand self-assigned this Jun 18, 2020
@alvestrand
Copy link
Collaborator Author

Removing privacy-tracker. The privacy aspecs are covered under another issue.

@henbos
Copy link
Collaborator

henbos commented Aug 27, 2020

The privacy issue is tracked in w3c/webrtc-pc#2460, and this one can be moved to webrtc-extensions.

@alvestrand
Copy link
Collaborator Author

The workaround for the timing issue is to call createOffer() (which is async), which can wait for the correct information to be gathered without blocking the main thread. Changing the API is an NV matter.

@jan-ivar
Copy link
Member

Any solution to this specific problem would be an API change which is not for final CR at this point. Moving to webrtc-extensions.

@alvestrand
Copy link
Collaborator Author

We seem to have another case where the sync nature of the interface causes some interesting issues: Slow initialization (?) of USB cameras. We know that the camera is there long before we know what the capabilities of the device is.

https://crbug.com/1153290

@henbos
Copy link
Collaborator

henbos commented Dec 2, 2020

See also w3c/webrtc-pc#2597

@henbos
Copy link
Collaborator

henbos commented Dec 2, 2020

@henbos
Copy link
Collaborator

henbos commented Jan 18, 2021

The result of the previous interim was to add this note:
https://github.com/w3c/webrtc-pc/pull/2617/files

It says that the capabilities in getCapabilities() and in the SDP from createOffer()/createAnswer() need to be consistent.

I think to really pinpoint when capabilities are discovered and exposed would require normative steps and is thus webrtc-extensions territory, but I don't think this needs to be tackled in the January interim. This may become a non-issue with media capabilities or if we add an async API.

@aboba
Copy link
Contributor

aboba commented Jun 17, 2021

@alvestrand Is there interest in implementing an async version of getCapabilities() if we were to specify one?

@aboba
Copy link
Contributor

aboba commented Jan 6, 2022

@alvestrand It seems like the current direction is to add functionality to Media Capabilities rather than updating RTCRtpSender.getCapabilities(). If so, should we close this issue?

@aboba aboba added the wontfix This will not be worked on label Jan 6, 2022
@youennf
Copy link
Contributor

youennf commented Jan 6, 2022

Makes sense to close it as we want to progressively remove this support (see #95)

@youennf youennf closed this as completed Jan 6, 2022
@alvestrand
Copy link
Collaborator Author

I agree. Let's focus on #95 for any new functionality, and keep getCapabilities as-is in the spec for backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

8 participants