694810 - Support Opus in WebRTC

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

This includes getting them into the upstream at Google.

Assigned to rillian based on my understanding he's been handling this.

Ralph, please update this with the current status and/or links.  See also bug 674225 for support for playing Opus; perhaps it should be a blocker here, but we can get it into WebRTC before it's supported in <audio>, etc.

Comment 1

[Ralph Giles (:rillian)]

Attachment: work in progress

I have been reminded that sharing code is beneficial.

This is a cleaned up version of the patch I did last year, ported webrtc trunk as of about two weeks ago. It compiles, and has untested C and C++/acm wrappers around the Opus codec.

The next major piece is to hook it up to NetEQ.

Comment 2

[Ralph Giles (:rillian)]

To those trying to apply to upstream webrtc.org code; you may need to disable -Werror to build locally:

Index: build/common.gypi
===================================================================
--- build/common.gypi	(revision 134666)
+++ build/common.gypi	(working copy)
@@ -1814,7 +1814,8 @@
         # Enable -Werror by default, but put it in a variable so it can
         # be disabled in ~/.gyp/include.gypi on the valgrind builders.
         'variables': {
-          'werror%': '-Werror',
+          #'werror%': '-Werror',
+          'werror%': '',
 	  'libraries_for_target%': '',

Comment 3

[Ralph Giles (:rillian)]

On Fedora 17, I'm having trouble with today's checkout building libvpx. Will update if I get it working.

Comment 4

[Ralph Giles (:rillian)]

Attachment: work in progress

Update patch. This is the same as the previous, just diffed against today's trunk.

Comment 5

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

Comment on attachment 629416 [details] [diff] [review]
work in progress

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

::: src/video_engine/main/test/AutoTest/source/vie_autotest_linux.cc
@@ +113,4 @@
>      XStoreName(_display, _window, title);
>      XSetIconName(_display, _window, title);
>  
> +    // make x report events for mask

All the changes to this file are spurious whitespace changes

::: src/video_engine/main/interface/vie_render.h
@@ +95,4 @@
>                                     const bool mirrorXAxis,
>                                     const bool mirrorYAxis) = 0;
>  
> +    // External render

spurious whitespace change

::: src/modules/audio_coding/codecs/opus/opus_interface.c
@@ +43,5 @@
> +    OpusEncoder *state = (OpusEncoder*)inst;
> +    opus_int16 *audio = (opus_int16 *)audioIn;
> +    unsigned char *coded = (unsigned char *)encoded;
> +
> +    int error = opus_encode(state, audio, len, coded, 1274);

what's the magic constant here? Should it be a #define (and should it live in a header)?

::: src/modules/audio_coding/main/source/acm_speex.cc
@@ +52,4 @@
>  namespace webrtc {
>  
>  #ifndef WEBRTC_CODEC_SPEEX
> +ACMSPEEX::ACMSPEEX(WebRtc_Word16 /* codecID */)

spurious whitespace change (it appears)

::: src/voice_engine/main/interface/voe_external_media.h
@@ +47,4 @@
>      // given by the |type| parameter. The function should modify the
>      // original data and ensure that it is copied back to the |audio10ms|
>      // array. The number of samples in the frame cannot be changed.
> +    // The sampling frequency will depend upon the codec used.

spurious whitespace change

::: AUTHORS
@@ +3,4 @@
>  
>  Google Inc.
>  Mozilla Foundation
> +Ben Strong <[e-mail]>

spurious whitespace change

::: OWNERS
@@ +2,4 @@
>  [e-mail]
>  [e-mail]
>  [e-mail]
> [e-mail]

spurious whitespace change

Comment 6

[Timothy B. Terriberry (:derf)]

Comment on attachment 629416 [details] [diff] [review]
work in progress

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

This patch is very preliminary, and there's a lot about it that's completely wrong. I don't think it's worth reviewing yet.

::: src/modules/audio_coding/codecs/opus/opus_interface.c
@@ +43,5 @@
> +    OpusEncoder *state = (OpusEncoder*)inst;
> +    opus_int16 *audio = (opus_int16 *)audioIn;
> +    unsigned char *coded = (unsigned char *)encoded;
> +
> +    int error = opus_encode(state, audio, len, coded, 1274);

This looks like an attempt to be the maximum packet size Opus supports, but it's wrong. The maximum payload of a single frame is 1275 bytes, plus 1 byte for the TOC header. For 60 ms CELT packets you can have 3 frames, so 3*1275+2*2+2 or 3831 bytes total. I can't actually tell if we'll ever ask for 60 ms packets... currently we appear to specify a default packet size of 0 samples (it's simply not initialized below where Opus is added to the codec database; this was one of Tina's review comments). Whatever it is, the value needs to be smaller than WebRTC's MAX_PAYLOAD_SIZE_BYTE.

Comment 7

[Ralph Giles (:rillian)]

Attachment: work in progress

Minor cleanup; removed some of the whitespace changes, updated to today's trunk, confirmed it still builds.

Comment 8

[Ralph Giles (:rillian)]

Attachment: work in progress

Another small update:

- Diff against r2382 (today)
- Build opus is VAR_ARRAYS instead of USE_ALLOCA because gold can't find alloca
- Add repacketizer.c and ACMOpus::CreateInstance for missing symbols

Working on getting audio_coding_unittests to pass next; not all the codec registration stuff is hooked up correctly.

Comment 9

[Ralph Giles (:rillian)]

Attachment: work in progress

Another minor update.

Fill in some missing acm_codec_database entries. audio_coding_unittests works now.

Comment 10

[Ralph Giles (:rillian)]

Attachment: work in progress

Rebased against today's trunk, r2406.

Comment 11

[Jean-Marc Valin]

Attachment: Updated patch that passes audio_coding_module_test

Simple fix on top of Ralph's patch. Fixes the ordering of the codec enum which is what prevented audio_coding_module_test from passing.

Comment 12

[Ralph Giles (:rillian)]

Attachment: work in progress

Updated patch, incorporating jm's ordering changes. Passed both audio_coding_unittests and audio_coding_module_test.

Comment 13

[Cyril Lashkevich]

Attachment: work in progress

Updated patch. Passed both audio_coding_unittests and audio_coding_module_test, and tested on custom application. Unittests are still missed.

Comment 14

[Ralph Giles (:rillian)]

(In reply to Cyril Lashkevich from comment #13)

> Updated patch. Passed both audio_coding_unittests and
> audio_coding_module_test, and tested on custom application. Unittests are
> still missed.

Awesome, thanks! Can we see the custom application? Does round-trip audio work yet?

Comment 15

[Cyril Lashkevich]

Attachment: work in progress

Updated patch. Added missed in previous patch files, fixed crash in unittest on 20ms frame. I'm going to provide patch for running peerconnection sample app on opus soon.

Comment 16

[Cyril Lashkevich]

Attachment: work in progress

PLC processing added, bitrate set on EncoderCreate.

Comment 17

[Ralph Giles (:rillian)]

Attachment: gclient diff against peerconnection

Thanks for the update, Cyril.

Derf and I worked on the patch some this week as well. I'm attaching a 'gclient diff' against the peerconnection tree, so the opus checkout is in a different place. 'trunk/third_party' isn't under version control with peerconnection like it is with webrtc dev, so it was easier to just stick it in the top level for now so we have somewhere to keep the gypi.

This version maps between libopus' native 48 kHz on encode/decode and in the RTP payload, and 32 kHz in the audio codec wrappers, resampling the audio in the ACM wrapper and converting the advertised rate in the voice_engine channel code.

An new MOZILLA_BUILD define is added so we can default to opus for our uses.

In our testing, connecting between a linux machine and linux vm through a peerconnection_server running on the Mac hosting the vm, this patch allowed negotiation of opus as the audio codec and successful configuration with the sdp, but then dies with an assert trying to process STUN ping packets between the peerconnection_client instances.

Some progress then. I'd like to try it with bridged networking for the vm instead of having it behind NAT, since the stun ping crash didn't make much sense. It looked it it was passing the udp packets to an tcp hander, which reasonably rejected them.

Comment 18

[Ralph Giles (:rillian)]

Not better with the bridged network. Found some different asserts under valgrind, but that's all.

Comment 19

[Ralph Giles (:rillian)]

Attachment: patch against alder

More work in progress, this time against the alder project tree of firefox.

With this and the patch from bug 781281, media/webrtc/tests/mediapipeline_unittests passes with 'opus' as the selected codec. For whatever that is worth.

Comment 20

[Eric Rescorla (:ekr)]

Attachment: A patch to test opus with mediaconduit_unittest

This patch does two things:

(1) it replaces G.711 with Opus. As you can see, this doesn't produce viable output.
(2) It dumps the raw packets in a form that is suitable for use with opus_demo, demonstrating that the output bitstream is OK.

Comment 21

[Ralph Giles (:rillian)]

Attachment: patch against alder

Update my alder patch. Playback still failing, further in. We need to find an earlier point to do the sample frequency translation.

Comment 22

[Ralph Giles (:rillian)]

Attachment: patch against alder

Updated patch. Make sure we give enough output buffer space to opus_decode. We not get garbled, but recognizable output from mediaconduit_unittests' recorded.pcm. Thanks to Jean-Marc for debugging assistance.

Comment 23

[Ralph Giles (:rillian)]

Attachment: patch against alder

Updated patch against alder. No advances in decoding, but it no longer requires a duplicate copy of opus in media/webrtc/trunk/third_party/opus/source. Instead it refers to and links with the gkmedias version when build_with_mozilla==1.

Tim, you may find my pcap-based packet dumper at https://git.xiph.org/?p=users/giles/opus-tools.git;a=blob;f=src/opusrtp.c;hb=rtp I don't think it's working properly (opusinfo complains about the output) but it may be useful for external diagnostics while webrtc_standalone_test runs.

Comment 24

[Timothy B. Terriberry (:derf)]

Attachment: additional patch against alder

This applies on top of rillian's "patch against alder".

It fixes timestamp processing to actually run at 48 kHz, like the Opus RTP spec says, converts the RTP/RTCP receiver codec database to use the internal ACM representation instead of the "external" representation (without this, the output gets resampled to the wrong rate), and moves the resampling to C, since NetEQ calls the codec's C functions directly, bypassing the C++ interface. It also bumps up a number of internal NetEQ buffers, which were too small to handle Opus's maximum packet size (no guarantee I got them all, but I fixed what I could find).

Comment 25

[Ralph Giles (:rillian)]

Attachment: patch against alder

Updated patch against alder. This rolls in Tim's fixes to get decoding working, but replacing his struct hack with an explicit codec context pointer, which is easier to read.

We'd like to land this on alder, with or without the extra debug output in the unit tests.

Comment 26

[Ralph Giles (:rillian)]

Attachment: patch against webrtc.org

Rough port of the latest alder patch to upstream webrtc.org code r2645.

Work in progress checkpoint; doesn't build.

Comment 27

[Ralph Giles (:rillian)]

Attachment: patch against webrtc.org

Update webrtc.org patch to include the WebRtcOpus_* wrapper files.

Comment 28

[Ralph Giles (:rillian)]

Attachment: patch against webrtc.org

Updated patch against webrtc svn r2650. This one compiles.

Comment 29

[Ralph Giles (:rillian)]

Attachment: patch against webrtc.org

Updated patch against webrtc.org r2646, to address some points derf brought up:

Pass the resampler buffer length to opus_decode() rather than NETEQ_MAX_FRAME_SIZE, which reduces our dependence on NETEQ_48KHZ_WIDEBAND for having enough space.

Don't disable AVT aka DMTF in the mozilla build.

Comment 30

[Ralph Giles (:rillian)]

Jesup, ping on this. Re tonight's irc discussion, derf, ekr and I are in favour of landing the patch against Alder as it is. Waiting for review is holding up further work towards a working opus call.

Reasons to preceed even if you don't have time for a proper review:

- The patch is rough, but it works. Getting it in alder will speed up the cleanup.
- The patch only affects the third-party webrtc code, so it's "external" code from mozilla's point of view.
- It will be reviewed by google when it lands upstream. I will continue to update our code as that version of the patch evolves.

Comment 31

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

Comment on attachment 653519 [details] [diff] [review]
patch against alder

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

r- for uninitialized value and to answer some of the questions/issues raised.  Should be easy r+ tomorrow.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +47,5 @@
>    TestAgent() :
>        audio_flow_(new TransportFlow()),
>        audio_prsock_(new TransportLayerPrsock()),
>        audio_dtls_(new TransportLayerDtls()),
> +      audio_config_(109, "opus", 48000, 480, 1, 64000),

Is there a reason for removing G711 here?

::: media/webrtc/trunk/DEPS
@@ +28,5 @@
>      (Var("googlecode_url") % "webrtc") + "/trunk/third_party/libvpx@" + Var("webrtc_revision"),
>  
> +  "trunk/third_party/opus/source":
> +    "http://git.opus-codec.org/opus.git",
> +

Every other DEPS entry specifies a specific version or set by a var; this probably should too

::: media/webrtc/trunk/src/engine_configurations.h
@@ +33,1 @@
>  #define WEBRTC_CODEC_ISAC       // floating-point iSAC implementation (default)

Is there a reason we can't define ISAC and Opus?  Seems odd..

::: media/webrtc/trunk/src/modules/audio_coding/codecs/opus/opus.gypi
@@ +21,5 @@
> +        'interface',
> +      ],
> +      'conditions': [
> +        ['build_with_mozilla==1', {
> +          # Mozilla provides its own build of the opus library.

This would probably be better termed "internal_opus==0" or "internal_codec_opus==0"; and then set it with the other main build-config settings.

::: media/webrtc/trunk/src/modules/audio_coding/codecs/opus/opus_interface.c
@@ +78,5 @@
> +                                WebRtc_Word16 encodedLenByte,
> +                                WebRtc_Word16 len)
> +{
> +    WebRtc_Word16 buffer16[48*WEBRTC_OPUS_MAX_ENCODE_FRAME_SIZE_MS];
> +    WebRtc_Word32 buffer32[64*WEBRTC_OPUS_MAX_ENCODE_FRAME_SIZE_MS+7];

Where do these constants come from?  Why are they what they are?  If these are real constants, they should be in a codec include?

@@ +175,5 @@
> +{
> +    /* Enough for 120 ms (the largest Opus packet size) of mono audio at 48 kHz.
> +       This will need to be enlarged for stereo decoding. */
> +    WebRtc_Word16 buffer16[48*120];
> +    WebRtc_Word32 buffer32[48*120+7];

arbitrary constants?

::: media/webrtc/trunk/src/modules/audio_coding/main/source/acm_codec_database.cc
@@ +225,5 @@
>    {3, {320, 640, 960}, 0, 1},
>  #endif
> +#ifdef WEBRTC_CODEC_OPUS
> +  // See opus_encoder.c opus_encode(...)
> +  // ?? There is no sence to use frames less than 10ms

sense
And so is there an actionable item/issue here?

@@ +1009,5 @@
>  }
>  
> +// Checks if the bitrate is valid for Opus.
> +bool ACMCodecDB::IsOpusRateValid(int rate) {
> +  if ((rate >= 6000) && (rate <= 510000)) {

Are these part of the spec, or #defined anywhere?  Or are they reasonable but arbitrary?

::: media/webrtc/trunk/src/modules/audio_coding/main/source/acm_opus.cc
@@ +353,5 @@
>  WebRtc_Word16
>  ACMOPUS::SetBitRateSafe(
>      const WebRtc_Word32 rate)
>  {
> +    if (rate < 500 || rate > 510000) {

The lower bound here doesn't match acm_codec_database.cc

::: media/webrtc/trunk/src/modules/audio_coding/neteq/recout.c
@@ +115,5 @@
>          + SCRATCH_ALGORITHM_BUFFER;
>      DSP2MCU_info_t *dspInfo = (DSP2MCU_info_t*) (pw16_scratchPtr + SCRATCH_DSP_INFO);
>  #else
> +    WebRtc_Word16 pw16_decoded_buffer[NETEQ_MAX_FRAME_SIZE+240*6];
> +    WebRtc_Word16 pw16_NetEqAlgorithm_buffer[NETEQ_MAX_OUTPUT_SIZE+240*6];

What's 240*6 and why is it here?  At least please comment it; perhaps promote it to a #define

::: media/webrtc/trunk/src/supplement.gypi
@@ +3,5 @@
>  # Please see the WebRTC DEPS file for details.
>  {
>    'variables': {
>      'build_with_chromium': 0,
> +    'build_with_mozilla': 1,

This is set in configure.in when it calls gyp; I don't see why it should be here.  Is this actually needed?

::: media/webrtc/trunk/src/voice_engine/main/source/channel.cc
@@ +2469,5 @@
>      WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId),
>                   "Channel::GetRecPayloadType()");
>      WebRtc_Word8 payloadType(-1);
> +    CodecInst acmCodec;
> +    if (_rtpRtcpModule.ReceivePayloadType(acmCodec, &payloadType) != 0)

acmCodec is uninitialized here

::: media/webrtc/trunk/src/voice_engine/main/source/voe_codec_impl.cc
@@ +688,5 @@
> +    {
> +        toInst.plfreq = 48000;
> +        // TODO: This should not be a fixed value.
> +        // Opus does not use fixed frame sizes.
> +        toInst.pacsize = 960;

I assume these are "large enough to be safe" values

@@ +734,5 @@
> +    {
> +        toInst.plfreq = 32000;
> +        // TODO: This should not be a fixed value.
> +        // Opus does not use fixed frame sizes.
> +        toInst.pacsize = 640;

ditto

Comment 32

[Ralph Giles (:rillian)]

Comment on attachment 653519 [details] [diff] [review]
patch against alder

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

Thanks for the review. I've responded inline. I'll try backporting the patch against webrtc.org, which also has a bunch changes to match the google style guide upstream prefers.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +47,5 @@
>    TestAgent() :
>        audio_flow_(new TransportFlow()),
>        audio_prsock_(new TransportLayerPrsock()),
>        audio_dtls_(new TransportLayerDtls()),
> +      audio_config_(109, "opus", 48000, 480, 1, 64000),

So we can test opus? I suppose we should test both, but I thought it was an either/or thing here.

::: media/webrtc/trunk/DEPS
@@ +28,5 @@
>      (Var("googlecode_url") % "webrtc") + "/trunk/third_party/libvpx@" + Var("webrtc_revision"),
>  
> +  "trunk/third_party/opus/source":
> +    "http://git.opus-codec.org/opus.git",
> +

No, right now master is the best option. In any case this code is just for upstream, we use the copy from media/libopus. They can pick a specific revision later if they want.

::: media/webrtc/trunk/src/engine_configurations.h
@@ +33,1 @@
>  #define WEBRTC_CODEC_ISAC       // floating-point iSAC implementation (default)

We're disabling iSAC here because we don't intend to support it, per bug 784082.

::: media/webrtc/trunk/src/modules/audio_coding/codecs/opus/opus.gypi
@@ +21,5 @@
> +        'interface',
> +      ],
> +      'conditions': [
> +        ['build_with_mozilla==1', {
> +          # Mozilla provides its own build of the opus library.

The include path is mozilla-specific.

::: media/webrtc/trunk/src/modules/audio_coding/codecs/opus/opus_interface.c
@@ +78,5 @@
> +                                WebRtc_Word16 encodedLenByte,
> +                                WebRtc_Word16 len)
> +{
> +    WebRtc_Word16 buffer16[48*WEBRTC_OPUS_MAX_ENCODE_FRAME_SIZE_MS];
> +    WebRtc_Word32 buffer32[64*WEBRTC_OPUS_MAX_ENCODE_FRAME_SIZE_MS+7];

They're supposed to be the maximum size of pcm data at 32 and 48 kHz, respectively, plus lapping for the resampler.

@@ +175,5 @@
> +{
> +    /* Enough for 120 ms (the largest Opus packet size) of mono audio at 48 kHz.
> +       This will need to be enlarged for stereo decoding. */
> +    WebRtc_Word16 buffer16[48*120];
> +    WebRtc_Word32 buffer32[48*120+7];

48000 samples per second * 120 milliseconds / (1000 ms per second). The +7 is resample lapping again.

I'll make this buffer16[WEBRTC_OPUS_MAX_FRAME_SIZE].

::: media/webrtc/trunk/src/modules/audio_coding/main/source/acm_codec_database.cc
@@ +225,5 @@
>    {3, {320, 640, 960}, 0, 1},
>  #endif
> +#ifdef WEBRTC_CODEC_OPUS
> +  // See opus_encoder.c opus_encode(...)
> +  // ?? There is no sence to use frames less than 10ms

Tina says we should just 20 ms frames here.

@@ +1009,5 @@
>  }
>  
> +// Checks if the bitrate is valid for Opus.
> +bool ACMCodecDB::IsOpusRateValid(int rate) {
> +  if ((rate >= 6000) && (rate <= 510000)) {

These numbers are from spec, the start of section 2.

::: media/webrtc/trunk/src/modules/audio_coding/main/source/acm_opus.cc
@@ +353,5 @@
>  WebRtc_Word16
>  ACMOPUS::SetBitRateSafe(
>      const WebRtc_Word32 rate)
>  {
> +    if (rate < 500 || rate > 510000) {

You're right. Fixed.

::: media/webrtc/trunk/src/modules/audio_coding/neteq/neteq_defines.h
@@ +346,5 @@
> +/* Define this unconditionally, since the defines above
> + * don't seem to be active and we need the larger frame
> + * size for opus_decode.
> + */
> +#define NETEQ_48KHZ_WIDEBAND

I think this isn't necessary any more with derf's _SIZE doubling below.

::: media/webrtc/trunk/src/modules/audio_coding/neteq/recout.c
@@ +115,5 @@
>          + SCRATCH_ALGORITHM_BUFFER;
>      DSP2MCU_info_t *dspInfo = (DSP2MCU_info_t*) (pw16_scratchPtr + SCRATCH_DSP_INFO);
>  #else
> +    WebRtc_Word16 pw16_decoded_buffer[NETEQ_MAX_FRAME_SIZE+240*6];
> +    WebRtc_Word16 pw16_NetEqAlgorithm_buffer[NETEQ_MAX_OUTPUT_SIZE+240*6];

I have no idea, this was derf's patch.

::: media/webrtc/trunk/src/modules/audio_conference_mixer/source/audio_conference_mixer_impl.cc
@@ +249,5 @@
>                  break;
> +            case 48000:
> +                if(OutputFrequency() != kWbInHz)
> +                {
> +                    SetOutputFrequency(kWbInHz);

These should use kFbInHz.

::: media/webrtc/trunk/src/supplement.gypi
@@ +3,5 @@
>  # Please see the WebRTC DEPS file for details.
>  {
>    'variables': {
>      'build_with_chromium': 0,
> +    'build_with_mozilla': 1,

I was doing a parallel construction to build_with_chromium. Having it here documents the flag. I see it's also set in configure; if that overrides, we can keep the upstream setting of having both zero here.

::: media/webrtc/trunk/src/voice_engine/main/source/channel.cc
@@ +2469,5 @@
>      WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId,_channelId),
>                   "Channel::GetRecPayloadType()");
>      WebRtc_Word8 payloadType(-1);
> +    CodecInst acmCodec;
> +    if (_rtpRtcpModule.ReceivePayloadType(acmCodec, &payloadType) != 0)

Good spotting. Fixed

::: media/webrtc/trunk/src/voice_engine/main/source/voe_codec_impl.cc
@@ +688,5 @@
> +    {
> +        toInst.plfreq = 48000;
> +        // TODO: This should not be a fixed value.
> +        // Opus does not use fixed frame sizes.
> +        toInst.pacsize = 960;

We hope. 960 is 20ms * 48 kHz, so it's enough for the 20ms packets we're sending now.

@@ +734,5 @@
> +    {
> +        toInst.plfreq = 32000;
> +        // TODO: This should not be a fixed value.
> +        // Opus does not use fixed frame sizes.
> +        toInst.pacsize = 640;

likewise, 20ms * 32kHz for the internal audio handling.

Comment 33

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #32)
> > +    WebRtc_Word16 buffer16[48*WEBRTC_OPUS_MAX_ENCODE_FRAME_SIZE_MS];
> > +    WebRtc_Word32 buffer32[64*WEBRTC_OPUS_MAX_ENCODE_FRAME_SIZE_MS+7];
> 
> They're supposed to be the maximum size of pcm data at 32 and 48 kHz,
> respectively, plus lapping for the resampler.

48 kHz and 64 kHz, actually. We upsample to 64 kHz before downsampling to 48 kHz, because those were the resampling routines the webrtc.org code had available.

Comment 34

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

>::: media/webrtc/trunk/src/supplement.gypi
>@@ +3,5 @@
>>  # Please see the WebRTC DEPS file for details.
>>  {
>>    'variables': {
>>      'build_with_chromium': 0,
>> +    'build_with_mozilla': 1,
>
>I was doing a parallel construction to build_with_chromium. Having it here >documents the flag. I see it's also set in configure; if that overrides, we can >keep the upstream setting of having both zero here.

I'm pretty certain the command-line does NOT override this.  If you want that, you'd need to make it somehow conditional on build_with_mozilla existing, and I don't know how that's done.

Comment 35

[Ralph Giles (:rillian)]

Attachment: patch against alder

Updated patch against alder.

- Address uninitialize acmCodec issue
- Better document voodoo constants
- Sync style and config changes from the webrtc.org patch

Comment 36

[Ralph Giles (:rillian)]

Attachment: patch against webrtc.org

Updated patch against webrtc.org. Addresses the uninitialized acmCodec issue jesup found in the patch against alder, along with some other minor cleanup.

Comment 37

[Ralph Giles (:rillian)]

Jesup landed the patch against alder on alder. Test away.

http://hg.mozilla.org/projects/alder/rev/33f1ba43b5d4

Comment 38

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

Comment on attachment 655189 [details] [diff] [review]
patch against alder

checked in split into 2 patches in order to split webrtc.org from mozilla code

Comment 39

[Ralph Giles (:rillian)]

Comment on attachment 655189 [details] [diff] [review]
patch against alder

marked the wrong patch obsolete

Comment 40

[Ralph Giles (:rillian)]

Attachment: patch against webrtc.org

More style fixes for the upstream patch.

Comment 41

[Ralph Giles (:rillian)]

Comment on attachment 656518 [details] [diff] [review]
patch against webrtc.org

Since this bug is getting so long, I've opened bug 786750 to track upstreaming the opus support patch. Please see that bug for newer versions of the patch against webrtc.org.

I'll leave this as the overall tracking bug for opus support in webrtc.

Comment 42

[User2351]

This test is doomed to fail:

media/webrtc/trunk/src/modules/audio_coding/main/source/acm_codec_database.cc

bool ACMCodecDB::IsOpusRateValid(int rate) {
  if ((rate < 6000) && (rate > 510000)) {

Comment 43

[Ralph Giles (:rillian)]

(In reply to User2351 from comment #42)

> This test is doomed to fail:

Indeed. Thanks!

Comment 44

[Ralph Giles (:rillian)]

Closing. Basic support has landed and works. In theory this could continue as a tracking bug for remaining issues, but the thread is already very long, so I suggest opening a new one if we want that.