Closed Bug 1101651 Opened 10 years ago Closed 10 years ago

Enable WebRTC unit tests to be built using standalone WebRTC library

Categories

(Core :: WebRTC, defect)

Product:
Core
Component:
WebRTC
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

Attachments

(3 files, 24 obsolete files)

0001-Bug-1101651-Part-1-xpcomrt-version-of-dom-media-library-need-for-standalone-webrtcs-15040909-a9ab13d.patch
11.91 KB, patch
Details | Diff | Splinter Review
0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040909-40de3e8.patch
138.84 KB, patch
glandium
: feedback-
Details | Diff | Splinter Review
0003-Bug-1101651-Part-3-update-cppunitest.ini-file-15040909-32545f9.patch
1.22 KB, patch
Details | Diff | Splinter Review
It should be possible to build and run most of the WebRTC unit tests with the standalone WebRTC library. This will help verify the standalone library and help detect build breakage.
Assignee: nobody → rbarker
Summary: Enable WebRTC unit test to be built using standalone WebRTC library → Enable WebRTC unit tests to be built using standalone WebRTC library
Blocks: 1079348
Depends on: 1093934, 1094250, 1097804
Depends on: 1112236
Attached patch DOM Media v1 (obsolete) — — Splinter Review
Attached patch WebRTC Standalone v1 (obsolete) — — Splinter Review
Attached patch Part 1: DOM Media v2 (obsolete) — — Splinter Review
Attachment #8544902 - Attachment is obsolete: true
Attached patch Part 2: WebRTC Standalone v3 (obsolete) — — Splinter Review
Attachment #8544904 - Attachment is obsolete: true
Blocks: 1121679
No longer blocks: 1079348
Blocks: 1079348
Blocks: 1121677
Attachment #8554044 - Flags: review?(rjesup)
Attachment #8554046 - Flags: review?(rjesup)
Comment on attachment 8554047 [details] [diff] [review] 0003-Bug-1101651-Part-3-share-common-moz.build-for-webrtc-unit-tests-db2092b.patch I'm not sure if it makes sense to make a common build file since the end goal is to only have one set of unit tests, but until the standalone supports GMP and nsPref the unit tests need to be built twice.
Attachment #8554047 - Flags: review?(rjesup)
Attachment #8554047 - Attachment is obsolete: true
Attachment #8554047 - Flags: review?(rjesup)
Attachment #8554049 - Attachment is obsolete: true
Comment on attachment 8554898 [details] [diff] [review] 0001-Bug-1101651-Part-1-xpcomrt-version-of-dom-media-library-need-for-standalone-webrtcs-4082709.patch Updated patch after rebase.
Attachment #8554898 - Flags: review?(rjesup)
Comment on attachment 8554899 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-caf1ba1.patch Updated patch after rebase.
Attachment #8554899 - Flags: review?(rjesup)
Comment on attachment 8554900 [details] [diff] [review] 0003-Bug-1101651-Part-3-share-common-moz.build-for-webrtc-unit-tests-d31e1f6.patch Updated patch after rebase.
Attachment #8554900 - Flags: review?(rjesup)
Comment on attachment 8554898 [details] [diff] [review] 0001-Bug-1101651-Part-1-xpcomrt-version-of-dom-media-library-need-for-standalone-webrtcs-4082709.patch Review of attachment 8554898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/AudioSegment.h @@ +198,5 @@ > MOZ_ASSERT(channels == segmentChannelCount); > output.SetLength(channels); > bufferPtrs.SetLength(channels); > +#if !defined(MOZILLA_XPCOMRT_API) > +// FIXME XPCOMRT perhaps more descriptive, or point to a bug? @@ +211,5 @@ > T* out = output[i].AppendElements(outSize); > uint32_t outFrames = outSize; > > +#if !defined(MOZILLA_XPCOMRT_API) > +// FIXME XPCOMRT ditto ::: dom/media/SimpleImageBuffer.cpp @@ +15,5 @@ > + mWidth = width; > + mHeight = height; > + if (!mBuffer || (size > mBufferSize)) { > + if (mBuffer) { > + delete []mBuffer; Nit: add a space (or move it). In the tree, there are 9 samples like this, and 1000 of delete[] foo or delete [] foo. @@ +25,5 @@ > + } > + } > + > + if (mBuffer) { > + if (frame && (size > 0)) { memcpy((void *)mBuffer, (const void*)frame, size); } no one-line "if (foo) { blah; }" @@ +41,5 @@ > + > +const unsigned char* > +SimpleImageBuffer::GetImage(unsigned int* size) const > +{ > + if (size) { *size = mSize; } ditto ::: dom/media/SimpleImageBuffer.h @@ +21,5 @@ > + const unsigned char* GetImage(unsigned int* size) const; > + void GetWidthAndHeight(int* width, int* height) const > + { > + if (width) { *width = mWidth; } > + if (height) { *height = mHeight; } newlines @@ +27,5 @@ > + > +protected: > + ~SimpleImageBuffer() > + { > + delete []mBuffer; add a space ::: dom/media/standalone/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': the moz.build changes will need build peer review; look good to me
Attachment #8554898 - Flags: review?(rjesup) → review+
Comment on attachment 8554899 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-caf1ba1.patch Review of attachment 8554899 [details] [diff] [review]: ----------------------------------------------------------------- leaving open to decide on how to handle the unittest directory breakdown ::: media/mtransport/testlib/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +include('../objs.mozbuild') non-trivial moz.build changes need build peer review ::: media/webrtc/signaling/test/moz.build @@ +9,2 @@ > 'jsep_session_unittest', > + #'mediaconduit_unittests', This is a problem... ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +4309,3 @@ > match = offer.find("profile-level-id"); > ASSERT_NE(std::string::npos, match); > + if (match != std::string::npos) { Do we need the if() right after asserting it's not npos? ::: media/webrtc/signaling/test/xul/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': Why is this directory (and the tests) called "xul"? I'd far prefer to leave the main directory as the primary tests, and have a 'standalone' subdir that includes files from the main. Is that not possible?
Comment on attachment 8554900 [details] [diff] [review] 0003-Bug-1101651-Part-3-share-common-moz.build-for-webrtc-unit-tests-d31e1f6.patch Review of attachment 8554900 [details] [diff] [review]: ----------------------------------------------------------------- r+ modulo the directory name/split issue (and build peer approval).
Attachment #8554900 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #18) > Comment on attachment 8554899 [details] [diff] [review] > 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using- > standalone-WebRTC-library-caf1ba1.patch > > Review of attachment 8554899 [details] [diff] [review]: > ----------------------------------------------------------------- > > leaving open to decide on how to handle the unittest directory breakdown > I have no problem switching it. It probably makes more sense to put the standalone tests in their own directory. The original thinking was that the unit tests would only be built once with the XPCOMRT. Unfortunately the missing parts (GMP, nsPrefs, nsIOService etc) in XPCOMRT make that unrealistic in the short term. Eventually the standalone WebRTC library will need to support these missing features but that is something that can be dealt with if/when it happens. > ::: media/mtransport/testlib/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +include('../objs.mozbuild') > > non-trivial moz.build changes need build peer review > > ::: media/webrtc/signaling/test/moz.build > @@ +9,2 @@ > > 'jsep_session_unittest', > > + #'mediaconduit_unittests', > > This is a problem... > It is only commented out of the xpcomrt version because it uses GMP. See Bug 1121677. I thought I had a comment in there with the bug, must have gone missing in one of the many rebases. I'll add it back in. > ::: media/webrtc/signaling/test/signaling_unittests.cpp > @@ +4309,3 @@ > > match = offer.find("profile-level-id"); > > ASSERT_NE(std::string::npos, match); > > + if (match != std::string::npos) { > > Do we need the if() right after asserting it's not npos? > I'm pretty sure it was need at the time I added it because the unit test was crashing but may not be need now. I will remove it to verify. > ::: media/webrtc/signaling/test/xul/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > > Why is this directory (and the tests) called "xul"? > > I'd far prefer to leave the main directory as the primary tests, and have a > 'standalone' subdir that includes files from the main. Is that not possible? I will switch things around so that standalone is the subdir.
Blocks: 1126414
Blocks: 1127510
Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Updated patch to address comments.
Attachment #8558845 - Flags: review?(rjesup)
Attachment #8558846 - Flags: review?(dminor)
Attachment #8558845 - Flags: review?(gps)
Updated patch to address review comments. carry forward r+ from :rjesup
Attachment #8554898 - Attachment is obsolete: true
Attachment #8558846 - Flags: review?(dminor) → review+
Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Review of attachment 8558845 [details] [diff] [review]: ----------------------------------------------------------------- glandium should take this since he did the Gyp integration last and has the libraries story more paged in than I do.
Attachment #8558845 - Flags: review?(gps) → review?(mh+mozilla)
Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Review of attachment 8558845 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/standalone/moz.build @@ +4,5 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > + Library('mtransport_standalone') If you don't need this library in that case, don't recurse in this directory instead. ::: media/mtransport/testlib/moz.build @@ +72,5 @@ > +DEFINES['R_DEFINED_UINT2'] = 'uint16_t' > +DEFINES['R_DEFINED_INT4'] = 'int32_t' > +DEFINES['R_DEFINED_UINT4'] = 'uint32_t' > +DEFINES['R_DEFINED_INT8'] = 'int64_t' > +DEFINES['R_DEFINED_UINT8'] = 'uint64_t' Why do we need to build mtransport *three* times? (Also, that's three moz.builds with 98% the same content) ::: media/webrtc/moz.build @@ +99,5 @@ > build_for_test=1 > ) > + GYP_DIRS['signalingtest'].variables.update( > + build_for_standalone=0 > + ) GYP_DIRS['signalingtest'].variables.update( build_for_test=1, build_for_standalone=0, ) ::: toolkit/toolkit.mozbuild @@ +63,5 @@ > '/media/webrtc', > '/media/mtransport/third_party', > '/media/mtransport/build', > '/media/mtransport/standalone', > + '/media/mtransport/testlib', I think it's time to make directory traversal for /media/webrtc and /media/mtransport more "normal" (as in, have those directories be in DIRS in /media/{webrtc,mtransport}/moz.build.
Attachment #8558845 - Flags: review?(mh+mozilla) → feedback-
Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Review of attachment 8558845 [details] [diff] [review]: ----------------------------------------------------------------- One concern is that the proliferation of "if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API)" everywhere; it'll be easy for people to mess up. Maybe define a single thing to check everywhere? "if !defined(MOZILLA_EXTERNAL_LINKAGE)"?? r+ with note that glandium's comments need to be dealt with. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ +624,5 @@ > NS_ABORT_IF_FALSE(CheckThreadInt(), "Wrong thread"); > } > bool CheckThreadInt() const { > +#if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API) > +// FIXME XPCOMRT Work needed here it seems ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +169,5 @@ > mMainThread(mParent->GetMainThread()), > mSTSThread(mParent->GetSTSThread()), > mProxyResolveCompleted(false) { > +#if defined(MOZILLA_XPCOMRT_API) > + // FIXME Bug 1126039 Standalone XPCOMRT does not currently support nsIProtocolProxyService or nsIIOService // TODO(bug N) please ::: media/webrtc/signaling/test/FakeMediaStreams.h @@ +33,5 @@ > class MediaSegment; > }; > > +class Fake_VideoSink { > + public: indent wrong
Attachment #8558845 - Flags: review?(rjesup) → review+
Rebased patch to latest inbound. carry forward r+ from :rjesup
Attachment #8558851 - Attachment is obsolete: true
Updated patch to address review comments. carry forward r+ from :rjesup
Attachment #8558845 - Attachment is obsolete: true
Rebased to latest inbound. carry forward r+ from :dminor
Attachment #8558846 - Attachment is obsolete: true
Keywords: checkin-needed
Rebased patch to latest inbound. carry forward r+ from :rjesup
Attachment #8587531 - Attachment is obsolete: true
Flags: needinfo?(rbarker)
Updated patch to address mochitest failure. carry forward r+ from :rjesup
Attachment #8587534 - Attachment is obsolete: true
Rebased to latest inbound. carry forward r+ from :dminor
Attachment #8587536 - Attachment is obsolete: true
Keywords: checkin-needed
I have identified the mochi test failure and the code is now able to pass with this latest patch.
Hi, Part 2 failed to apply: patching file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp Hunk #6 FAILED at 383 1 out of 42 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040315-0e5f89b.patch
Flags: needinfo?(rbarker)
Keywords: checkin-needed
Rebased patch to latest inbound. carry forward r+ from :rjesup
Attachment #8588208 - Attachment is obsolete: true
Flags: needinfo?(rbarker)
Rebased patch to latest inbound. carry forward r+ from :rjesup
Attachment #8588209 - Attachment is obsolete: true
Rebased to latest inbound. carry forward r+ from :dminor
Attachment #8588210 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Randall, sorry this failed again like patching file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp Hunk #5 FAILED at 312 1 out of 42 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040715-eb47f92.patch could you take a look, thanks!
Flags: needinfo?(rbarker)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #44) > Hi Randall, > > sorry this failed again like patching file > media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > Hunk #5 FAILED at 312 > 1 out of 42 hunks FAILED -- saving rejects to file > media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working dir > errors during apply, please fix and refresh > 1-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC- > library-15040715-eb47f92.patch > > > could you take a look, thanks! Unfortunately this patch bit rots rapidly, sometimes it will have bit rotted before I've even completed a try run with it.
Flags: needinfo?(rbarker)
Rebased patch to latest inbound. carry forward r+ from :rjesup
Attachment #8589345 - Attachment is obsolete: true
Rebased patch to latest inbound. carry forward r+ from :rjesup
Attachment #8589347 - Attachment is obsolete: true
Rebased to latest inbound. carry forward r+ from :dminor
Attachment #8589349 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 1153378
Patch 2 breaks build --with-system-nspr for me ============================== ERROR PROCESSING MOZBUILD FILE ============================== The error occurred while processing the following file or one of the files it includes: ./mozilla-central/media/webrtc/signaling/test/standalone/moz.build The error occurred when validating the result of the execution. The reported error is: USE_LIBS contains "/nsprpub/lib/libc/src/plc4", but there is no "plc4" LIBRARY_NAME in nsprpub/lib/libc/src. Am I doing something wrong?
Comment on attachment 8590317 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040909-40de3e8.patch Review of attachment 8590317 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/test/moz.build @@ +40,5 @@ > LOCAL_INCLUDES += [ > '/media/mtransport/third_party/nrappkit/src/port/linux/include', > ] > + USE_LIBS += [ > + 'static:/nsprpub/lib/libc/src/plc4', You really shouldn't do this. Just USE_LIBS += ['nspr'] That's what causes comment 51.
Attachment #8590317 - Flags: feedback-
Depends on: 1156621
Depends on: 1160745
Blocks: 1167306
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: