Closed Bug 1088621 Opened 11 years ago Closed 6 years ago

Need constraint support for fake audio/video on test machines to test camera constraints & capabilities

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Version:
32 Branch
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1537986
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jib, Assigned: jib)

References

(Blocks 1 open bug)

Details

Attachments

(15 files, 5 obsolete files)

Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Wire up fake getSettings().
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1088621 - Test constraining video cameras by width/height/frameRate.
58 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1088621 - Put back mock failure for automated test, to fix try.
58 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1088621 - Move code from MediaEngineWebRTCAudio.cpp to MediaEngineAudioSource base class for better testing.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Fix fake audio to work with constraints.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Split audio constraints handling into shared MediaEngineMicrophoneAudioSource base class for testing.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Test constraining of microphones.
58 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1088621 - Make code compile with --disable-webrtc.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Have DefaultAudioDevice service multiple concurrent tracks.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1088621 - Update constraints test to s/NotSupportedError/TypeError/, to match spec.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Add wiggle room in gUM test for different default values on android.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Have DefaultVideoDevice service multiple concurrent tracks.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1088621 - Convert constraints tests to use async/await.
59 bytes, text/x-review-board-request
Details
How to auto-test camera constraints and capabilities? As Bug 1003274 comment 2 mentions, it adds: "the constraints algorithm for camera selection is finally wired into the camera capabilities code, so that a camera's potentially numerous sets of width/height/frameRate capabilities are taken into account when choosing or rejecting the camera in the first place. This does bupkis on a mac, so I'll need to do testing on Windows next, and write some mochitests for this, although I'll probably update the constraint syntax first (Bug 1033885) so I'll only have to write them once."
One option could be to set capabilities for the fake video stream with a special testing API so that the constraints can be evaluated against something when the fake video gets requested. The other option/question from my side would be: do we really need to test this from a mochitest? Maybe some kind of unit test is more appropriate for this (as it would hopefully would not require to add a special JS testing API).
As an exercise I wrote this mochitest which would have caught Bug 1094537 on macs at least. It works locally, and I ran a try for fun before understanding we have no cameras even on osx builders (or are they disabled)? https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3019687&repo=try I'm putting the test here for storage, as it's useful locally. It doesn't test camera *selection* per se, rather that width/height constraints work to modify the dimensions returned from the camera at all. I'm curious, do any of the builders come with cameras + mics that could be enabled, even on an individual basis? If so then tests could be written to exploit that. It seems to me that even an occasional intermittent would be preferable to silent breakage at this point. On fake cameras, I'm a fan of end-to-end testing, as I'm unsure I trust that code interacting with a fake camera would overlap significantly enough with code interacting with real cameras on all platforms, to catch us from breaking the real experience. On unit vs. mochi, not sure that makes much difference, as the work seems to be writing the fake camera down below?
Flags: needinfo?(drno)
Attachment #8519003 - Flags: feedback?(drno)
Depends on: 1094537
Comment on attachment 8519003 [details] [diff] [review] OSX mochitest for constraining video camera by width/height Review of attachment 8519003 [details] [diff] [review]: ----------------------------------------------------------------- Over all looks good to me. ::: dom/media/tests/mochitest/test_getUserMedia_constrainVideo.html @@ +15,5 @@ > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=882145">Test mozGetUserMedia Constraint Video (OSX)</a> > +<p id="display"></p> > +<div id="content" style="display: none"> > + <video id="v1" controls="controls" width="120" autoplay></video><br> > + <video id="v2" controls="controls" width="120" autoplay></video><br> What is v2 used for?
Attachment #8519003 - Flags: feedback?(drno) → feedback+
We have two Linux servers in the QA lab with real USB cameras attached to them. Not sure if it would help to run it on there?! My guess would be that the Mac build servers are just mac mini's with no camera. But I think we have Mac minis as well in the QA lab. Maybe we could attach a camera to one of them and run the tests there?
Flags: needinfo?(drno)
(In reply to Nils Ohlmeier [:drno] from comment #3) > What is v2 used for? Thanks, removed.
Attachment #8519003 - Attachment is obsolete: true
(In reply to Nils Ohlmeier [:drno] from comment #4) > We have two Linux servers in the QA lab with real USB cameras attached to > them. Not sure if it would help to run it on there?! I (or someone) would have to expand or clone the test to expect a camera other than the OSX 4:3/16:9 Facetime hybrid. That's the trouble with testing discovery APIs. Need something known to test against. I suppose the test could be generalized to try for low res followed by hires, at some cut-off, and expect them to be different. > My guess would be that the Mac build servers are just mac mini's with no > camera. But I think we have Mac minis as well in the QA lab. Maybe we could > attach a camera to one of them and run the tests there? I know this one runs on my mac, or did you mean on a regular basis?
See also bug 815002, where we enabled v4l2loopback and PulseAudio's sine-source as the input video/audio devices on our Linux testers.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #6) > (In reply to Nils Ohlmeier [:drno] from comment #4) > > My guess would be that the Mac build servers are just mac mini's with no > > camera. But I think we have Mac minis as well in the QA lab. Maybe we could > > attach a camera to one of them and run the tests there? > > I know this one runs on my mac, or did you mean on a regular basis? Yes I meant it on a regular base. Do we need to test this only on Mac, or on all platforms?
All platforms would of course be ideal, but anything would be good to start.
Depends on: 907352
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
likely the best path is the loopback devices, but support in fake devices may help too. Bumping priority as lack of good tests for constraints has bitten us before.
Rank: 35 → 22
Priority: P3 → P2
Summary: Need fake (or real?) cameras on builders to test camera constraints & capabilities → Need constraint support for fake audio/video on test machines to test camera constraints & capabilities
See Also: → 1191298
Any update on this bug? We currently have resolution and framerate automated tests turned off for Firefox because of this bug and it has bitten us. It would be great to be able to turn these tests on.
Nico -- Can you take this as your next bug? I'd love to get this into Fx48 (which uplifts to Dev Edition on April 18th). Adam -- Thanks for the info in your last comment. We'll try to get this fixed within the next 6 weeks or so.
Assignee: nobody → ngrunbaum
(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #13) > Nico -- Can you take this as your next bug? I'd love to get this into Fx48 > (which uplifts to Dev Edition on April 18th). Sure thing.
It might be sufficient to move mHardcodedCapabilities [1] from MediaEngineCameraVideoSource up to MediaEngineVideoSource. [1] http://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineCameraVideoSource.h?rev=728713d8e7c6#118
Awesome, thanks Maire.
See Also: → 1286096
Assignee: ngrunbaum → jib
Comment on attachment 8772635 [details] Bug 1088621 - Weaken assert in TextureClient::SetLastFwdTransactionId. Hi Nicolas, I was wondering if you could help me figure out why I'm tripping this assert. This patch set (which goes on top of bug 1286096) changes MediaEngineDefaultVideoSource to be a single shared source, something a comment [1] suggests used to be the case in a distant past. STR: 1. Open https://jsfiddle.net/bu2bd327/ in two tabs, 2. Click [320] to change resolution in one of the tabs. Expected result: resolution changes in both tabs. Actual result: I hit assert (repeat with different buttons if not immediate) The ids are always the same when it hits (hence me kludging the assert). With this kludge, everything seems to work ok, so is this a reasonable "fix", or is there a downside? [1] https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineDefault.cpp#581-582
Flags: needinfo?(nical.bugzilla)
Attachment #8519349 - Attachment is obsolete: true
No longer blocks: 1213517
Depends on: 1213517
Depends on: 1286096
No longer depends on: 1213517
(In reply to Jan-Ivar Bruaroey [:jib] from comment #20) > The ids are always the same when it hits (hence me kludging the assert). > With this kludge, everything seems to work ok, so is this a reasonable > "fix", or is there a downside? It is a reasonable fix and Sotaro recently landed the same fix in bug 1284074, so you just have to rebase as far as this assertion is concerned.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8772635 [details] Bug 1088621 - Weaken assert in TextureClient::SetLastFwdTransactionId. https://reviewboard.mozilla.org/r/65384/#review62516 The patch is good but the same fix landed concurrently on an other bug so I am cancelling the review to avoid confusion.
Attachment #8772635 - Flags: review?(nical.bugzilla)
Great! That teaches me to rebase more often.
Attachment #8772633 - Flags: review?(rjesup) → review+
Comment on attachment 8772633 [details] Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better. https://reviewboard.mozilla.org/r/65380/#review62558 all minor nits. Super vs SuperClass is just minor style; up to you - either way is good. ::: dom/media/webrtc/MediaEngineDefault.h:39 (Diff revision 1) > * The default implementation of the MediaEngine interface. > */ > class MediaEngineDefaultVideoSource : public nsITimerCallback, > public MediaEngineCameraVideoSource > { > + typedef MediaEngineCameraVideoSource Super; SuperClass? ::: dom/media/webrtc/MediaEngineDefault.h:117 (Diff revision 1) > class SineWaveGenerator; > > class MediaEngineDefaultAudioSource : public nsITimerCallback, > public MediaEngineAudioSource > { > + typedef MediaEngineCameraVideoSource Super; ditto ::: dom/media/webrtc/MediaEngineDefault.cpp:583 (Diff revision 1) > - > - // aMediaSource is ignored for audio devices (for now). > > - for (int32_t i = 0; i < len; i++) { > - RefPtr<MediaEngineAudioSource> source = mASources.ElementAt(i); > - if (source->IsAvailable()) { > + // We once used to create a device for each user, but have tightened in the > + // implementation to mirror a single mic instead, since the primary purpose > + // of this fake device is to stand in for real microhpones when testing microphones
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. https://reviewboard.mozilla.org/r/65382/#review62566 ::: dom/media/webrtc/MediaEngineDefault.cpp:319 (Diff revision 1) > + // Hardcode generic desktop capabilities modeled on OSX camera. > + > + if (mHardcodedCapabilities.IsEmpty()) { > + for (int i = 0; i < 9; i++) { > + webrtc::CaptureCapability c; > + c.width = 1920 - i*128; > + c.height = 1080 - i*72; > + c.maxFPS = 30; > + mHardcodedCapabilities.AppendElement(c); > + } > + for (int i = 0; i < 16; i++) { > + webrtc::CaptureCapability c; > + c.width = 640 - i*40; > + c.height = 480 - i*30; > + c.maxFPS = 30; > + mHardcodedCapabilities.AppendElement(c); > + } Wow, that's a lot of resolutions... snd amazingly small ones. I've never seen a real camera produce smaller than around 160x120; maybe you could see 80x60, but i don't remember seeing it. Not really a problem, but surprising. Perhaps Apple is still hiding reality from you... I realize this is existing code, just moved
Attachment #8772634 - Flags: review?(rjesup) → review+
https://reviewboard.mozilla.org/r/65380/#review62558 I've always used Super if it's ok with you. I found only one dxr hit for SuperClass vs. several for Super...
https://reviewboard.mozilla.org/r/65382/#review62566 > Wow, that's a lot of resolutions... snd amazingly small ones. I've never seen a real camera produce smaller than around 160x120; maybe you could see 80x60, but i don't remember seeing it. Not really a problem, but surprising. Perhaps Apple is still hiding reality from you... > > I realize this is existing code, just moved You're right, we still had QTKit when I wrote that, which would scale crazy down, which is what I based those on. I'll change it to stop at 160x120. I think I'll also stop sooner for some of the lower frame rates to simulate what we're seeing on real cameras atm.
Comment on attachment 8772633 [details] Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65380/diff/1-2/
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65382/diff/1-2/
Attachment #8772635 - Attachment is obsolete: true
Rebased and addressed feedback.
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. I changed the hardcoded modes slightly. Randell, can you take another look?
Attachment #8772634 - Flags: review+ → review?(rjesup)
Attachment #8772634 - Flags: review?(rjesup) → review+
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. https://reviewboard.mozilla.org/r/65382/#review62660
Comment on attachment 8772633 [details] Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65380/diff/2-3/
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65382/diff/2-3/
Attachment #8775016 - Flags: review?(drno) → review+
Comment on attachment 8775016 [details] Bug 1088621 - Test constraining video cameras by width/height/frameRate. https://reviewboard.mozilla.org/r/67364/#review64396 Not the easiest test to grasp, but I guess that is the price of constraints getting more and more complex. LGTM
Thanks! I fixed some try problems (in two new patches to not disturb existing reviews). This also picks up changes in bug 1286096, which this patch-set is on top of, so I can re-run try.
Comment on attachment 8772633 [details] Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65380/diff/3-4/
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65382/diff/3-4/
Comment on attachment 8775014 [details] Bug 1088621 - Wire up fake getSettings(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67360/diff/1-2/
Comment on attachment 8775015 [details] Bug 1088621 - Fix bug where NotReadableError rather than OverconstrainedError was thrown. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67362/diff/1-2/
Comment on attachment 8775016 [details] Bug 1088621 - Test constraining video cameras by width/height/frameRate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67364/diff/1-2/
Attachment #8775387 - Flags: review?(drno) → review+
Comment on attachment 8775387 [details] Bug 1088621 - Fix test_getUserMedia_constraints.html to stop gUM stream between tests https://reviewboard.mozilla.org/r/67556/#review64750
Attachment #8775388 - Flags: review?(drno) → review+
Comment on attachment 8775388 [details] Bug 1088621 - Put back mock failure for automated test, to fix try. https://reviewboard.mozilla.org/r/67558/#review64752
Comment on attachment 8775014 [details] Bug 1088621 - Wire up fake getSettings(). https://reviewboard.mozilla.org/r/67360/#review65064
Attachment #8775014 - Flags: review?(padenot) → review+
Comment on attachment 8775015 [details] Bug 1088621 - Fix bug where NotReadableError rather than OverconstrainedError was thrown. https://reviewboard.mozilla.org/r/67362/#review65068
Attachment #8775015 - Flags: review?(padenot) → review+
Comment on attachment 8772633 [details] Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65380/diff/4-5/
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65382/diff/4-5/
Comment on attachment 8775014 [details] Bug 1088621 - Wire up fake getSettings(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67360/diff/2-3/
Comment on attachment 8775015 [details] Bug 1088621 - Fix bug where NotReadableError rather than OverconstrainedError was thrown. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67362/diff/2-3/
Comment on attachment 8775016 [details] Bug 1088621 - Test constraining video cameras by width/height/frameRate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67364/diff/2-3/
Comment on attachment 8775387 [details] Bug 1088621 - Fix test_getUserMedia_constraints.html to stop gUM stream between tests Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67556/diff/1-2/
Comment on attachment 8775388 [details] Bug 1088621 - Put back mock failure for automated test, to fix try. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67558/diff/1-2/
Attachment #8777032 - Flags: review?(drno) → review+
Attachment #8777029 - Flags: review?(rjesup) → review+
Comment on attachment 8777029 [details] Bug 1088621 - Move code from MediaEngineWebRTCAudio.cpp to MediaEngineAudioSource base class for better testing. https://reviewboard.mozilla.org/r/68638/#review65826
Comment on attachment 8777030 [details] Bug 1088621 - Fix fake audio to work with constraints. https://reviewboard.mozilla.org/r/68640/#review65834
Attachment #8777030 - Flags: review?(rjesup) → review+
Attachment #8777031 - Flags: review?(rjesup) → review+
Comment on attachment 8777031 [details] Bug 1088621 - Split audio constraints handling into shared MediaEngineMicrophoneAudioSource base class for testing. https://reviewboard.mozilla.org/r/68642/#review65848 r+ assuming the %f thing is a nit to fix ::: dom/media/webrtc/MediaEngineMicrophoneAudioSource.cpp:83 (Diff revision 1) > + " mozNoiseSuppression: { min: %f, max: %f, ideal: %f }" : > + " mozNoiseSuppression: { min: %f, max: %f }"), > + c.mMozNoiseSuppression.mMin, c.mMozNoiseSuppression.mMax, > + c.mMozNoiseSuppression.mIdeal.valueOr(0))); %f? all the others here are %d... (also - is %d correct? - i.e. is mMin/etc either int or (close enough to correct) int32_t?)
https://reviewboard.mozilla.org/r/68642/#review65848 > %f? all the others here are %d... > (also - is %d correct? - i.e. is mMin/etc either int or (close enough to correct) int32_t?) Gah, sloppy cut'n'paste from width, height, frameRate. :/ Should static analysis have caught this? I wish platform would consider << ...
Comment on attachment 8772633 [details] Bug 1088621 - Make fake audio/video device a single source, to mirror cam/mic better. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65380/diff/5-6/
Comment on attachment 8772634 [details] Bug 1088621 - Move hardcoded capabilities to MediaEngineDefaultVideoSource. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65382/diff/5-6/
Comment on attachment 8775014 [details] Bug 1088621 - Wire up fake getSettings(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67360/diff/3-4/
Comment on attachment 8775015 [details] Bug 1088621 - Fix bug where NotReadableError rather than OverconstrainedError was thrown. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67362/diff/3-4/
Comment on attachment 8775016 [details] Bug 1088621 - Test constraining video cameras by width/height/frameRate. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67364/diff/3-4/
Comment on attachment 8775387 [details] Bug 1088621 - Fix test_getUserMedia_constraints.html to stop gUM stream between tests Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67556/diff/2-3/
Comment on attachment 8775388 [details] Bug 1088621 - Put back mock failure for automated test, to fix try. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67558/diff/2-3/
Comment on attachment 8777029 [details] Bug 1088621 - Move code from MediaEngineWebRTCAudio.cpp to MediaEngineAudioSource base class for better testing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68638/diff/1-2/
Comment on attachment 8777030 [details] Bug 1088621 - Fix fake audio to work with constraints. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68640/diff/1-2/
Comment on attachment 8777031 [details] Bug 1088621 - Split audio constraints handling into shared MediaEngineMicrophoneAudioSource base class for testing. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68642/diff/1-2/
Comment on attachment 8777032 [details] Bug 1088621 - Test constraining of microphones. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68644/diff/1-2/
Comment on attachment 8778013 [details] Bug 1088621 - Make code compile with --disable-webrtc. https://reviewboard.mozilla.org/r/69388/#review67650
Attachment #8778013 - Flags: review?(rjesup) → review+
Depends on: 1295352
Attachment #8775015 - Attachment is obsolete: true
Rebase.
Attachment #8775387 - Attachment is obsolete: true
Attachment #8852325 - Flags: review?(padenot)
Attachment #8852326 - Flags: review?(rjesup)
Comment on attachment 8852325 [details] Bug 1088621 - Have DefaultAudioDevice service multiple concurrent tracks. https://reviewboard.mozilla.org/r/124592/#review127268 ::: dom/media/webrtc/MediaEngineDefault.h:209 (Diff revision 1) > > TrackID mTrackID; > > - TrackTicks mLastNotify; // Accessed in ::Start(), then on NotifyPull (from MSG thread) > - > - // Created on Allocate, then accessed from NotifyPull (MSG thread) > + // mMonitor protects mLastNotifys[] and mSineGenerators[] which are > + // Created/deleted in ::Start(), then accessed in NotifyPull (from MSG thread) > + Monitor mMonitor; Please don't add more locks to the MediaStreamGraph, it is hurting our performance badly. I know we have other locks, but I'm trying to remove them, and I don't want more locks creeping in. We have a way to communicate between other threads and the MSG rendering thread (via message passing), we should use it here and remove all locks. You can get to the graph and post a message to it via the `SourceMediaStream`, from the main thread. ::: dom/media/webrtc/MediaEngineDefault.h:211 (Diff revision 1) > > - TrackTicks mLastNotify; // Accessed in ::Start(), then on NotifyPull (from MSG thread) > - > - // Created on Allocate, then accessed from NotifyPull (MSG thread) > - nsAutoPtr<SineWaveGenerator> mSineGenerator; > + // mMonitor protects mLastNotifys[] and mSineGenerators[] which are > + // Created/deleted in ::Start(), then accessed in NotifyPull (from MSG thread) > + Monitor mMonitor; > + std::map<SourceMediaStream*, TrackTicks> mLastNotifys; > + std::map<SourceMediaStream*, UniquePtr<SineWaveGenerator>> mSineGenerators; Can you use hash tables instead of maps ? Or better, can you not use those maps, and ask the MSG to create a SineWaveGenerator via message passing, and stick it on the SourceMediaStream ? ::: dom/media/webrtc/MediaEngineDefault.cpp:599 (Diff revision 1) > { > MOZ_ASSERT(aID == mTrackID); > AudioSegment segment; > // avoid accumulating rounding errors > + MonitorAutoLock lock(mMonitor); > + if (mLastNotifys.find(aSource) == mLastNotifys.end()) { Why would that happen ? ::: dom/media/webrtc/MediaEngineDefault.cpp:603 (Diff revision 1) > + MonitorAutoLock lock(mMonitor); > + if (mLastNotifys.find(aSource) == mLastNotifys.end()) { > + return; //Stray. Avoid crashing. > + } > TrackTicks desired = aSource->TimeToTicksRoundUp(AUDIO_RATE, aDesiredTime); > - TrackTicks delta = desired - mLastNotify; > + TrackTicks delta = desired - mLastNotifys[aSource]; Can't you use the track time instead of having this other map ?
Attachment #8852325 - Flags: review?(padenot)
Comment on attachment 8852325 [details] Bug 1088621 - Have DefaultAudioDevice service multiple concurrent tracks. https://reviewboard.mozilla.org/r/124592/#review127268 > Why would that happen ? If MediaEngineDefaultAudioSource::NotifyPull is called on the audio thread and blocks on the lock held by MediaEngineDefaultAudioSource::Stop() on the media thread, then once it gets the lock, Stop() will have called mLastNotifys.erase(aSource); so we need to test for existence when we wake up, otherwise it crashed for me eventually.
Comment on attachment 8852325 [details] Bug 1088621 - Have DefaultAudioDevice service multiple concurrent tracks. https://reviewboard.mozilla.org/r/124592/#review127268 > Please don't add more locks to the MediaStreamGraph, it is hurting our performance badly. I know we have other locks, but I'm trying to remove them, and I don't want more locks creeping in. > > We have a way to communicate between other threads and the MSG rendering thread (via message passing), we should use it here and remove all locks. > > You can get to the graph and post a message to it via the `SourceMediaStream`, from the main thread. I've taken a different approach, removing state instead of protecting it. PTAL
Comment on attachment 8852327 [details] Bug 1088621 - Add wiggle room in gUM test for different default values on android. https://reviewboard.mozilla.org/r/124596/#review127748 ::: modules/libpref/init/all.js:418 (Diff revision 1) > pref("media.navigator.load_adapt.low_load","0.40"); > +#ifndef DEBUG > pref("media.navigator.video.default_fps",30); > pref("media.navigator.video.default_minfps",10); > +#else > +#if defined(MOZ_WIDGET_GONK) || defined(MOZ_WIDGET_ANDROID) drop gonk
Attachment #8852327 - Flags: review?(rjesup) → review+
Comment on attachment 8852326 [details] Bug 1088621 - Update constraints test to s/NotSupportedError/TypeError/, to match spec. https://reviewboard.mozilla.org/r/124594/#review127750
Attachment #8852326 - Flags: review?(rjesup) → review+
Comment on attachment 8852328 [details] Bug 1088621 - Have DefaultVideoDevice service multiple concurrent tracks. https://reviewboard.mozilla.org/r/124598/#review127752
Attachment #8852328 - Flags: review?(rjesup) → review+
Comment on attachment 8852325 [details] Bug 1088621 - Have DefaultAudioDevice service multiple concurrent tracks. https://reviewboard.mozilla.org/r/124592/#review127998 ::: dom/media/webrtc/MediaEngineDefault.h:205 (Diff revision 2) > > TrackID mTrackID; > + int mNumStarted; > > - TrackTicks mLastNotify; // Accessed in ::Start(), then on NotifyPull (from MSG thread) > - > + // Created in ::Start(), then accessed in NotifyPull (from MSG thread) > + UniquePtr<SineWaveGenerator> mSineGenerator; This will probably trip up TSan, but it should be correct, because it's synchronized indirectly via the msg.
Attachment #8852325 - Flags: review?(padenot) → review+
Rebase.
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Depends on: 1497932

I think patches here are OBE. Closing in favor of new bug 1537986.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: