1127642 - support H.264 max_mbps

Status: RESOLVED FIXED

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

Description

[karina]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/600.2.5 (KHTML, like Gecko) Version/8.0.2 Safari/600.2.5

Steps to reproduce:

set max_mbps for webrtc 


Actual results:

there is no any changes


Expected results:

resolution and frame rate should be updated

Comment 1

[karina]

Attachment: max-mbps

Comment 2

[karina]

Comment on attachment 8561932 [details]
max-mbps

># HG changeset patch
># Parent  3436787a82d0e1f7c4b43d8ec7b9b0be2f00bd04
>Bug 1127642
>
>diff --git a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
>--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
>+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
>@@ -71,16 +71,17 @@ WebrtcVideoConduit::WebrtcVideoConduit()
>   mEngineReceiving(false),
>   mChannel(-1),
>   mCapId(-1),
>   mCurSendCodecConfig(nullptr),
>   mSendingWidth(0),
>   mSendingHeight(0),
>   mReceivingWidth(640),
>   mReceivingHeight(480),
>+  mSendingFramerate(DEFAULT_VIDEO_MAX_FRAMERATE),
>   mVideoLatencyTestEnable(false),
>   mVideoLatencyAvg(0),
>   mMinBitrate(200),
>   mStartBitrate(300),
>   mMaxBitrate(2000)
> {
> }
> 
>@@ -669,17 +670,17 @@ WebrtcVideoConduit::ConfigureSendMediaCo
>   }
> 
>   if (!mVideoCodecStat) {
>     mVideoCodecStat = new VideoCodecStatistics(mChannel, mPtrViECodec, true);
>   }
> 
>   mSendingWidth = 0;
>   mSendingHeight = 0;
>-
>+  mSendingFramerate = video_codec.maxFramerate;
>   if(codecConfig->RtcpFbNackIsSet("")) {
>     CSFLogDebug(logTag, "Enabling NACK (send) for video stream\n");
>     if (mPtrRTP->SetNACKStatus(mChannel, true) != 0)
>     {
>       CSFLogError(logTag,  "%s NACKStatus Failed %d ", __FUNCTION__,
>                   mPtrViEBase->LastError());
>       return kMediaConduitNACKStatusError;
>     }
>@@ -1027,16 +1028,60 @@ WebrtcVideoConduit::SelectSendResolution
>       }
>       CSFLogDebug(logTag, "%s: Encoder resolution changed to %ux%u",
>                   __FUNCTION__, width, height);
>     } // else no change; mSendingWidth likely was 0
>   }
>   return true;
> }
> 
>+bool WebrtcVideoConduit::SelectSendFrameRate(unsigned int framerate){
>+    // Limit frame rate based on max-mbps
>+    mSendingFramerate = framerate;
>+    if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxMBPS){
>+        unsigned int cur_fs, mb_width, mb_height, max_fps;
>+        
>+        mb_width = (mSendingWidth + 15) >> 4;
>+        mb_height = (mSendingHeight + 15) >> 4;
>+        
>+        cur_fs = mb_width * mb_height;
>+        max_fps = mCurSendCodecConfig->mMaxMBPS/cur_fs;
>+        if(max_fps < mSendingFramerate)
>+            mSendingFramerate = max_fps;
>+        if(mCurSendCodecConfig->mMaxFrameRate < mSendingFramerate)
>+            mSendingFramerate = mCurSendCodecConfig->mMaxFrameRate;
>+    }
>+    if (mSendingFramerate != framerate)
>+    {
>+        // Get current vie codec.
>+        webrtc::VideoCodec vie_codec;
>+        int32_t err;
>+        
>+        if ((err = mPtrViECodec->GetSendCodec(mChannel, vie_codec)) != 0)
>+        {
>+            CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
>+            return false;
>+        }
>+        if (vie_codec.maxFramerate != mSendingFramerate)
>+        {
>+            vie_codec.maxFramerate = mSendingFramerate;
>+            
>+            if ((err = mPtrViECodec->SetSendCodec(mChannel, vie_codec)) != 0)
>+            {
>+                CSFLogError(logTag, "%s: SetSendCodec(%u) failed, err %d",
>+                            __FUNCTION__, mSendingFramerate, err);
>+                return false;
>+            }
>+            CSFLogDebug(logTag, "%s: Encoder framerate changed to %u",
>+                        __FUNCTION__, mSendingFramerate);
>+        }
>+    }
>+    return true;
>+}
> MediaConduitErrorCode
> WebrtcVideoConduit::SetExternalSendCodec(VideoCodecConfig* config,
>                                          VideoEncoder* encoder) {
>   if (!mPtrExtCodec->RegisterExternalSendCodec(mChannel,
>                                               config->mType,
>                                               static_cast<WebrtcVideoEncoder*>(encoder),
>                                               false)) {
>     mExternalSendCodecHandle = encoder;
>@@ -1094,16 +1139,20 @@ WebrtcVideoConduit::SendVideoFrame(unsig
>     return kMediaConduitSessionNotInited;
>   }
> 
>   if (!SelectSendResolution(width, height))
>   {
>     return kMediaConduitCaptureError;
>   }
> 
>+  if (!SelectSendFrameRate(mSendingFramerate))
>+  {
>+    return kMediaConduitCaptureError;
>+  }
>   //insert the frame to video engine in I420 format only
>   MOZ_ASSERT(mPtrExtCapture);
>   if(mPtrExtCapture->IncomingFrame(video_frame,
>                                    video_frame_length,
>                                    width, height,
>                                    type,
>                                    (unsigned long long)capture_time) == -1)
>   {
>diff --git a/media/webrtc/signaling/src/media-conduit/VideoConduit.h b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
>--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h
>+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
>@@ -124,17 +124,22 @@ public:
> 
>   /**
>    * Function to select and change the encoding resolution based on incoming frame size
>    * and current available bandwidth.
>    * @param width, height: dimensions of the frame
>    */
>   bool SelectSendResolution(unsigned short width,
>                             unsigned short height);
>-
>+  /**
>+   * Function to select and change the encoding frame rate based on incoming frame rate
>+   * and max-mbps setting.
>+   * @param framerate
>+   */
>+  bool SelectSendFrameRate(unsigned int framerate);
>   /**
>    * Function to deliver a capture video frame for encoding and transport
>    * @param video_frame: pointer to captured video-frame.
>    * @param video_frame_length: size of the frame
>    * @param width, height: dimensions of the frame
>    * @param video_type: Type of the video frame - I420, RAW
>    * @param captured_time: timestamp when the frame was captured.
>    *                       if 0 timestamp is automatcally generated by the engine.
>@@ -320,16 +325,17 @@ private:
>   int mChannel; // Video Channel for this conduit
>   int mCapId;   // Capturer for this conduit
>   RecvCodecList    mRecvCodecList;
>   VideoCodecConfig* mCurSendCodecConfig;
>   unsigned short mSendingWidth;
>   unsigned short mSendingHeight;
>   unsigned short mReceivingWidth;
>   unsigned short mReceivingHeight;
>+  unsigned int   mSendingFramerate;
>   bool mVideoLatencyTestEnable;
>   uint64_t mVideoLatencyAvg;
>   uint32_t mMinBitrate;
>   uint32_t mStartBitrate;
>   uint32_t mMaxBitrate;
> 
>   static const unsigned int sAlphaNum = 7;
>   static const unsigned int sAlphaDen = 8;

Comment 3

[karina]

Comment on attachment 8561932 [details]
max-mbps

># HG changeset patch
># Parent  3436787a82d0e1f7c4b43d8ec7b9b0be2f00bd04
>Bug 1127642
>
>diff --git a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
>--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
>+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
>@@ -71,16 +71,17 @@ WebrtcVideoConduit::WebrtcVideoConduit()
>   mEngineReceiving(false),
>   mChannel(-1),
>   mCapId(-1),
>   mCurSendCodecConfig(nullptr),
>   mSendingWidth(0),
>   mSendingHeight(0),
>   mReceivingWidth(640),
>   mReceivingHeight(480),
>+  mSendingFramerate(DEFAULT_VIDEO_MAX_FRAMERATE),
>   mVideoLatencyTestEnable(false),
>   mVideoLatencyAvg(0),
>   mMinBitrate(200),
>   mStartBitrate(300),
>   mMaxBitrate(2000)
> {
> }
> 
>@@ -669,17 +670,17 @@ WebrtcVideoConduit::ConfigureSendMediaCo
>   }
> 
>   if (!mVideoCodecStat) {
>     mVideoCodecStat = new VideoCodecStatistics(mChannel, mPtrViECodec, true);
>   }
> 
>   mSendingWidth = 0;
>   mSendingHeight = 0;
>-
>+  mSendingFramerate = video_codec.maxFramerate;
>   if(codecConfig->RtcpFbNackIsSet("")) {
>     CSFLogDebug(logTag, "Enabling NACK (send) for video stream\n");
>     if (mPtrRTP->SetNACKStatus(mChannel, true) != 0)
>     {
>       CSFLogError(logTag,  "%s NACKStatus Failed %d ", __FUNCTION__,
>                   mPtrViEBase->LastError());
>       return kMediaConduitNACKStatusError;
>     }
>@@ -1027,16 +1028,60 @@ WebrtcVideoConduit::SelectSendResolution
>       }
>       CSFLogDebug(logTag, "%s: Encoder resolution changed to %ux%u",
>                   __FUNCTION__, width, height);
>     } // else no change; mSendingWidth likely was 0
>   }
>   return true;
> }
> 
>+bool WebrtcVideoConduit::SelectSendFrameRate(unsigned int framerate){
>+    // Limit frame rate based on max-mbps
>+    mSendingFramerate = framerate;
>+    if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxMBPS){
>+        unsigned int cur_fs, mb_width, mb_height, max_fps;
>+        
>+        mb_width = (mSendingWidth + 15) >> 4;
>+        mb_height = (mSendingHeight + 15) >> 4;
>+        
>+        cur_fs = mb_width * mb_height;
>+        max_fps = mCurSendCodecConfig->mMaxMBPS/cur_fs;
>+        if(max_fps < mSendingFramerate)
>+            mSendingFramerate = max_fps;
>+        if(mCurSendCodecConfig->mMaxFrameRate < mSendingFramerate)
>+            mSendingFramerate = mCurSendCodecConfig->mMaxFrameRate;
>+    }
>+    if (mSendingFramerate != framerate)
>+    {
>+        // Get current vie codec.
>+        webrtc::VideoCodec vie_codec;
>+        int32_t err;
>+        
>+        if ((err = mPtrViECodec->GetSendCodec(mChannel, vie_codec)) != 0)
>+        {
>+            CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
>+            return false;
>+        }
>+        if (vie_codec.maxFramerate != mSendingFramerate)
>+        {
>+            vie_codec.maxFramerate = mSendingFramerate;
>+            
>+            if ((err = mPtrViECodec->SetSendCodec(mChannel, vie_codec)) != 0)
>+            {
>+                CSFLogError(logTag, "%s: SetSendCodec(%u) failed, err %d",
>+                            __FUNCTION__, mSendingFramerate, err);
>+                return false;
>+            }
>+            CSFLogDebug(logTag, "%s: Encoder framerate changed to %u",
>+                        __FUNCTION__, mSendingFramerate);
>+        }
>+    }
>+    return true;
>+}
> MediaConduitErrorCode
> WebrtcVideoConduit::SetExternalSendCodec(VideoCodecConfig* config,
>                                          VideoEncoder* encoder) {
>   if (!mPtrExtCodec->RegisterExternalSendCodec(mChannel,
>                                               config->mType,
>                                               static_cast<WebrtcVideoEncoder*>(encoder),
>                                               false)) {
>     mExternalSendCodecHandle = encoder;
>@@ -1094,16 +1139,20 @@ WebrtcVideoConduit::SendVideoFrame(unsig
>     return kMediaConduitSessionNotInited;
>   }
> 
>   if (!SelectSendResolution(width, height))
>   {
>     return kMediaConduitCaptureError;
>   }
> 
>+  if (!SelectSendFrameRate(mSendingFramerate))
>+  {
>+    return kMediaConduitCaptureError;
>+  }
>   //insert the frame to video engine in I420 format only
>   MOZ_ASSERT(mPtrExtCapture);
>   if(mPtrExtCapture->IncomingFrame(video_frame,
>                                    video_frame_length,
>                                    width, height,
>                                    type,
>                                    (unsigned long long)capture_time) == -1)
>   {
>diff --git a/media/webrtc/signaling/src/media-conduit/VideoConduit.h b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
>--- a/media/webrtc/signaling/src/media-conduit/VideoConduit.h
>+++ b/media/webrtc/signaling/src/media-conduit/VideoConduit.h
>@@ -124,17 +124,22 @@ public:
> 
>   /**
>    * Function to select and change the encoding resolution based on incoming frame size
>    * and current available bandwidth.
>    * @param width, height: dimensions of the frame
>    */
>   bool SelectSendResolution(unsigned short width,
>                             unsigned short height);
>-
>+  /**
>+   * Function to select and change the encoding frame rate based on incoming frame rate
>+   * and max-mbps setting.
>+   * @param framerate
>+   */
>+  bool SelectSendFrameRate(unsigned int framerate);
>   /**
>    * Function to deliver a capture video frame for encoding and transport
>    * @param video_frame: pointer to captured video-frame.
>    * @param video_frame_length: size of the frame
>    * @param width, height: dimensions of the frame
>    * @param video_type: Type of the video frame - I420, RAW
>    * @param captured_time: timestamp when the frame was captured.
>    *                       if 0 timestamp is automatcally generated by the engine.
>@@ -320,16 +325,17 @@ private:
>   int mChannel; // Video Channel for this conduit
>   int mCapId;   // Capturer for this conduit
>   RecvCodecList    mRecvCodecList;
>   VideoCodecConfig* mCurSendCodecConfig;
>   unsigned short mSendingWidth;
>   unsigned short mSendingHeight;
>   unsigned short mReceivingWidth;
>   unsigned short mReceivingHeight;
>+  unsigned int   mSendingFramerate;
>   bool mVideoLatencyTestEnable;
>   uint64_t mVideoLatencyAvg;
>   uint32_t mMinBitrate;
>   uint32_t mStartBitrate;
>   uint32_t mMaxBitrate;
> 
>   static const unsigned int sAlphaNum = 7;
>   static const unsigned int sAlphaDen = 8;

Comment 4

[Ethan Hugg [:ehugg]]

It appears that you did not use mercurial queues and bzexport as we had discussed, otherwise your attachment would have been marked as a patch and have your name in the header.   I assume with the comments you were trying to update the attachment to take out the stray printf statement and perhaps other updates.

Attachments cannot be edited so what you want to do is upload a new one, use bzexport or mark it yourself as a patch type.  I will alter this one to mark it as obselete so it will no longer show in the attachment list.

Comment 5

[karina]

when I use "hg bzexport",there is some errors. I had updated the extension.

** Unknown exception encountered with possibly-broken third-party extension qimportbz
** which supports versions unknown of Mercurial.
** Please disable qimportbz and try your action again.
** If that fixes the bug please report it to https://bugzilla.mozilla.org/enter_bug.cgi?product=Developer%20Services&component=Mercurial%3A%20qimportbz
** Python 2.7.6 (default, Sep  9 2014, 15:04:36) [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.39)]
** Mercurial Distributed SCM (version 3.2.4+20150107)
** Extensions loaded: progress, color, rebase, histedit, strip, mq, reviewboard, bzpost, firefoxtree, qimportbz, bzexport, relink
Traceback (most recent call last):
  File "/usr/local/bin/hg", line 43, in <module>
    mercurial.dispatch.run()
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 28, in run
    sys.exit((dispatch(request(sys.argv[1:])) or 0) & 255)
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 71, in dispatch
    ret = _runcatch(req)
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 140, in _runcatch
    return _dispatch(req)
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 850, in _dispatch
    cmdpats, cmdoptions)
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 611, in runcommand
    ret = _runcommand(ui, options, cmd, d)
  File "/Library/Python/2.7/site-packages/mercurial/extensions.py", line 196, in wrap
    return wrapper(origfn, *args, **kwargs)
  File "/Library/Python/2.7/site-packages/hgext/color.py", line 485, in colorcmd
    return orig(ui_, opts, cmd, cmdfunc)
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 941, in _runcommand
    return checkargs()
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 912, in checkargs
    return cmdfunc()
  File "/Library/Python/2.7/site-packages/mercurial/dispatch.py", line 847, in <lambda>
    d = lambda: util.checksignature(func)(ui, *args, **cmdoptions)
  File "/Library/Python/2.7/site-packages/mercurial/util.py", line 677, in check
    return func(*args, **kwargs)
  File "/Library/Python/2.7/site-packages/mercurial/extensions.py", line 151, in wrap
    util.checksignature(origfn), *args, **kwargs)
  File "/Library/Python/2.7/site-packages/mercurial/util.py", line 677, in check
    return func(*args, **kwargs)
  File "/Library/Python/2.7/site-packages/hgext/mq.py", line 3462, in mqcommand
    return orig(ui, repo, *args, **kwargs)
  File "/Library/Python/2.7/site-packages/mercurial/util.py", line 677, in check

Comment 6

[Ethan Hugg [:ehugg]]

OK, you can get the same result then by doing 'hg export qtip > patch.txt' and uploading patch.txt as an attachment and selecting the checkbox for 'patch'.  Assuming the patch you want to upload is on the top of your mq.

Assuming you have the path to bzexport in your hgrc and the top thing on your patch queue has a comment that starts with "Bug 1127642" I'm not sure why bzexport isn't working for you.  It does work on OSX normally.

Comment 7

[karina]

Attachment: add_select_framerate.txt

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 8562497 [details] [diff] [review]
add_select_framerate.txt

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1037,5 @@
> +    // Limit frame rate based on max-mbps
> +    mSendingFramerate = framerate;
> +    if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxMBPS){
> +        unsigned int cur_fs, mb_width, mb_height, max_fps;
> +        

Should remove this trailing whitespace.

@@ +1049,5 @@
> +        if(mCurSendCodecConfig->mMaxFrameRate < mSendingFramerate)
> +            mSendingFramerate = mCurSendCodecConfig->mMaxFrameRate;
> +    }
> +    if (mSendingFramerate != framerate)
> +    {

Should be consistent and place this brace on the line above similar to the block above this.

Comment 9

[Ethan Hugg [:ehugg]]

I think this will work so I'll ask for review.  I built it and tried it out and it didn't break anything but I don't have a test for it.  How are you testing this?

Comment 10

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

Comment on attachment 8562497 [details] [diff] [review]
add_select_framerate.txt

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

r+ with nits

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +1043,5 @@
> +        mb_height = (mSendingHeight + 15) >> 4;
> +        
> +        cur_fs = mb_width * mb_height;
> +        max_fps = mCurSendCodecConfig->mMaxMBPS/cur_fs;
> +        if(max_fps < mSendingFramerate)

space after if (I realize it's inconsistent within the file)

@@ +1044,5 @@
> +        
> +        cur_fs = mb_width * mb_height;
> +        max_fps = mCurSendCodecConfig->mMaxMBPS/cur_fs;
> +        if(max_fps < mSendingFramerate)
> +            mSendingFramerate = max_fps;

All if's should be braced per codestyle guidelines

@@ +1142,5 @@
>    {
>      return kMediaConduitCaptureError;
>    }
>  
> +  if (!SelectSendFrameRate(mSendingFramerate))

It would be better to combine SelectSendResolution with SelectSendFrameRate, and once all modifications to vie_codec are complete, apply them once.  This is, however, a nice-to-have; it should work as-is.  Please add a comment here referencing a followup bug, and file the followup bug to do so.

Comment 11

[karina]

yes, it can work on http://mozilla.github.io/webrtc-landing/pc_test.html. (In reply to Ethan Hugg [:ehugg] from comment #9)
> I think this will work so I'll ask for review.  I built it and tried it out
> and it didn't break anything but I don't have a test for it.  How are you
> testing this?

Comment 12

[karina]

which tool can be used to make coding style correct? (In reply to Ethan Hugg [:ehugg] from comment #8)
> Comment on attachment 8562497 [details] [diff] [review]
> add_select_framerate.txt
> 
> Review of attachment 8562497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +1037,5 @@
> > +    // Limit frame rate based on max-mbps
> > +    mSendingFramerate = framerate;
> > +    if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxMBPS){
> > +        unsigned int cur_fs, mb_width, mb_height, max_fps;
> > +        
> 
> Should remove this trailing whitespace.
> 
> @@ +1049,5 @@
> > +        if(mCurSendCodecConfig->mMaxFrameRate < mSendingFramerate)
> > +            mSendingFramerate = mCurSendCodecConfig->mMaxFrameRate;
> > +    }
> > +    if (mSendingFramerate != framerate)
> > +    {
> 
> Should be consistent and place this brace on the line above similar to the
> block above this.

Comment 13

[karina]

Attachment: update_select_framerate.txt

Comment 14

[Ethan Hugg [:ehugg]]

(In reply to karina from comment #12)
> which tool can be used to make coding style correct? (In reply to Ethan Hugg

You can't use a tool to fix coding style on Mozilla code.  The Mozilla coding style is described here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
However some areas of the code like webrtc/trunk are upstreamed from other sources that use a different coding style so the best rule is to follow what's already there and try to be consistent.

Comment 15

[Ethan Hugg [:ehugg]]

Comment on attachment 8563196 [details] [diff] [review]
update_select_framerate.txt

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

Bringing forward r+ from Jesup.

Comment 16

[Ethan Hugg [:ehugg]]

M-I is closed today I will probably get this in tomorrow.

Comment 17

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/888760868216

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/888760868216