Closed Bug 1307630 Opened 4 years ago Closed 4 years ago

Media with both audio and video tracks return the audio settings for the video track's getSettings()

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed

People

(Reporter: sheppy, Assigned: jib)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

When you use getUserMedia() to access a camera and ask for both audio and video tracks, then call the video track's getSettings() function, the results are actually the settings for the audio track.

See this fiddle: https://jsfiddle.net/frev5wvm/
Rank: 15
Component: WebRTC → WebRTC: Audio/Video
OS: Unspecified → All
Priority: -- → P1
Hardware: Unspecified → All
Assignee: nobody → jib
[Tracking Requested - why for this release]:

Embarrassing bug in track.getSettings(), new feature in 50.

Found in soon-to-be MDN example (URL). Hoping to uplift this super-trivial fix.
Blocks: 1286096
No longer blocks: 1213517
Comment on attachment 8797834 [details]
Bug 1307630 - Have videoTrack.getSettings() return settings for video, not audio, from source with both.

https://reviewboard.mozilla.org/r/83442/#review82088
Attachment #8797834 - Flags: review?(rjesup) → review+
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2da81945cf4e
Have videoTrack.getSettings() return settings for video, not audio, from source with both. r=jesup
Tracking 52+ as this is an issue relating to a new feature.
Adding dev-doc-needed since it will impact docs if it doesn't get accepted for 50.
Keywords: dev-doc-needed
Comment on attachment 8797834 [details]
Bug 1307630 - Have videoTrack.getSettings() return settings for video, not audio, from source with both.

Approval Request Comment
[Feature/regressing bug #]: Bug 1286096

[User impact if declined]: 

Whenever camera and mic are obtained together, calls to micTrack.getSettings() returns video settings instead of audio settings.

This is a black eye on the new track.getSettings() feature going out in 50.

It is featured prominently in an MDN demo [1] which currently leaves audio blank to avoid embarrassment.

[Describe test coverage new/current, TreeHerder]:

Landed on m-c. Tested and verified manually. All code paths are already covered by audio-only and video-only sources. 

[Risks and why]:

Extremely low risk. Basically forgot to pass in a boolean to return the right settings when source contains both audio and video, exposing no new code that hasn't run before.

(mTrackID is a glorified boolean seen at the lone two places of instantiation [2] to be either kAudioTrack or kVideoTrack.)

[String/UUID change made/needed]: none

[1] https://developer.mozilla.org/en-US/docs/Web/API/Media_Streams_API/Constraints#Result
[2] https://dxr.mozilla.org/mozilla-central/rev/ea104eeb14cc54da9a06c3766da63f73117723a0/dom/media/MediaManager.cpp#1213
Attachment #8797834 - Flags: approval-mozilla-beta?
Attachment #8797834 - Flags: approval-mozilla-aurora?
Sure, let's fix that before the release. Tracking for now, we will probably accept it once it is in m-c.
https://hg.mozilla.org/mozilla-central/rev/2da81945cf4e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Hi Jan-Ivar, this seems like a pretty basic scenario, is it possible to add an automated test for it? Thanks!
Flags: needinfo?(jib)
Comment on attachment 8797834 [details]
Bug 1307630 - Have videoTrack.getSettings() return settings for video, not audio, from source with both.

Fixes a regression, Aurora51+, Beta50+
Attachment #8797834 - Flags: approval-mozilla-beta?
Attachment #8797834 - Flags: approval-mozilla-beta+
Attachment #8797834 - Flags: approval-mozilla-aurora?
Attachment #8797834 - Flags: approval-mozilla-aurora+
Hi Ritu, the tests for this are in bug 1088621, currently blocked on bug 1295352 which keeps bouncing. I'll devote some time again to get those landed.

The tests are https://reviewboard.mozilla.org/r/67364/diff/4#index_header for video
and https://reviewboard.mozilla.org/r/68644/diff/2#index_header for audio.

They didn't catch this since they test audio and video separately, so I'll add a third one to bug 1088621 to test audio+video combined.
Flags: needinfo?(jib)
Revised the constraint exerciser example to do both audio and video, and removed the note explaining why audio was disabled.
You need to log in before you can comment on or make changes to this bug.