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)

defect

Tracking

()

RESOLVED WORKSFORME
mozilla28
Tracking Status
firefox27 --- affected
firefox28 --- affected

People

(Reporter: jesup, Assigned: jesup)

Details

(Keywords: regression, Whiteboard: [getusermedia][webrtc-uplift])

Attachments

(1 file, 3 obsolete files)

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.
Attachment #825439 - Flags: review?(anant)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/80bfcf2d28c7
Needs aurora uplift ASAP
Whiteboard: [getusermedia] → [getusermedia][webrtc-uplift]
Target Milestone: --- → mozilla28
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.
Attached patch interdiffs to resolve leak (obsolete) — Splinter Review
Updated full patch
Reworked it to make the MediaEngines refcounted (silly that they weren't in the first place).
Attachment #825770 - Attachment is obsolete: true
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)
Attachment #825769 - Attachment is obsolete: true
Attachment #825439 - Attachment is obsolete: true
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+
(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.
Also beware of the thread-switch in one of my suggestions.
Attachment #826025 - Flags: review?(anant)
Steven - this may be of interest
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
Looks fully absorbed to me. Bug 853356 seems to have landed most of it somehow.
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.

Attachment

General

Created:
Updated:
Size: