1004396 - Make peerconnection video bitrates configurable

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

Default bitrates should always have been configurable...

Comment 1

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

Attachment: Make video codec default bitrates configurable for WebRTC

Comment 2

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

Comment on attachment 8418896 [details] [diff] [review]
Make video codec default bitrates configurable for WebRTC

Also removes some tabs that got added, and a few mis-indents

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 8418896 [details] [diff] [review]
Make video codec default bitrates configurable for WebRTC

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +203,5 @@
>    CSFLogDebug(logTag,  "%s this=%p other=%p", __FUNCTION__, this, other);
>  
> +#ifdef MOZILLA_INTERNAL_API
> +  // already know we must be on MainThread barring unit test weirdness
> +  if (NS_IsMainThread()) {

If that's true, why isn't this an assert? the unit tests do not define MOZILLA_INTERNAL_API

@@ +206,5 @@
> +  // already know we must be on MainThread barring unit test weirdness
> +  if (NS_IsMainThread()) {
> +    nsresult rv;
> +    nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
> +    if (NS_SUCCEEDED(rv))

Can this fail? It should also be an assert.

@@ +215,5 @@
> +      {
> +        branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable);
> +        branch->GetIntPref("media.peerconnection.video.min_bitrate", &mMinBitrate);
> +        branch->GetIntPref("media.peerconnection.video.start_bitrate", &mStartBitrate);
> +        branch->GetIntPref("media.peerconnection.video.max_bitrate", &mMaxBitrate);

These all seem like they can fail, so there should be error checks, right?

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8418896 [details] [diff] [review]
Make video codec default bitrates configurable for WebRTC

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +215,5 @@
> +      {
> +        branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable);
> +        branch->GetIntPref("media.peerconnection.video.min_bitrate", &mMinBitrate);
> +        branch->GetIntPref("media.peerconnection.video.start_bitrate", &mStartBitrate);
> +        branch->GetIntPref("media.peerconnection.video.max_bitrate", &mMaxBitrate);

If the argument here is that they remain unchanged, then maybe just an assert so we catch failure to store them?

Comment 5

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

(In reply to Eric Rescorla (:ekr) from comment #4)
> Comment on attachment 8418896 [details] [diff] [review]
> Make video codec default bitrates configurable for WebRTC
> 
> Review of attachment 8418896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +215,5 @@
> > +      {
> > +        branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable);
> > +        branch->GetIntPref("media.peerconnection.video.min_bitrate", &mMinBitrate);
> > +        branch->GetIntPref("media.peerconnection.video.start_bitrate", &mStartBitrate);
> > +        branch->GetIntPref("media.peerconnection.video.max_bitrate", &mMaxBitrate);
> 
> If the argument here is that they remain unchanged, then maybe just an
> assert so we catch failure to store them?

We can; it doesn't actually buy us anything.  Since those keep the (safe) defaults if that were to somehow fail, I don't see much being gained by doing so.  Generally with prefs if there are safe defaults we ignore the result of GetXxxxPref().

> > +#ifdef MOZILLA_INTERNAL_API
> > +  // already know we must be on MainThread barring unit test weirdness
> > +  if (NS_IsMainThread()) {
>
> If that's true, why isn't this an assert? the unit tests do not define MOZILLA_INTERNAL_API

I moved the code and added more prefs; it was that way before.  Likely cruft due to the unit tests not running on MainThread.... perhaps with ehugg's work we can relax that (or do so soon).  I'll swap it to an #ifdef'd assert.

> > +    nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
> > +    if (NS_SUCCEEDED(rv))
>
> Can this fail? It should also be an assert.

Everyplace else I looked does something like NS_ENSURE_SUCCESS (and a few places don't check).  None I saw asserted on it.  I can do NS_WARN_IF if you like.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #5)
> (In reply to Eric Rescorla (:ekr) from comment #4)
> > Comment on attachment 8418896 [details] [diff] [review]
> > Make video codec default bitrates configurable for WebRTC
> > 
> > Review of attachment 8418896 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > @@ +215,5 @@
> > > +      {
> > > +        branch->GetBoolPref("media.video.test_latency", &mVideoLatencyTestEnable);
> > > +        branch->GetIntPref("media.peerconnection.video.min_bitrate", &mMinBitrate);
> > > +        branch->GetIntPref("media.peerconnection.video.start_bitrate", &mStartBitrate);
> > > +        branch->GetIntPref("media.peerconnection.video.max_bitrate", &mMaxBitrate);
> > 
> > If the argument here is that they remain unchanged, then maybe just an
> > assert so we catch failure to store them?
> 
> We can; it doesn't actually buy us anything.  Since those keep the (safe)
> defaults if that were to somehow fail, I don't see much being gained by
> doing so.  Generally with prefs if there are safe defaults we ignore the
> result of GetXxxxPref().

My thinking here is that this would detect these being accidentally removed
from all.js. Right now, they are duplicated.


> > > +#ifdef MOZILLA_INTERNAL_API
> > > +  // already know we must be on MainThread barring unit test weirdness
> > > +  if (NS_IsMainThread()) {
> >
> > If that's true, why isn't this an assert? the unit tests do not define MOZILLA_INTERNAL_API
> 
> I moved the code and added more prefs; it was that way before.  Likely cruft
> due to the unit tests not running on MainThread.... perhaps with ehugg's
> work we can relax that (or do so soon).  I'll swap it to an #ifdef'd assert.

SGTM.


> > > +    nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
> > > +    if (NS_SUCCEEDED(rv))
> >
> > Can this fail? It should also be an assert.
> 
> Everyplace else I looked does something like NS_ENSURE_SUCCESS (and a few
> places don't check).  None I saw asserted on it.  I can do NS_WARN_IF if you
> like.

OK.

Comment 7

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

Attachment: Make video codec default bitrates configurable for WebRTC

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 8420474 [details] [diff] [review]
Make video codec default bitrates configurable for WebRTC

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

r+ once signed/unsigned agreement fixed.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +195,5 @@
>    return result;
>  }
>  
>  /**
> + * Peforms initialization of the MANDATORY components of the Video Engine

Fix typo. "Performs"

@@ +1138,5 @@
>      cinst.maxFramerate = codecInfo->mMaxFrameRate;
>    }
> +  cinst.minBitrate = mMinBitrate;
> +  cinst.startBitrate = mStartBitrate;
> +  cinst.maxBitrate = mMaxBitrate;

These are unsigned in VideoCodec, but the members are signed.

    unsigned int        startBitrate;  // kilobits/sec.
    unsigned int        maxBitrate;  // kilobits/sec.
    unsigned int        minBitrate;  // kilobits/sec.

It appears that GetIntPref() takes an int32_t, so probably
best to make the members unsigned int and then range check
the values returned from GetIntPref.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4947f68a3c0d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4947f68a3c0d