Repository navigation
ENG-3433: Ask the platform for the preferred audio input in getUserMedia - #118
dragan-n-dream wants to merge 1 commit into
Conversation
…ENG-3433) Car head units can take a phone's microphone stream and pause the session, or let a game record from the car's microphone. When the platform announces it in ready (audioInputExchangeEnabled: true), getUserMedia now asks which audio input to use (requestPreferredAudioInputDevice / preferredAudioInputDevice) before handing the stream to the game, reports the devices after each resolve (audioInputDevicesReported) and reports failures the platform would not otherwise learn of (userMediaRequestFailed). The permission timeout becomes 60 s of unpaused time and stops while the screen is paused. Without the flag getUserMedia is unchanged. Changed in airconsole-1.11.0.js in place and copied to beta/airconsole-1.12.0.js.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Platform side (ENG-3436, airconsole-appengine): https://github.com/AirConsole/airconsole-appengine/pull/3739 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6c21b5ab5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| me.preferredAudioInputDeviceHandler_ = null; | ||
| me.mediaPermissionStreams_ = []; | ||
| // Taken per request, so a later ready cannot switch flows while a request is pending. | ||
| me.mediaPermissionUsesExchange_ = me.audioInputExchangeEnabled_ === true; |
There was a problem hiding this comment.
Keep the new exchange out of the released 1.11 bundle
Adding the exchange flow to airconsole-1.11.0.js retroactively changes the public bundle already served to games pinned to 1.11.0: when the platform enables the flag, those games now receive different device-selection, timeout, and destroy() behavior. The same implementation is already staged in beta/airconsole-1.12.0.js, so the released 1.11 bundle should remain unchanged and this feature should ship with the next version instead.
AGENTS.md reference: AGENTS.md:L13-L17
Useful? React with 👍 / 👎.
| * wall-clock time, as before the exchange existed. | ||
| * @private | ||
| */ | ||
| const BASE_PERMISSION_TIMEOUT_MS = 45000; |
There was a problem hiding this comment.
| const BASE_PERMISSION_TIMEOUT_MS = 45000; | |
| const MEDIA_PERMISSION_TIMEOUT_MS = 45000; |
| const PERMISSION_TIMEOUT_MS = 60000; | ||
|
|
||
| /** | ||
| * The getUserMedia timeout when the platform does not announce the preferred audio input device exchange: counted in |
There was a problem hiding this comment.
This comment is misleading and no clue what "wall clock time" is, simpler suggestion: // Timeout for getUserMedia if the player doesn't respond to the permission request within this period.
| * denial overlay. | ||
| * @private | ||
| */ | ||
| const PERMISSION_TIMEOUT_MS = 60000; |
There was a problem hiding this comment.
Way too much comment which is unclear and also the name isn't clear what this is defining, below is the actual timeout for the getUserMedia, why is this needed?
| me.preferredAudioInputDeviceHandler_ = null; | ||
| me.mediaPermissionStreams_ = []; | ||
| // Taken per request, so a later ready cannot switch flows while a request is pending. | ||
| me.mediaPermissionUsesExchange_ = me.audioInputExchangeEnabled_ === true; |
There was a problem hiding this comment.
This shouldn't be needed, it should be stateless for the API so that the platform sends the right flow events to the Game API, not the Game API deciding on top of the platform but with the same flag.
There was a problem hiding this comment.
Agreed.
While the long running promise is a form of state that is unavoidable for this functionality to exist, it does not need a 'per request' state as the respective boolean flag will be true or false for the length of the game session as the controller can not switch between screens without the game being restarted completely.
Also the documented claim is wrong, putting doubt on the necessity of the intent.
Aside of the point above that per request can not happen, there is also the factually incorrect claim that there can be a later ready.
That can not be. The getUserMedia function checks that the ready event has been received, otherwise it throws an error (see
airconsole-api/airconsole-1.11.0.js
Line 853 in 53d7be9
marc-n-dream
left a comment
There was a problem hiding this comment.
There are quite a few things that need to be addressed and I propose we do it in a pair programming session to discuss details as we address them.
In particular that the PR feels the need to track multiple getUserMedia requests indicates that it does not uphold the APIs expectations in the way they are intended and API documented.
Note on writing JSDoc in this repository: A JSDoc describes what a function does and the parameter / return are.
A JSDoc must never describe how it is being done, that is implementation detail and represented through the code and subject to change independent of the public APIs JSDoc potentially
| * @private | ||
| */ | ||
| AirConsole.prototype.expireMediaPermission_ = function expireMediaPermission_() { | ||
| this.mediaPermissionTimeout_ = undefined; |
There was a problem hiding this comment.
before a timeout reference is overwritten with undefined I expect an if not undefined then clearTimeout block.
We want to use timeout / interval sparingly but when we use them we want to make sure that they are used consistently - no clearing of timeouts or intervals without setting them undefined and no setting them undefined without first clearing them if they were defined.
| devices: devices, | ||
| activeDeviceId: AirConsole.getActiveAudioInputDeviceId_(stream1), | ||
| }, function (deviceId) { | ||
| const track = stream1.getAudioTracks()[0]; |
There was a problem hiding this comment.
This needs to check if the stream and the audio track actually exist before using it as it otherwise breaks game flow logic by throwing an error in the depths of this flow.
i understand that you verify stream1 before but this is within a promise somewhen later and accessing a resource this promise resolution flow does not own and control.
| * @param {number} requestId | ||
| * @param {string} handOffType - 'promptUserMediaPermission' (web) or 'userMediaPermissionGranted' (native app). | ||
| */ | ||
| AirConsole.prototype.startFirstMediaPermissionRequest_ = function startFirstMediaPermissionRequest_(requestId, |
There was a problem hiding this comment.
The newly introduced split if first request vs later request is not desirable.
While this is necessary for a special subset of screen environments controlled by the new flag, that small subset of the total userbase is no justification to degrade the correct behavior and user experience of everyone because there is no need to override the microphone used in any other case.
Given that a significant part of the 450 addition depends on it, this is something that does not fit into a PR comment.
| ### Added | ||
|
|
||
| - Added `AirConsole.requestPhotonEngineAuth()` and `onPhotonEngineAuth(ticket)` so screens can authenticate with Photon Custom Authentication. | ||
| - `AirConsole.getUserMedia` asks the platform which audio input a stream should use before handing it to the game |
There was a problem hiding this comment.
Can this be rephrased to meet the requirements of the CHANGELOG?
The CHANGELOG is API user facing. The information covers public API offered by the AirConsole prototype and changes / bug fixes to it but not implementation details.
| */ | ||
| AirConsole.getConstraintsWithAudioInputDevice_ = function getConstraintsWithAudioInputDevice_(constraints, deviceId) { | ||
| if (!deviceId) { | ||
| return constraints; |
There was a problem hiding this comment.
this control flow does not look correct.
If the provided constraints where to include a deviceId assignment, shouldn't it be removed instead of being left alone?
| me.preferredAudioInputDeviceHandler_ = null; | ||
| me.mediaPermissionStreams_ = []; | ||
| // Taken per request, so a later ready cannot switch flows while a request is pending. | ||
| me.mediaPermissionUsesExchange_ = me.audioInputExchangeEnabled_ === true; |
There was a problem hiding this comment.
Agreed.
While the long running promise is a form of state that is unavoidable for this functionality to exist, it does not need a 'per request' state as the respective boolean flag will be true or false for the length of the game session as the controller can not switch between screens without the game being restarted completely.
Also the documented claim is wrong, putting doubt on the necessity of the intent.
Aside of the point above that per request can not happen, there is also the factually incorrect claim that there can be a later ready.
That can not be. The getUserMedia function checks that the ready event has been received, otherwise it throws an error (see
airconsole-api/airconsole-1.11.0.js
Line 853 in 53d7be9
| me.startBaseMediaPermissionRequest_(type); | ||
| return; | ||
| } | ||
| // The hand-off is accepted once per request, so a duplicate hand-off cannot open a second stream. |
There was a problem hiding this comment.
the flow chain here is not exactly readable.
From what I see, everything from this line to line 2154 where the current type if-block ends should probably be a within a function dedicated to this alternative flow where the device id is forced instead of using the browsers provided one.
| } | ||
| ); | ||
| if (!me.mediaPermissionUsesExchange_) { | ||
| me.startBaseMediaPermissionRequest_(type); |
There was a problem hiding this comment.
there is no such concept as a BaseMediaPermissionRequest
Isn't this just startDefaultMediaPermissionReuqest(...)?
| me.mediaPermissionRemainingMs_ -= Date.now() - me.mediaPermissionTimerStartedAt_; | ||
| me.mediaPermissionTimerStartedAt_ = undefined; | ||
| } | ||
| } else if (!running) { |
There was a problem hiding this comment.
the if is not necessary here.
| return; | ||
| } | ||
| const running = me.mediaPermissionTimeout_ !== undefined; | ||
| if (me.isScreenPaused_()) { |
There was a problem hiding this comment.
This breaks AirConsole.getUserMedia in case of a onPause.
If the getUserMedia is pending when this is is executed, the correct closure of the promise becomes conditional on an unpause happening before the game state changes.
We don't control the game state as such we need to make sure to uphold guaranteed contracts so game developers can rely on the API.
Description
AirConsole.getUserMediaasks the platform which audio input a stream should use before it hands the stream to thegame, so a controller connected to a car can keep a game on the phone's built-in microphone. It runs only when the
platform announces it in
ready(audioInputExchangeEnabled: true), which the platform does only on a car screen;otherwise
getUserMediais unchanged.requestPreferredAudioInputDevice→preferredAudioInputDevice(always answered,null= nopreference).
audioInputDevicesReportedafter every resolve,userMediaRequestFailedfor a timeout or anopen error.
destroy()ends a pending request like a timeout.airconsole-1.11.0.jsin place, copied tobeta/airconsole-1.12.0.js; both run the sharedspec-usermedia-permissions.js.Jira
ENG-3433. The platform side is ENG-3436 (airconsole-appengine), reviewed together with this one; its submodule points
at this branch until this pull request is merged. Please keep the
drg/eng-3433branch until the appengine pullrequest has moved its pointer to the merge.
Testing
tests/spec/methods/spec-usermedia-permissions.jscover the exchange, the timeout, the pause-awareclock, failures and the unchanged flow without the flag.
cd ci && npm test: all versions pass.Android controllers (results in ENG-3436).