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

Status: RESOLVED FIXED

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

Description

[Eric Shepherd [:sheppy]]

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/

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1307630 - Have videoTrack.getSettings() return settings for video, not audio, from source with both.

Review commit: https://reviewboard.mozilla.org/r/83442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83442/

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[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.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2da81945cf4e
Have videoTrack.getSettings() return settings for video, not audio, from source with both. r=jesup

Comment 5

[Marcia Knous [:marcia]]

Tracking 52+ as this is an issue relating to a new feature.

Comment 6

[Eric Shepherd [:sheppy]]

Adding dev-doc-needed since it will impact docs if it doesn't get accepted for 50.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Sure, let's fix that before the release. Tracking for now, we will probably accept it once it is in m-c.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2da81945cf4e

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jan-Ivar, this seems like a pretty basic scenario, is it possible to add an automated test for it? Thanks!

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/388d917d5fdd

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/efb8e679a23c

Comment 15

[Eric Shepherd [:sheppy]]

Revised the constraint exerciser example to do both audio and video, and removed the note explaining why audio was disabled.