Closed
Bug 933384
Opened 11 years ago
Closed 8 years ago
fake:true for getUserMedia() broken by bug 853356
Categories
(Core :: WebRTC: Audio/Video, defect, P3)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
WORKSFORME
mozilla28
backlog | webrtc/webaudio+ |
People
(Reporter: jesup, Assigned: jesup)
Details
(Keywords: regression, Whiteboard: [getusermedia][webrtc-uplift])
Attachments
(1 file, 3 obsolete files)
8.28 KB,
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
The problem is that GetBackend() uses the cached mBackend always, ignoring aFake if mBackend is set (bad things also happen if the first stream is fake!), and also even if you fix that GetUserMediaRunnable::Run() ignores the backend passed in, and called GetBackend(mWindowID), and thus the fakeness is totally lost. Retained the ability to provide other backends (with the idea of supporting image, video, audio, or canvas sources for the stream returned by GUM()), per discussion with Anant.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #825439 -
Flags: review?(anant)
Comment 2•11 years ago
|
||
Comment on attachment 825439 [details] [diff] [review] Fix fake:true in mozGetUserMedia Review of attachment 825439 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #825439 -
Flags: review?(anant) → review+
Assignee | ||
Comment 3•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/80bfcf2d28c7 Needs aurora uplift ASAP
status-firefox27:
--- → affected
status-firefox28:
--- → affected
Whiteboard: [getusermedia] → [getusermedia][webrtc-uplift]
Target Milestone: --- → mozilla28
Comment 4•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f477a4fe4c32 - one or the other, this or bug 932215, leaked in mochitest-3 and crashtest.
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
Updated full patch
Assignee | ||
Comment 7•11 years ago
|
||
Reworked it to make the MediaEngines refcounted (silly that they weren't in the first place).
Assignee | ||
Updated•11 years ago
|
Attachment #825770 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 826025 [details] [diff] [review] Fix fake:true in mozGetUserMedia * * Anant or jib, whomever comes first - the only additional piece from the previous patch is switching to refcounting.
Attachment #826025 -
Flags: review?(jib)
Attachment #826025 -
Flags: review?(anant)
Assignee | ||
Updated•11 years ago
|
Attachment #825769 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #825439 -
Attachment is obsolete: true
Comment 9•11 years ago
|
||
Comment on attachment 826025 [details] [diff] [review] Fix fake:true in mozGetUserMedia * * Review of attachment 826025 [details] [diff] [review]: ----------------------------------------------------------------- looks good, with a possible simplification. ::: dom/media/MediaManager.cpp @@ +847,5 @@ > + MediaEngine* backend = mBackend; > + // Was a backend provided? > + if (!backend) { > + backend = mManager->GetBackend(mWindowID); > + } Confusingly, there are two mBackend's in this patch. The one here is private to this Runnable, so this would be simpler: // Was a backend provided? if (!mBackend) { mBackend = mManager->GetBackend(mWindowID); } or move it to the constructor: mBackend(aBackend? aBackend : mManager->GetBackend(aWindowID)), @@ +852,4 @@ > > // Was a device provided? > if (!mDeviceChosen) { > + nsresult rv = SelectDevice(backend); Then you don't need this change, @@ +923,5 @@ > return NS_OK; > } > > nsresult > + SelectDevice(MediaEngine* backend) or this, @@ +928,3 @@ > { > if (mConstraints.mPicture || mConstraints.mVideo) { > + ScopedDeletePtr<SourceSet> sources (GetSources(backend, or this, @@ +940,5 @@ > LOG(("Selected video device")); > } > > if (mConstraints.mAudio) { > + ScopedDeletePtr<SourceSet> sources (GetSources(backend, or this.
Attachment #826025 -
Flags: review?(jib) → review+
Comment 10•11 years ago
|
||
(In reply to Jan-Ivar Bruaroey [:jib] from comment #9) > mBackend(aBackend? aBackend : mManager->GetBackend(aWindowID)), If you decide to do this, don't forget to move mManager above mBackend.
Comment 11•11 years ago
|
||
Also beware of the thread-switch in one of my suggestions.
Assignee | ||
Updated•11 years ago
|
Attachment #826025 -
Flags: review?(anant)
Assignee | ||
Comment 12•10 years ago
|
||
Steven - this may be of interest
Comment 13•9 years ago
|
||
Is this still relevant? If so, can we land this patch? (I suspect this was solved long ago, though perhaps bits of the patch are still useful)
backlog: --- → webRTC+
Rank: 35
Flags: needinfo?(rjesup)
Priority: -- → P3
Comment 14•9 years ago
|
||
Looks fully absorbed to me. Bug 853356 seems to have landed most of it somehow.
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(rjesup)
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•