1069646 - VCMQmResolution::UpdateCodecResolution() sets invalid frame rate

Status: RESOLVED FIXED

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

Description

[Jay]

[Blocking Requested - why for this release]:

While testing the low bit-rate mode with H.264, I am seeing that VCMQmResolution::UpdateCodecResolution() sets codec framerate to 30,000. I believe this should be 30fps instead of 30,000.

09-18 21:29:33.909  7040  7046 I WebRTC  : UpdateCodecResolution: [30000.000000] 30000.000000 fps => 20.993889 fps
09-18 21:29:44.539  7040  7046 I WebRTC  : UpdateCodecResolution: [30000.000000] 20.993889 fps => 30000.000000 fps

The problem seems to be "native_frame_rate_" was initialized with the wrong value.

The frame rate from [1] was multiplied by 1000 and it passed down to VCMQmResolution::Initialize(). Then, native_frame_rate_ assigned with framerate*1000 value.

[1]http://git.mozilla.org/?p=releases/gecko.git;a=blob;f=media/webrtc/trunk/webrtc/modules/video_coding/main/source/video_sender.cc;h=d25a38d1986d15c628346ff9df782aedec21af04;hb=HEAD#l160

Comment 1

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

Attachment: scale frame rate initialization in webrtc media_opimization

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8492749 [details] [diff] [review]
scale frame rate initialization in webrtc media_opimization

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

This patch begs the question whether this isn't the problem:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/video_sender.cc#160
but because it's an integer value, multiplying by 1000 makes sense.

Now, there's also this call on line 168:
content_->UpdateFrameRate(frame_rate);
should it not get the same change?

Looking further, it seems incoming_frame_rate_ is intended to be * 1000, see this formula:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc#647

and there are calls like:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc#125
and
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc#300
which suggests that setting the framerates * 1000 is intentional.

Now, this call is using float, so it likely does not expect the value to be * 1000:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/qm_select.cc#222

So I think you only need to change this line:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc#181
and not the others!

Note that user_frame_rate_ is a float. I suspect the code is probably consistent in that float=*1 and (u)int32's=*1000

Comment 3

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> Comment on attachment 8492749 [details] [diff] [review]
> scale frame rate initialization in webrtc media_opimization
> 
> Review of attachment 8492749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch begs the question whether this isn't the problem:
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/main/source/video_sender.cc#160
> but because it's an integer value, multiplying by 1000 makes sense.

Sure
 
> Now, there's also this call on line 168:
> content_->UpdateFrameRate(frame_rate);
> should it not get the same change?

That also takes a uint32_t.  The other call is at :605, and passes qm->frame_rate, which is an unscaled float.  So one must be wrong.  Using the initialization value as a guide, it's clear it's expected unscaled (i.e. only the first call from SetEncodingData() was wrong.  Since the source for most of it is a float anyways, I converted it to take a float.  In practice due to what it's used for (averaging), this will make little difference over a uint32_t. (so long as both are unscaled).

> Looking further, it seems incoming_frame_rate_ is intended to be * 1000, see
> this formula:
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/main/source/media_optimization.cc#647

No, the 1000 factor there is because diff is in ms.  (This adds greatly to the confusion since they weren't clear.

I think your comment later is correct: floats are always unscaled, integers may be scaled (video_sender.cc/etc) or unscaled (counts of actual frames), and SetRates()).

> 
> and there are calls like:
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/main/source/media_optimization.cc#125

Takes a float; appears unscaled.  media_opt_utils.h

> and
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/main/source/media_optimization.cc#300
> which suggests that setting the framerates * 1000 is intentional.

Takes a float; code clearly wants unscaled.

> Now, this call is using float, so it likely does not expect the value to be
> * 1000:
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/main/source/qm_select.cc#222
> 
> So I think you only need to change this line:
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_coding/main/source/media_optimization.cc#181
> and not the others!

Per above, I disagree.

> 
> Note that user_frame_rate_ is a float. I suspect the code is probably
> consistent in that float=*1 and (u)int32's=*1000

Yes.

Comment 4

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

Attachment: scale frame rate initialization in webrtc media_opimization

Comment 5

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=148862fe121e

Comment 6

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #5)
> https://tbpl.mozilla.org/?tree=Try&rev=148862fe121e

Sorry. Looks like a bug in the bzpost Mercurial extension updated the wrong bug.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/148862fe121e

Comment 8

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

Comment on attachment 8493134 [details] [diff] [review]
scale frame rate initialization in webrtc media_opimization

[Approval Request Comment]
Bug caused by (feature/regressing bug #): enabling content analysis some time ago

User impact if declined: mis-setting of initial conditions for the content analsys code.  This may cause poor choices in the short term, and also it may confuse the encoder (or make it make bad choices about bits) if it believes it's being told an FPS of 30000 instead of 30.  These problems should mostly go away the longer you're in the call, though it may take a while for some parts of it (due to running averages).

Testing completed: On m-c.  Should be verified before actual uplift occurs.

Risk to taking this patch (and alternatives if risky): very low; corrects an obvious fault that should only avoid confusing optimization/allocation-of-bits.

String or UUID changes made by this patch: none.

Comment 9

[bhavana bajaj [:bajaj]]

nils, can you please help with testing/verifying this on a nightly before we can uplift?

Comment 11

[Joshua Mitchell (Inactive)]

Comment on attachment 8493134 [details] [diff] [review]
scale frame rate initialization in webrtc media_opimization

I believe you are just asking for a verify on the patch landed to master - the verifyme tag is not for that (confusing, I know)- please use QA-Approval flag on the patch to get it verified as fixed

Comment 12

[Joshua Mitchell (Inactive)]

Jayme - when you are done with your current task please verify this patch has fixed this issue in the latest Master build.

Comment 13

[Nils Ohlmeier [:drno]]

I tried yesterday to verify this, by putting load on my desktop machine while in a H264 call, but I failed to find the log messages from the original report in my log files. Hopefully Jayme has a more reliable way how to reproduce and test this. Let know if I can help in any way.

Comment 14

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

As with Nils in comment 13 I don't have a reliable way of reproducing this either.

Jay: Can you provide some steps that we can use to verify this issue was fixed, including if possible information on what you were using to view the h.264 video in your comment 0?

Comment 15

[Jay]

(In reply to Jayme Mercado [:JMercado] from comment #14)
> As with Nils in comment 13 I don't have a reliable way of reproducing this
> either.
> 
> Jay: Can you provide some steps that we can use to verify this issue was
> fixed, including if possible information on what you were using to view the
> h.264 video in your comment 0?

I saw this issue when I ran the test with bad network condition. The test was performed with two 8x10 devices connected to two different APs, then the APs are connected to a managed switch. The switch is configured to low bandwidth 384K or 512K.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8493134 [details] [diff] [review]
scale frame rate initialization in webrtc media_opimization

I am taking it for beta 8, otherwise it is going to be too late. WebRTC is one of the feature we are going to put the focus on.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/daa7e7584b2a
https://hg.mozilla.org/releases/mozilla-beta/rev/bc5451d18901

Comment 18

[Joshua Mitchell (Inactive)]

removing qa-wanted tag - issue seems to be resolved already + we can not replicate the steps in this lab.

Comment 19

[bhavana bajaj [:bajaj]]

Comment on attachment 8493134 [details] [diff] [review]
scale frame rate initialization in webrtc media_opimization

Jay,

If you could confirm this fixes the issue, given you have a reliable way of reproducing this, It'd be helpful. Meanwhile :drno will try his best to get this verified based on your feedback in parallel with landing..

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/26b6a3897158

Comment 21

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/28d7d85bd5eb