1073710 - Typos in vcmCheckAttribs

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

We're fetching the wrong params here for max_dpb and max_fs:

        if ( ccsdpAttrGetFmtpMaxCpb(sdp_p, level, 0, fmtp_inst, &t_uint) == SDP_SUCCESS )
        {
            rcap->max_cpb = t_uint;
        }

        if ( ccsdpAttrGetFmtpMaxCpb(sdp_p, level, 0, fmtp_inst, &t_uint) == SDP_SUCCESS )
        {
            rcap->max_dpb = t_uint;
        }

        if ( ccsdpAttrGetFmtpMaxCpb(sdp_p, level, 0, fmtp_inst, &t_uint) == SDP_SUCCESS )
        {
            rcap->max_br = t_uint;
        }

Comment 1

[Byron Campen [:bwc]]

Attachment: Fix typos in vcmCheckAttribs that caused the max-dpb and max-fr to be set equal to the max-cpb

Should be simple enough.

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8496306 [details] [diff] [review]
Fix typos in vcmCheckAttribs that caused the max-dpb and max-fr to be set equal to the max-cpb

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

https://tbpl.mozilla.org/?tree=Try&rev=606272c3e3b5

Comment 3

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

(In reply to Byron Campen [:bwc] from comment #1)
> Created attachment 8496306 [details] [diff] [review]
> Fix typos in vcmCheckAttribs that caused the max-dpb and max-fr to be set
> equal to the max-cpb

Actually, it was just doing tell-me-three-times on cpb, and letting dpb and br default.

Comment 4

[Byron Campen [:bwc]]

I'm pretty sure we actually were setting all three to the same thing, since we were re-using t_uint in each case. Should we double-check with some H264 tests to be sure this doesn't have any surprising side-effects?

Comment 5

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

(In reply to Byron Campen [:bwc] from comment #4)
> I'm pretty sure we actually were setting all three to the same thing, since
> we were re-using t_uint in each case. Should we double-check with some H264
> tests to be sure this doesn't have any surprising side-effects?

never mind, you're right.

max-br would be good to check; set it and make a call and check network traffic.

Comment 6

[Byron Campen [:bwc]]

So I've tested the patch without tweaking any of these params, and I'm not seeing any deleterious effects. I'll give a try and see what happens when I set the max_br to something smallish.

Comment 7

[Byron Campen [:bwc]]

max_br does seem to limit the bandwidth used.

Comment 8

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a04cf72848f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a04cf72848f