Closed Bug 966885 Opened 6 years ago Closed 6 years ago

Enable audio level RTP extension (patch included)

Categories

(Core :: WebRTC: Signaling, defect)

29 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: ggb, Assigned: ggb)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch audio_level_ext.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.102 Safari/537.36

Steps to reproduce:

Create a PeerConnection and check the SDP and the RTP packets.


Actual results:

Audio level extension (RFC6464) is not included in local SDP and neither enabled when received in the remote SDP.

This extension is enabled by default in Chrome, RECOMMENDED to implement in draft-ietf-rtcweb-rtp-usage-11 and very useful to detect Voice Activity in server elements without requiring media decoding.


Expected results:

Extension is included in local SDP by default.    If supported by both endpoints the extension is activated and the RTP header being included in the RTP packets.

Patch included to implement this behaviour.
Attachment #8369319 - Attachment is patch: true
Attachment #8369319 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8369319 [details] [diff] [review]
audio_level_ext.patch

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

r+ from me on the AudioConduit parts.  r? to ehugg on the sipcc code.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +465,4 @@
>                                                codecConfig->mRate);
>  
>    mEngineTransmitting = true;
> +

Lets not mess with style with the blank line additions

@@ +578,5 @@
>  MediaConduitErrorCode
> +WebrtcAudioConduit::EnableAudioLevelExtension(bool enabled)
> +{
> +  CSFLogDebug(logTag,  "%s %d ", __FUNCTION__, enabled);
> +  int error = 0; //webrtc engine errors

This is unused, remove it

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4714,5 @@
> +        i++;
> +    } while (uri != NULL);
> +
> +    media->audio_level = audio_level;
> +    

trailing spaces

@@ +5690,5 @@
>            }
>  
> +          /* Add supported audio level rtp extension */
> +          if (media_cap->type == SDP_MEDIA_AUDIO) {
> +              gsmsdp_set_extmap_attribute(level, dcb_p->sdp->src_sdp, 1, 

trailing space

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +144,4 @@
>      /* Flag to indicate if Multicast */
>      boolean         is_multicast;
>      uint16_t        multicast_port;
> +

Let's not add spurious style changes
Attachment #8369319 - Flags: review?(ethanhugg)
Comment on attachment 8369319 [details] [diff] [review]
audio_level_ext.patch

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

This looks good.  r+ assuming nits will be fixed.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +579,5 @@
> +WebrtcAudioConduit::EnableAudioLevelExtension(bool enabled)
> +{
> +  CSFLogDebug(logTag,  "%s %d ", __FUNCTION__, enabled);
> +  int error = 0; //webrtc engine errors
> +  bool success = false;

I believe this is unused as well.  This shows up in the warnings from the build.  Please check that you aren't adding new warnings.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
@@ -311,4 @@
>  sdp_build_attr_sdescriptions(sdp_t *sdp_p, sdp_attr_t *attr_p,
>                               flex_string *fs);
>  
> -

This change looks unnecessary.

::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +743,5 @@
> +  ASSERT_NE(body.find("a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level\r\n"), std::string::npos);
> +}
> +
> +TEST_F(SdpTest, parseExtMap) {
> +  ParseSdp(kVideoSdp + 

See if you can set your editor to eat trailing whitespace.
Attachment #8369319 - Flags: review?(ethanhugg) → review+
Please upload the nits-fixed patch (make sure hg knows who you are via .hgrc or "hg qref -u user..."), then mark the bug for checkin by adding "checkin-needed" to the Keywords field
Attached patch audio_level_ext_nits_fixed.patch (obsolete) — Splinter Review
Attachment #8369319 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c466f72d2a
for the failure:
cppunittests TEST-UNEXPECTED-FAIL | sdp_unittests | test failed with return code 1
occurring on all platforms.

For more details see https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=707c231b203f
Status: UNCONFIRMED → NEW
Ever confirmed: true
On the plus side, I gave you canconfirm and editbugs privileges in Bugzilla.  For more details on those, see
https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla
Log shows this:

18:19:48     INFO -  [ RUN      ] SdpTest.addExtMap
18:19:48     INFO -  ../../../../../../media/webrtc/signaling/test/sdp_unittests.cpp:743: Failure
18:19:48     INFO -  Value of: std::string::npos
18:19:48     INFO -    Actual: 18446744073709551615
18:19:48     INFO -  Expected: body.find("a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level\r\n")
18:19:48     INFO -  Which is: 18446744073709551615

I can do a TRY run for you of your next patch so we can catch this before pushing to M-C.
Attachment #8371944 - Attachment is obsolete: true
Thank you guys and apologize for the mistakes, I'm still trying to learn all the mozilla tools&processes.  I though those tests where already executed when doing "mach gtest" but I see that they are not.  Find attached the new patch, now all those test are passing locally for me.  

@ehugg, Can you try?
No worries. Here's a try run on today's M-C:

https://tbpl.mozilla.org/?tree=Try&rev=e483be91826c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e7f819ed1fe
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.