778801 - Add support for setting and managing video resolution in getUserMedia (part of Constraints)

Status: RESOLVED INCOMPLETE

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

Description

[Anant Narayanan [:anant]]

In the current implementation we return the first available "capability", which is usually the lowest resolution in response to a getUserMedia call for video. jesup's suggestion is to return the largest available resolution <= 640x480 for the time being.

This mechanism will change when we write a more sophisticated chooser that should be based on a variety of factors such as destination <video>'s size, feedback from PeerConnection etc. but is good enough for now.

Comment 1

[Paul Neave]

VGA would be a better default minimum. WebRTC's recommended resolution of CIF 352x288 is very low res, not 4:3 aspect ratio, and the majority of cameras are capable of VGA minimum.

Comment 2

[Anant Narayanan [:anant]]

Attachment: WIP - v0.1

Work in progress patch for reference.

Comment 3

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

Attachment: Select best resolution available given a target (except on Mac); add logging

unbitrot's anants patch and extends it - adds optional logging, if no resolutions are at the requested size or below, take the next above.  Adds a minFPS value, so we can reject very-high-but-slow resolutions (currently defaults to 640x480, minFPS 10).

Comment 4

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

Comment on attachment 666051 [details] [diff] [review]
Select best resolution available given a target (except on Mac); add logging

Note: this is just the first stab at solving constraints and selecting resolutions, but it gives us a clear way to set resolution.

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 666051 [details] [diff] [review]
Select best resolution available given a target (except on Mac); add logging

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

Mostly minor things, but this needs another go.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +24,5 @@
>  
>  namespace mozilla {
>  
>  MediaEngineWebRTCVideoSource::MediaEngineWebRTCVideoSource(webrtc::VideoEngine* videoEnginePtr,
>                                                             int index, int aFps)

If this is really aMinFps now, it should be named as such (like it is in the header).

@@ +82,5 @@
>    }
>  
>    for (int i = 0; i < num; i++) {
> +#ifdef DEBUG
> +    const unsigned int kMaxDeviceNameLength = 128; // XXX FIX!

Is this Google code? Why the kFoo constant names?

@@ +93,5 @@
> +    int error = ptrViECapture->GetCaptureDevice(i, deviceName,
> +                                                kMaxDeviceNameLength, uniqueId,
> +                                                kMaxUniqueIdLength);
> +    if (error) {
> +			LOG((" VieCapture:GetCaptureDevice: Failed %d", 

Trailing whitespace. Also crazy indentation (if you're going to insist on using tabs in source code, tab stops are 8 spaces).

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +117,3 @@
>  
>    int mCapIndex;
> +  bool mCapChosen;

So the "Cap" in mCapIndex stands for "Capture Device", while the "Cap" in "mCapChosen" stands for "Capability". The former is totally unrelated to the latter, but having the same abbreviation and putting them right next to each other is only going to cause confusion.

@@ +117,4 @@
>  
>    int mCapIndex;
> +  bool mCapChosen;
> +  int mWidth, mHeight; // XXX unsigned

I don't think these should be unsigned.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +84,5 @@
> +  if (num <= 0) {
> +    // Set to default values
> +    mCap.width  = mOpts.mWidth  = aWidth;
> +    mCap.height = mOpts.mHeight = aHeight;
> +    mCap.maxFPS = mOpts.mMaxFPS = 30;

You've already initialized mFps to the constant 30. Either you should be using that value here, or you should have a named constant for it.

@@ +85,5 @@
> +    // Set to default values
> +    mCap.width  = mOpts.mWidth  = aWidth;
> +    mCap.height = mOpts.mHeight = aHeight;
> +    mCap.maxFPS = mOpts.mMaxFPS = 30;
> +                  mOpts.codecType = kVideoCodecI420;

Tabs.

@@ +110,5 @@
> +        higher = false;
> +      }
> +    } else {
> +      if (cap.width > aWidth || cap.height > aHeight || cap.maxFPS < aMinFPS) {
> +        break;

break? Not continue?

@@ +121,5 @@
> +        // FIXME: expose expected capture delay?
> +      }
> +    }
> +  }
> +  mCapChosen = true;

If num==0 or every capability has, e.g., (mOpts.mWidth > cap.width && mOpts.mHeight < cap.height), mCap will be uninitialized (even assuming you change the "break" above).

@@ +129,3 @@
>  MediaEngineWebRTCVideoSource::GetName(nsAString& aName)
>  {
> +  aName.Assign(NS_ConvertASCIItoUTF16(mDeviceName));

The WebRTC.org documentation says this is UTF-8, not ASCII. Using the ASCII converters is a bad idea anyway (while you're here, I believe MediaEngineWebRTCAudio also uses them).

@@ +133,5 @@
>  
>  void
>  MediaEngineWebRTCVideoSource::GetUUID(nsAString& aUUID)
>  {
> +  aUUID.Assign(NS_ConvertASCIItoUTF16(mUniqueId));

See above.

@@ +143,5 @@
>    if (mState != kReleased) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (!mCapChosen) {

This guard should be in ChooseCapability() itself. Otherwise, you need to at a minimum assert !mCapChosen at the start of ChooseCapability().

@@ +179,5 @@
>  {
> +  if (!mCapChosen) {
> +    ChooseCapability(mWidth, mHeight, mMinFps);
> +  }
> +  return mOpts;

Could you return a const reference instead?

@@ +355,5 @@
>      return;
>    }
>  
> +  memset(mDeviceName, 0, KMaxDeviceNameLength);
> +  memset(mUniqueId, 0, KMaxUniqueIdLength);

Should these be in the constructor instead? Otherwise, what happens when, e.g., GetUUID() is called before Start()?

Comment 6

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

> @@ +82,5 @@
> >    }
> >  
> >    for (int i = 0; i < num; i++) {
> > +#ifdef DEBUG
> > +    const unsigned int kMaxDeviceNameLength = 128; // XXX FIX!
> 
> Is this Google code? Why the kFoo constant names?

This code interfaces to webrtc code and uses some style and symbols from there and other code that uses it.  Those kXxxx values are used elsewhere in getUserMedia code already; however this debug block was copied (and cleaned up some) from media/webrtc/signaling

> @@ +93,5 @@
> > +    int error = ptrViECapture->GetCaptureDevice(i, deviceName,
> > +                                                kMaxDeviceNameLength, uniqueId,
> > +                                                kMaxUniqueIdLength);
> > +    if (error) {
> > +			LOG((" VieCapture:GetCaptureDevice: Failed %d", 
> 
> Trailing whitespace. Also crazy indentation (if you're going to insist on
> using tabs in source code, tab stops are 8 spaces).

Sorry, cut/paste from sipcc.  Fixed.

> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +117,3 @@
> >  
> >    int mCapIndex;
> > +  bool mCapChosen;
> 
> So the "Cap" in mCapIndex stands for "Capture Device", while the "Cap" in
> "mCapChosen" stands for "Capability". The former is totally unrelated to the
> latter, but having the same abbreviation and putting them right next to each
> other is only going to cause confusion.

Renamed all of them... mCaptureIndex, mCapability, mCapabilityChosen.

> 
> @@ +117,4 @@
> >  
> >    int mCapIndex;
> > +  bool mCapChosen;
> > +  int mWidth, mHeight; // XXX unsigned
> 
> I don't think these should be unsigned.

Most of the functions they get passed to are unsigned.  The only advantage I see to signed is it makes checking for underflow conditions easier (but we never do any math on these values).  However, it also doesn't hurt anything except cause more signed->unsigned warnings (and casts to suppress them).

> 
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +84,5 @@
> > +  if (num <= 0) {
> > +    // Set to default values
> > +    mCap.width  = mOpts.mWidth  = aWidth;
> > +    mCap.height = mOpts.mHeight = aHeight;
> > +    mCap.maxFPS = mOpts.mMaxFPS = 30;
> 
> You've already initialized mFps to the constant 30. Either you should be
> using that value here, or you should have a named constant for it.

Done, and for min fps as well (10 is what I chose).

> @@ +85,5 @@
> > +    // Set to default values
> > +    mCap.width  = mOpts.mWidth  = aWidth;
> > +    mCap.height = mOpts.mHeight = aHeight;
> > +    mCap.maxFPS = mOpts.mMaxFPS = 30;
> > +                  mOpts.codecType = kVideoCodecI420;
> 
> Tabs.

That one is spaces; I by habit was lining up the structure members.
 
> @@ +110,5 @@
> > +        higher = false;
> > +      }
> > +    } else {
> > +      if (cap.width > aWidth || cap.height > aHeight || cap.maxFPS < aMinFPS) {
> > +        break;
> 
> break? Not continue?

Thanks - Bug I carried through when I updated the patch.

> 
> @@ +121,5 @@
> > +        // FIXME: expose expected capture delay?
> > +      }
> > +    }
> > +  }
> > +  mCapChosen = true;
> 
> If num==0 or every capability has, e.g., (mOpts.mWidth > cap.width &&
> mOpts.mHeight < cap.height), mCap will be uninitialized (even assuming you
> change the "break" above).

Not for num=0 because in that mCap.xx = mOpts.xx = aXx block above I set the default.  And we start with higher = true, and should accept the first cap entry we see by default (regardless of fps) - but the code doesn't do that (originally it grabbed entry num-1 as the default before entering the loop, but as I changed the algorithm to hunt up and down I got rid of that).  With that mod, we never can end up without mCapability set (renamed value).  I'll add an i == 0 || (the rest) to the test and add a comment.  Thanks.

> @@ +129,3 @@
> >  MediaEngineWebRTCVideoSource::GetName(nsAString& aName)
> >  {
> > +  aName.Assign(NS_ConvertASCIItoUTF16(mDeviceName));
> 
> The WebRTC.org documentation says this is UTF-8, not ASCII. Using the ASCII
> converters is a bad idea anyway (while you're here, I believe
> MediaEngineWebRTCAudio also uses them).

Good catch (pre-existing in the patch and Audio code).

> @@ +143,5 @@
> >    if (mState != kReleased) {
> >      return NS_ERROR_FAILURE;
> >    }
> >  
> > +  if (!mCapChosen) {
> 
> This guard should be in ChooseCapability() itself. Otherwise, you need to at
> a minimum assert !mCapChosen at the start of ChooseCapability().

Added an NS_WARN_IF_FALSE(), as this is merely inefficient to evaluate again.

> 
> @@ +179,5 @@
> >  {
> > +  if (!mCapChosen) {
> > +    ChooseCapability(mWidth, mHeight, mMinFps);
> > +  }
> > +  return mOpts;
> 
> Could you return a const reference instead?

Yes, makes more sense (the Default class was using a local).  Also, found the default FPS value there was 10 instead of 30.

> 
> @@ +355,5 @@
> >      return;
> >    }
> >  
> > +  memset(mDeviceName, 0, KMaxDeviceNameLength);
> > +  memset(mUniqueId, 0, KMaxUniqueIdLength);
> 
> Should these be in the constructor instead? Otherwise, what happens when,
> e.g., GetUUID() is called before Start()?

To be honest, they also shouldn't be memsets.  webrtc.org does it all over the place in their test code, but not so much in production code.

Comment 7

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

Attachment: Select best resolution available given a target (except on Mac); add logging

Comment 8

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

> > @@ +355,5 @@
> > >      return;
> > >    }
> > >  
> > > +  memset(mDeviceName, 0, KMaxDeviceNameLength);
> > > +  memset(mUniqueId, 0, KMaxUniqueIdLength);
> > 
> > Should these be in the constructor instead? Otherwise, what happens when,
> > e.g., GetUUID() is called before Start()?
> 
> To be honest, they also shouldn't be memsets.  webrtc.org does it all over
> the place in their test code, but not so much in production code.

Also, they're cleared (and GetCaptureDevice() called) in ::Init() which is called from the constructor (ok, kinda odd), so they are valid always.

Comment 9

[Timothy B. Terriberry (:derf)]

Comment on attachment 666258 [details] [diff] [review]
Select best resolution available given a target (except on Mac); add logging

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

Much better.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +23,5 @@
>  #include "ImageContainer.h"
>  
>  namespace mozilla {
>  
> +MediaEngineWebRTCVideoSource::MediaEngineWebRTCVideoSource(webrtc::VideoEngine* aVideoEnginePtr,

I don't understand why this function is in this file, instead of in MediaEngineWebRTCVideo.cpp (that was why I didn't see that it called Init() before).

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +361,5 @@
>    if (mViECapture == NULL || mViERender == NULL) {
>      return;
>    }
>  
> +  mDeviceName[0] = '\0'; // paranoia

Okay, I see this function is called from the constructor, but with the early returns it can still leave these uninitialized. I suggest moving them to the top of the function for true paranoia.

Comment 10

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

http://hg.mozilla.org/projects/alder/rev/0da68d581d1f

With the two mods suggested by derf

Leaving open since this patch in no way solves the entire issue.

Comment 11

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

Nominate to re-check blocking status

Comment 12

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

This bug is no longer really useful; constraints will be handled in other bugs