Support prefs for default gum capture size

RESOLVED FIXED in Firefox 22

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21 unaffected, firefox22 fixed)

Details

(Whiteboard: [getUserMedia][blocking-gum-][qa-])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Default is 640x480@30fps, but some uses might want lower values.  Provide a default pref until (or even after) it's made adaptive.
(Assignee)

Comment 1

5 years ago
Created attachment 715646 [details] [diff] [review]
add prefs for default gUM resolution
(Assignee)

Comment 2

5 years ago
Comment on attachment 715646 [details] [diff] [review]
add prefs for default gUM resolution

Works fine for <= 640x480 on my linux cam (Logitech 615).  Green or flashing green above that.... not sure why, but comes to DeliverFrame() that way.

Will need testing on other cameras.
Attachment #715646 - Flags: review?(tterribe)
Comment on attachment 715646 [details] [diff] [review]
add prefs for default gUM resolution

Review of attachment 715646 [details] [diff] [review]:
-----------------------------------------------------------------

What about all the uses of DEFAULT_WIDTH and DEFAULT_HEIGHT in MediaEngineDefault.[h|cpp]?

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +40,5 @@
>  {
>    webrtc::ViEBase* ptrViEBase;
>    webrtc::ViECapture* ptrViECapture;
>    // We spawn threads to handle gUM runnables, so we must protect the member vars
>    MutexAutoLock lock(mMutex);

Can we move the lock down until the point where we start accessing member variables?

@@ +47,5 @@
> +  int32_t height = MediaEngineWebRTCVideoSource::DEFAULT_VIDEO_HEIGHT;
> +  int32_t fps    = MediaEngineWebRTCVideoSource::DEFAULT_VIDEO_FPS;
> +  int32_t minfps = MediaEngineWebRTCVideoSource::DEFAULT_VIDEO_MIN_FPS;
> +
> +  // FIX - these should be passed in originating in prefs and/or getUserMedia constraints

Please file a bug for this and include the bug number here.
Attachment #715646 - Flags: review?(tterribe) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 715897 [details] [diff] [review]
add prefs for default gUM resolution
(Assignee)

Updated

5 years ago
Attachment #715646 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Comment on attachment 715897 [details] [diff] [review]
add prefs for default gUM resolution

Not moving the lock, since I want any contenders to stall before accessing prefs.
Attachment #715897 - Flags: review?(tterribe)
Comment on attachment 715897 [details] [diff] [review]
add prefs for default gUM resolution

Review of attachment 715897 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but see nits.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +395,5 @@
>    MutexAutoLock lock(mMutex);
>    int32_t found = false;
>    int32_t len = mVSources.Length();
>  
> +  int32_t width  = MediaEngineDefaultVideoSource::DEFAULT_VIDEO_WIDTH;

I'm not super-excited about duplicating all this code in two places, especially with the hard-coded string constants. Is there somewhere common we can put it where both can use it?

@@ +413,5 @@
> +      branch->GetIntPref("media.navigator.video.default_height", &height);
> +      branch->GetIntPref("media.navigator.video.default_fps", &fps);
> +    }
> +  }
> +  //  LOG(("%s: %dx%d @%dfps", __FUNCTION__, width, height, fps));

Either uncomment this or remove it.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +30,5 @@
>     unsigned int w, unsigned int h, unsigned int streams)
>  {
>    mWidth = w;
>    mHeight = h;
> +  LOG(("Video FrameSizeChange: %u/%u", w, h));

%ux%u would make a lot more sense.
Attachment #715897 - Flags: review?(tterribe) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3150c558e0

Nits fixed.  de-duplication can happen through the bug to support constraint setting of resolution (where the resolution will come from above in MediaManager)
Target Milestone: --- → mozilla22
Rebased version landed on alder:

https://hg.mozilla.org/projects/alder/rev/6eed44261112

Updated

5 years ago
Blocks: 843469
https://hg.mozilla.org/mozilla-central/rev/fa3150c558e0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][qa-]
The preferences service isn't threadsafe (bug 619487) and all this code runs off the main thread.  You need to back this out and figure out another way to do this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 619487
(Assignee)

Comment 11

5 years ago
Created attachment 718913 [details] [diff] [review]
Refactor gUM prefs use to be off mainthread, and prepare for constraints
(Assignee)

Comment 12

5 years ago
Comment on attachment 718913 [details] [diff] [review]
Refactor gUM prefs use to be off mainthread, and prepare for constraints

And yes, I did intend this time to change DEFAULT_VIDEO_FPS to 30.   Fairly large changeset, which was why I didn't do all this to start (was going to wait for per-getUserMedia constraint support).
Attachment #718913 - Flags: review?(tterribe)
(Assignee)

Updated

5 years ago
status-firefox21: --- → unaffected
status-firefox22: --- → affected
Whiteboard: [getUserMedia][blocking-gum-][qa-] → [getUserMedia][blocking-gum-]
(Assignee)

Comment 13

5 years ago
Created attachment 718936 [details] [diff] [review]
Refactor gUM prefs use to be off mainthread, and prepare for constraints
(Assignee)

Updated

5 years ago
Attachment #718913 - Attachment is obsolete: true
Attachment #718913 - Flags: review?(tterribe)
(Assignee)

Comment 14

5 years ago
Comment on attachment 718936 [details] [diff] [review]
Refactor gUM prefs use to be off mainthread, and prepare for constraints

Added all.js
Attachment #718936 - Flags: review?(tterribe)
I figured out a work around for Bug 832677, so this is the only thing blocking threadsafety assertions for the pref service now.
Comment on attachment 718936 [details] [diff] [review]
Refactor gUM prefs use to be off mainthread, and prepare for constraints

Review of attachment 718936 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 842715: Refactor gUM prefs use to be off mainthread, and prepare for constraints

I thought the idea was to use the prefs service _on_ the main thread.

In any case, I think there are a number of things seriously broken here, so r- for now.

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +61,5 @@
>    }
>  
> +  mOpts.mWidth = (uint32_t) aPrefs.mWidth;
> +  mOpts.mHeight = (uint32_t) aPrefs.mHeight;
> +  mOpts.mMaxFPS = (uint32_t) aPrefs.mMinFPS;

Why are you setting mMaxFPS to mMinFPS?

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +64,5 @@
>      int aIndex,
> +    int aWidth  = MediaEngine::DEFAULT_VIDEO_WIDTH,
> +    int aHeight = MediaEngine::DEFAULT_VIDEO_HEIGHT,
> +    int aFps    = MediaEngine::DEFAULT_VIDEO_FPS,
> +    int aMinFps = MediaEngine::DEFAULT_VIDEO_MIN_FPS)

mMinFps is no longer used for anything. You should kill it entirely.

For the rest of these... it seems like initializing them to apparently-valid values is just a red herring. These values should no longer do anything. The real values should be chosen by ChooseCapability(), which, it appears, does not update them. Maybe we are always expecting FrameSizeChange() to get called?

In any case, I think they should be initialized to something invalid (e.g., -1). That should make it much easier to spot errors where they're being used without being set properly.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +205,4 @@
>  {
>    LOG((__FUNCTION__));
> +  if (mState == kReleased && mInitDone) {
> +    // XXX  if shared, allow a later opener to affect the resolution!

Please file a bug about this and include the bug number here.

@@ +210,2 @@
>  
> +    // Note: allow to override earlier GetOptions() call

See below. This is broken anyway thanks to the mCapabilityChosen guard in ChooseCapability() itself.

@@ +262,5 @@
>    return NS_OK;
>  }
>  
>  const MediaEngineVideoOptions*
>  MediaEngineWebRTCVideoSource::GetOptions()

AFAICT, the only code which used this function is the code to share default sources that you just killed above. I think you should kill GetOptions() entirely, because otherwise this is kind of a mess.

::: dom/media/MediaManager.cpp
@@ +1223,5 @@
> +MediaManager::GetPref(nsIPrefBranch *aBranch, const char *aPref,
> +                      const char *aData, int32_t *aVal)
> +{
> +  int32_t temp;
> +  if (aData == nullptr || strcmp(aPref,aData) == 0) {

I don't think aData can be NULL.

@@ +1240,5 @@
>  
> +  if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
> +    nsCOMPtr<nsIPrefBranch> branch( do_QueryInterface(aSubject) );
> +    if (branch) {
> +      const char *pref = NS_ConvertUTF16toUTF8(aData).get();

If I understand C++ lifetime rules correctly, NS_ConvertUTF16toUTF8's destructor gets called as soon as the full expression containing it is finished being evaluated. That means every use of pref after the initial assignment is a use-after-free.
Attachment #718936 - Flags: review?(tterribe) → review-
(In reply to Timothy B. Terriberry (:derf) from comment #16)
> Comment on attachment 718936 [details] [diff] [review]
> Refactor gUM prefs use to be off mainthread, and prepare for constraints
> 
> Review of attachment 718936 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > Bug 842715: Refactor gUM prefs use to be off mainthread, and prepare for constraints
> 
> I thought the idea was to use the prefs service _on_ the main thread.

Yes, that's the idea.  Not sure how we got confused here.
(Assignee)

Comment 18

5 years ago
Created attachment 719369 [details] [diff] [review]
Refactor gUM prefs use to be on mainthread, and prepare for constraints
(Assignee)

Updated

5 years ago
Attachment #718936 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Comment on attachment 719369 [details] [diff] [review]
Refactor gUM prefs use to be on mainthread, and prepare for constraints

Since mOpts are gone, cleaned up ChooseCapability as well
Attachment #719369 - Flags: review?(tterribe)
(Assignee)

Comment 20

5 years ago
Created attachment 719703 [details] [diff] [review]
Refactor gUM prefs use to be on mainthread, and prepare for constraints

minor fix for warning
(Assignee)

Updated

5 years ago
Attachment #719369 - Attachment is obsolete: true
Attachment #719369 - Flags: review?(tterribe)
(Assignee)

Updated

5 years ago
Attachment #719703 - Flags: review?(tterribe)
Comment on attachment 719703 [details] [diff] [review]
Refactor gUM prefs use to be on mainthread, and prepare for constraints

Review of attachment 719703 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +61,5 @@
>    virtual int DeliverFrame(unsigned char*, int, uint32_t, int64_t);
>  
>    MediaEngineWebRTCVideoSource(webrtc::VideoEngine* aVideoEnginePtr,
>      int aIndex,
> +    int aWidth  = -1,

Actually looking at ViEExternalRendererImpl, I guess these should be initialized to 0 (since it initializes its own tracking variables to 0, it won't call our FrameSizeChange in that case).

But my actual point was that we should initialize them unconditionally, not as default arguments. Specifying values here doesn't actually do anything.

@@ +64,5 @@
>      int aIndex,
> +    int aWidth  = -1,
> +    int aHeight = -1,
> +    int aFps    = MediaEngine::DEFAULT_VIDEO_FPS,
> +    int aMinFps = MediaEngine::DEFAULT_VIDEO_MIN_FPS)

These won't do anything, either (even if you follow the plan bug 846188 outlined below), and should also be set to something invalid.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +54,5 @@
>      LOG(("DeliverFrame: video not started"));
>      return 0;
>    }
>  
> +  MOZ_ASSERT(mWidth*mHeight*3/2 == size);

NS_ENSURE_TRUE?

@@ +125,5 @@
>    LOGFRAME(("NotifyPull, desired = %ld, target = %ld, delta = %ld %s", (int64_t) aDesiredTime, 
>              (int64_t) target, (int64_t) delta, image ? "" : "<null>"));
> +
> +  // Bug 846188 We may want to limit incoming frames to the requested frame rate
> +  // mFPS - if you want 30FPS, and the camera gives you 60FPS, this could

mFps
Attachment #719703 - Flags: review?(tterribe) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2de61abbce1d
https://hg.mozilla.org/mozilla-central/rev/2de61abbce1d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [getUserMedia][blocking-gum-] → [getUserMedia][blocking-gum-][qa-]

Updated

5 years ago
status-firefox22: affected → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 843469
You need to log in before you can comment on or make changes to this bug.