I'm assuming Chrome M26 will use stable branch 3.20 (the latest); it's always possible they'll use 3.21; if so we'll probably update to that. Import is still largely by-hand due to mercurial patch-history size issues. Documenting the procedure a bit: (originally was supposed to be almost entirely automatic via media/webrtc/webrtc_updates.sh) --- Typically I pull the stable branch, then grab the latest full peerconnection package and replace trunk/third_party/webrtc with the webrtc directory in stable (and move it to trunk/src, since that's where it used to live). I run the rest of the old update procedure, and then instead of using "hg pull" to update m-c, I generate a rollup of all changes we've made to trunk/src, then replace trunk/src with the code from the update procedure, then re-apply the changes we made. This avoids bringing in any history or unneeded changesets or deleted (large) files from the merge repo.

Too large (8MB) to attach directly with gzipping

Also needs the backout in bug 818631

This appears to work reasonably well. Tried the 3 main webrtc-landing test groups, all seem to work. I have NOT run the mochitests, crashtests or unit tests yet! I've only compiled it on Linux64 (F17)! Try: https://tbpl.mozilla.org/?tree=Try&rev=29b1e2db4b3c

https://tbpl.mozilla.org/?tree=Try&rev=0801fd6798c2 The updates include unit test fixes; all pass locally, and all mochitests pass locally. Note for clang on mac you need bug 830908 (new upstream bug)

(In reply to Randell Jesup [:jesup] from comment #4) > Also needs the backout in bug 818631 Actually Bug 818618 - numbers were too close together... :-(

To summarize: you currently need the 3 patches here: 1) Update media/webrtc/trunk/webrtc 2) Update media/webrtc/trunk except for /webrtc 3) Rollup of changes and 4) bug 818618 5) bug 830908

Comment on attachment 702430 [details] [diff] [review] rollup of changes to media/webrtc/trunk, and backouts of some temp patches Review of attachment 702430 [details] [diff] [review]: ----------------------------------------------------------------- Note: some of this rollup patch is undoing fubars in the automated import process (mostly duplicated opus stuff). I'll try to tease them apart for future re-use of this roll-up.

Comment on attachment 702175 [details] [diff] [review] Update media/webrtc/trunk except /webrtc (tools, etc) Review of attachment 702175 [details] [diff] [review]: ----------------------------------------------------------------- rs=me ::: media/webrtc/trunk/build/whitespace_file.txt @@ +65,5 @@ > A NAME MEANS A LOT JUST BY ITSELF > A POSITIVE ATTITUDE MEANS ALL THE DIFFERENCE IN THE WORLD > A RELAXED MAN IS NOT NECESSARILY A BETTER MAN > + > +This commit will change the world as we know it. Or maybe the next one.

Comment on attachment 702430 [details] [diff] [review] rollup of changes to media/webrtc/trunk, and backouts of some temp patches Review of attachment 702430 [details] [diff] [review]: ----------------------------------------------------------------- rs=me ::: media/webrtc/signaling/test/mediaconduit_unittests.cpp @@ +510,5 @@ > ASSERT_EQ(mozilla::kMediaConduitNoError, err); > > //configure send and recv codecs on the audio-conduit > //mozilla::AudioCodecConfig cinst1(124,"PCMU",8000,80,1,64000); > + mozilla::AudioCodecConfig cinst1(124,"opus",48000,960,2,64000); I think at least one of either this test or the mediapipline test need to use mono, because that's what we want to use on the actual calls until we get SDP handling for the sprop-stereo and stereo parameters. ::: media/webrtc/signaling/test/mediapipeline_unittest.cpp @@ +46,5 @@ > TestAgent() : > audio_flow_(new TransportFlow()), > audio_prsock_(new TransportLayerPrsock()), > audio_dtls_(new TransportLayerDtls()), > + audio_config_(109, "opus", 48000, 960, 2, 64000), This, too.

The compile here gets further along than it did when I started. I expect that one of my gyp changes is wrong. I may get a chance to look at this again tonight, but if not, gcp, can you give it a shot to see if you can turn it into something that builds?

Comment on attachment 702618 [details] [diff] [review] Reapply Bas's video capture patch (bug 731407) This is just re-applying the parts of bas's patch that didn't merge automatically and correcting for path renames

Windows now builds: https://tbpl.mozilla.org/?tree=Try&rev=7f79410fb48c

Comment on attachment 702618 [details] [diff] [review] Reapply Bas's video capture patch (bug 731407) Review of attachment 702618 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Comment on attachment 702641 [details] [diff] [review] WIP patch to make android build again by disabling neon Review of attachment 702641 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/shared_libs.mk @@ +47,5 @@ > endif > > # neon for ARM > +#ifeq ($(HAVE_ARM_NEON),1) > +ifeq (0,1) This can probably go entire, and the above Intel x86/SSe2 things too. Verifying... ::: media/webrtc/trunk/webrtc/common_audio/signal_processing/signal_processing.gypi @@ +83,5 @@ > ], > }], > ], > + 'defines!': [ > + 'WEBRTC_ARCH_ARM_NEON' This was done because the old drop didn't have runtime NEON detection, but this one does, so this is no longer needed (which is equivalent to backing out my workaround patch).

Comment on attachment 702430 [details] [diff] [review] rollup of changes to media/webrtc/trunk, and backouts of some temp patches Review of attachment 702430 [details] [diff] [review]: ----------------------------------------------------------------- I don't see anything offensive in the build bits of this. ::: media/webrtc/webrtc_update.sh @@ +72,5 @@ > > +#echo "" > +#hg update --clean webrtc-pending > +#hg merge -r webrtc-import-last > +#hg commit -m "merge latest import to pending, $revision" Any reason you've left this in but commented out?

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19) > > I don't see anything offensive in the build bits of this. Thanks > ::: media/webrtc/webrtc_update.sh > @@ +72,5 @@ > > > > +#echo "" > > +#hg update --clean webrtc-pending > > +#hg merge -r webrtc-import-last > > +#hg commit -m "merge latest import to pending, $revision" > > Any reason you've left this in but commented out? Because it's part of the original design, which tried to separate out pending-upstream-patches from will-stay-local-to-mozilla patches, and make pending upstreams easy to remove in the update process (by making it *look* to our code like they've been in upstream all along). However, I only ever used it for Opus, and it depended on using the process in the manner I intended - and which didn't work in practice due to the impact on hg repo size. A ToDo is to redesign all this to allow easier updates without importing history (or limiting it).

Incomplete patch to get the Android stuff to work. Still needs more work to get the .S (instead of .s) assembly files to link correctly.

This is an iteration on gcp's patch to make Android build by enabling neon. It turns out that a number of files have moved from C to assembly in this release. There are one or two that require even more build magic (teaching mozmake.py about generate_asm_header.gypi or something equivalent, however, those files have C equivalents still in the tree (which is what we were using previously anyway), so I've started by switching those files back. The rest now build after teaching mozmake.py and arm_neon.gypi about ASFLAGS and friends, as well as switching from ASFILES to SSRCS. I've got a build in-progress now, and if it manages to link, the next steps I can see are: * try the mobile audio demo and make sure it still works * try unit tests & mochitests and make sure they still pass

Hmm, we don't have linkage yet, the missing functions are in the files that I reverted to the C versions. As an example: /usr/local/android-ndk-r8c/toolchains/arm-linux-androideabi-4.6/prebuilt/darwin-x86/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /Users/dmose/r/inbound/objdir-droid/toolkit/library/../../media/webrtc/trunk/webrtc/modules/modules_audio_processing/audio_processing/aecm/aecm_core.o: in function WebRtcAecm_InitNeon:/Users/dmose/r/inbound/src/media/webrtc/trunk/webrtc/modules/audio_processing/aecm/aecm_core.c:530: error: undefined reference to 'WebRtcAecm_WindowAndFFTNeon' Interestingly, if I'm interpreting the output of the android ndk "objdump -t" correctly, the symbols appears to exist in aecm_core_neon.o: aecm_core_neon.o: file format elf32-littlearm SYMBOL TABLE: 00000000 l d .text.WebRtcAecm_WindowAndFFTNeon 00000000 .text.WebRtcAecm_WindowAndFFTNeon 00000000 l d .ARM.extab.text.WebRtcAecm_WindowAndFFTNeon 00000000 .ARM.extab.text.WebRtcAecm_WindowAndFFTNeon 00000000 l d .ARM.exidx.text.WebRtcAecm_WindowAndFFTNeon 00000000 .ARM.exidx.text.WebRtcAecm_WindowAndFFTNeon 00000000 g F .text.WebRtcAecm_WindowAndFFTNeon 000001d8 .hidden WebRtcAecm_WindowAndFFTNeon A good next step will be to decode the meaning of "objdump -t" output, both the various letters and the .hidden. I may get back to poke at this more this evening; not yet sure. I wouldn't object if these patches landed with Android still broken, as at this point it's just me and gcp that it's blocking, and we'll be working on this whether or not the main stuff is in the tree.

The last problem I described appears to be that the object with the neon symbols wasn't being linked into gkmedia; here's an updated patch that should do that. Build in progress.

OK, so Android Tbuilds with that patch link and run. Most of the mtransport C++ unit tests (ICE, Timer, SocketTransportService, Transport, Dispatch) pass. Two of the SCTP Transport tests fail. This is the exact state things were in without the uplift patches. Signaling unit tests have regressed: [ RUN ] SignalingTest.JustInit ###!!! ASSERTION: Unsupported Signaling State Transition: 'Not Reached', file /Users/dmose/r/inbound/src/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp, line 213 NS_DebugBreak+0x0000001D [libxpcom.so +0x000022B6] MediaPiplineTests crash: [ RUN ] MediaPipelineTest.AudioSend Segmentation fault remotecppunittests TEST-UNEXPECTED-FAIL | mediapipeline_unittest | test failed with return code 139 One of the TransportConduitTests asserts and fails: [ RUN ] TransportConduitTest.TestDummyAudioWithTransport /Users/dmose/r/inbound/src/media/webrtc/signaling/test/mediaconduit_unittests.cpp:495: Failure Expected: (mAudioSession) != ((void*)__null), actual: 4-byte object <00-00 00-00> vs NULL [ FAILED ] TransportConduitTest.TestDummyAudioWithTransport (121 ms) Given that, it's not too surprising that all the existing gUM mochitests pass, but as soon as we hit the first peerConnection test, the suite hangs: 54 INFO TEST-START | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudio.html

So I think I'm testing with a slightly different patch stack and slightly different changeset on mozilla-inbound that gcp was, so it'd be interesting for someone else to try this stuff, as those could conceivably be the cause of these problems (though I'd be somewhat surprised). Worth knowing, as far as running tests go: I think the instructions here are more or less right: <https://wiki.mozilla.org/Mobile/Fennec/Android#Testing>, but... on an un-rooted Android device, when running mochitests, /data/local/tests _must_ exist. However, when running C++ unit tests, I believe that /data/local/tests _must_not_ exist.

ASSERTION: Unsupported Signaling State Transition: 'Not Reached' I've seen that a few times on linux; seems intermittent. This seems likely an intermittent in signaling unrelated to this patch. Given the other comments and how close we are, I think we'll have at least temporary resolutions on these issues soon, and so I'm going to plan to land the update today (Thursday) in the the middle of the (EST) day. If we can get fixes for these issues before then, great!

Getting closer! This patch fixes some of the C++ tests by using the newfangled gyp option for enabling OpenSL ES, and fixes the gUM audio crash by fixing an uninitialized member variable bug from upstream. The audio test on the gUM landing page now worked in the one short test I tried. The assertion that jesup mentions happens 100% of the runs outside of the debugger for me (i.e. not intermittent at all), so I think this is something else. But commenting it out makes all the signaling unit tests pass. :-) Mochitests aren't even starting on my device for some reason, though they did yesterday. I briefly tested the mobile demo and couldn't make audio work. So I think the next steps are: * try the mobile demo and debug * get the mochitests running and see how they fare * debug the NS_NOTREACHED() in PeerConnectionCtx.cpp that gets triggered by the signaling tests

Mochitests run for me, the gUM test page does so too but with very stuttery audio. The demo page works too, but same problem: audio quality is much worse/more stuttery than before.

The cause of the stuttering is: ERROR ; (17:22:53:351 | 262) AUDIO DEVICE: 1 99; 10322; Audio Rec thread buffers underrun Some investigation shows that the OpenSLS backend got rewritten with a clearer buffer management. In addition, the buffers were lowered from 200ms to 80ms: media/webrtc/trunk/webrtc/modules/audio_device/android/audio_device_opensles_android.h const WebRtc_UWord16 N_REC_QUEUE_BUFFERS = 8; Apparently based on some device testing by upstream. Some experimentation shows that the stuttering goes away (on my Tab 10.1, i.e. non-NEON device) when we change it to: const WebRtc_UWord16 N_REC_QUEUE_BUFFERS = 16; The N_PLAY_QUEUE_BUFFERS doesn't seem to have an effect (or doesn't underrun/overrun).

There are still bad audio-quality bugs on Android, and some archaeology turned up the issues described in bug 832551. The short version is that for the demo we're working on, we should definitely cut a branch from before the webrtc.org landing and continue our work there. The question still remains about what to do with the Android patch we've accumulated in this bug. My suggestion would be to land it along with the uplift, since it'll leave the code in a closer-to-working state than otherwise. But I don't feel terribly strongly about that.

Updates the Android patch with gcp's buffer changes as well as fixes a couple of compiler warnings.

https://tbpl.mozilla.org/?tree=Try&rev=f0ee61a4ec78 and with mediapipeline_unittests fixed: https://tbpl.mozilla.org/?tree=Try&rev=b0355f472e9c

Plan to land Monday evening, expecting to be in Wed. Nightly

small tweak to unbreak some local builds to the revision string (remove parens); carrying forward r=ted

Try green and also end-to-end tested cross-platform https://tbpl.mozilla.org/?tree=Try&rev=1a288871646e

https://hg.mozilla.org/integration/mozilla-inbound/rev/266b7ce809a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a273b6bcd01 https://hg.mozilla.org/integration/mozilla-inbound/rev/45dcba9211fa https://hg.mozilla.org/integration/mozilla-inbound/rev/647d5aa77a34 https://hg.mozilla.org/integration/mozilla-inbound/rev/8f729db25fa0

https://hg.mozilla.org/mozilla-central/rev/266b7ce809a8 https://hg.mozilla.org/mozilla-central/rev/6a273b6bcd01 https://hg.mozilla.org/mozilla-central/rev/45dcba9211fa https://hg.mozilla.org/mozilla-central/rev/647d5aa77a34 https://hg.mozilla.org/mozilla-central/rev/8f729db25fa0

Note: that checkin was actually bug 845814