Closed Bug 981780 Opened 10 years ago Closed 10 years ago

Build error in --disable-webrtc builds: "VP8TrackEncoder.cpp:319: error: undefined reference to 'NV12ToI420'" and more

Categories

(Core :: WebRTC, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox29 --- unaffected
firefox30 --- fixed
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: dholbert, Assigned: jesup)

References

Details

(Keywords: regression, Whiteboard: [qa-] )

Attachments

(5 files, 7 obsolete files)

1.68 KB, patch
Details | Diff | Splinter Review
13.37 KB, patch
Details | Diff | Splinter Review
4.97 KB, patch
jesup
: review-
Details | Diff | Splinter Review
2.81 KB, patch
drno
: review+
Details | Diff | Splinter Review
4.60 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Since bug 970787's landing, I'm getting this build error:
{
/usr/bin/ld.gold.real: error: $OBJDIR/toolkit/library/../../content/media/encoder/Unified_cpp_content_media_encoder0.o: requires dynamic R_X86_64_PC32 reloc against 'NV12ToI420' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: read-only segment has dynamic relocations
$SRCDIR/content/media/encoder/VP8TrackEncoder.cpp:319: error: undefined reference to 'NV12ToI420'
$SRCDIR/content/media/encoder/VP8TrackEncoder.cpp:326: error: undefined reference to 'NV21ToI420'
$SRCDIR/content/media/encoder/VP8TrackEncoder.cpp:334: error: undefined reference to 'I444ToI420'
$SRCDIR/content/media/encoder/VP8TrackEncoder.cpp:342: error: undefined reference to 'I422ToI420'
}

Seems to happen reliably in clobber builds with --disable-webrtc in mozconfig.

Seems to happen if I have --disable-webrtc in my .mozconfig.  At least, it fixed itself when I removed that option.
jesup points out that libyuv doesn't live in /webrtc source tree.

However, it looks like webrtc does control whether or not it's built, as shown by this MXR search:
 http://mxr.mozilla.org/mozilla-central/search?string=build_libyuv
===========
/build/gyp.mozbuild (View Hg log or Hg annotations)

    line 26 -- 'build_libyuv': 0,

/media/webrtc/trunk/webrtc/build/common.gypi (View Hg log or Hg annotations)

    line 97 -- 'build_libyuv%': 1,

/media/webrtc/trunk/webrtc/common_video/common_video.gyp (View Hg log or Hg annotations)

    line 30 -- ['build_libyuv==1', {
===========


So: looks like libyuv isn't built by default, and webrtc turns it on.
> /build/gyp.mozbuild (View Hg log or Hg annotations)
> 
>     line 26 -- 'build_libyuv': 0,

jesup suggested switching this from 0 to 1. I tried that (and clobbered), but unfortunately it doesn't fix this -- I still get the same build error.
Hi Benjamin, 
Please help for this bug. thanks.
Assignee: nobody → bechen
Component: Video/Audio → Video/Audio: Recording
Perhaps this file needs to have a separate section that checks MOZ_WEBM_ENCODER
rather than MOZ_WEBRTC for 'media/libyuv'?
http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit.mozbuild#62
Keywords: regression
Good find -- though it looks like that's also not sufficient.

If I make this change, then a clobber build with --disable-webrtc fails during the configure step, with:
{
python: can't open file './build/dir_exists.py': [Errno 2] No such file or directory
[...]
 ERROR PROCESSING MOZBUILD FILE
 The error occurred while processing the following file:
 $SRCDIR/mozilla/media/libyuv/moz.build
}
(er, I guess it fails just *after* the configure step.)

I filed bug 982185 as a helper-bug on that failure, with more details about the failure message, BTW, since it looks like there might be a build system bug involved. (Though even with that resolved, I'm betting it'd just mean we'll have a cleaner error here. Probably still more fixing required.)
Comment on attachment 8389283 [details]
strawman #1 (busted in --disable-webrtc builds)

Aha, yes, that's probably the problem  However, media/libyuv should now be outside of all configs I think; we want it to be available for all targets so we can leverage in other spots (gfx, compositor, etc).  And note: webm encoding isn't available on B2G yet, which is why the above patch may not be sufficient.
OK, here's a patch with media/libyuv added regardless of config, and with the gyp tweak from comment 2.

This still hits the same issue noted in comment 5, in --disable-webrtc builds. 

(Nether of these patches have problems in builds with webrtc enabled, but that's not surprising since those have always been fine.)
Attachment #8389283 - Attachment is obsolete: true
Attachment #8389283 - Attachment description: fix-toolkit.mozbuild → strawman #1 (busted in --disable-webrtc builds)
Attachment #8389335 - Attachment filename: fix-toolkit.mozbuild → fix-gyp-and-toolkit.patch
Attachment #8389335 - Attachment is patch: true
Still occurs.  Can someone please take a look?
Attached patch gyp.patch (obsolete) — Splinter Review
This patch only works on Linux for enable/disable webrtc. The reason why we have "can't open file './build/dir_exists.py'" error is that when building gyp projects, we always include "media/webrtc/trunk/build/common.gypi". It runs some script to get environment settings and the relative path is only correct when we are building webrtc. Should we eliminate the dependency between gyp projects and common.gypi that is remove the code in [1]?

[1] http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/gyp_reader.py#100
Flags: needinfo?(rjesup)
Hi Randell,
Do you have comments on slee's patch?
Why not convert media/libyuv to moz.build ? gfx/angle and gfx/skia are examples of doing so outside of webrtc build. skia also has generate_mozbuild.py to facilitate future updates.
Status: NEW → ASSIGNED
All future branches affected.. i was hitting it on aurora/sparc64 when it was still 30, and now i've hit it again on central/amd64.
Even if not an acceptable solution, att #8394229 allows me to pass configure on OpenBSD using --disable-webrtc with m-c. Testing Ffx 30.0b1.
Fwiw, with att #8394229 i can build m-c fully. Ffx 30.0b1 being broken with disable-webrtc, and needing to touch windows-related stuff in media/webrtc/trunk/build/common.gypi (which is supposed to be disabled, ffs!) to work around it shows that the build system has deep issues. I always hated gyp, now i know why.
The same patch allows me to build fx 30.0b1 with disable-webrtc, but this is still gross.
Assignee: bechen → nobody
Status: ASSIGNED → NEW
I know --disable-webrtc is NPOTB thus a configuration not tested by supported platforms, but this is still in use by lots of configurations/architectures out there, so having it fixed properly would be nice.

Bug 970787 made libyuv mandatory, not taking into account at all the configurations that dont build it (ie non-webrtc ones). Benjamin, instead of dropping ownership of the bug, i really think you should help us here......
gps, since you're the moz.build owner, any hint on how to solve this ?
Flags: needinfo?(gps)
Component: Video/Audio: Recording → WebRTC
Flags: needinfo?(rjesup)
gps is on leave right now.
Flags: needinfo?(gps)
Comment on attachment 8418822 [details] [diff] [review]
fix disable-webrtc on linux/etc

>diff --git a/tools/quitter/chrome.manifest b/tools/quitter/chrome.manifest
>--- a/tools/quitter/chrome.manifest
>+++ b/tools/quitter/chrome.manifest
>@@ -1,4 +1,4 @@
>+category profile-after-change @mozilla.org/quitter-observer;1 @mozilla.org/quitter-observer;1
>+component {c235a986-5ac1-4f28-ad73-825dae9bad90} components/QuitterObserver.js
> content quitter chrome/quitter/content/
>-component {c235a986-5ac1-4f28-ad73-825dae9bad90} components/QuitterObserver.js
> contract @mozilla.org/quitter-observer;1 {c235a986-5ac1-4f28-ad73-825dae9bad90}
>-category profile-after-change @mozilla.org/quitter-observer;1 @mozilla.org/quitter-observer;1

You don't want these changes in your patch. (See bug 1006541 for what happened here. Shouldn't affect m-c anymore.)
Attached patch disable webrtcSplinter Review
I added one more flag to check whether we enable webrtc or not and use this flag to prevent running some unnecessary commands.
Attachment #8394229 - Attachment is obsolete: true
Comment on attachment 8418855 [details] [diff] [review]
disable webrtc

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

I think I found a simpler way to deal with it in my patch.  And I don't want to modify common.gypi any more than I have to, and my patch avoids any mods there.  (Thanks for the patches though; I build on what you found)
Attachment #8418855 - Flags: review-
Attachment #8418822 - Attachment is obsolete: true
<   jesup > gaston: bonus points if you make fabrice happy and write a patch to disable webrtc tests if we have --disable-webrtc (dom/media/tests/crashtests & mochitests)
<  RyanVM > lol
<  RyanVM > jesup: well played ;)

I took you up on this, and here's a shot at it, only tested in the --disable-webrtc case but seems to do the trick. Since i'm not so used to running tests ,that might of course be totally wrong, and i didnt run the whole test suite, only the webrtc relevant parts you mentioned - there might be others :)

 $mach mochitest-plain dom/media/tests/mochitest/
....
 1:07.70 10 INFO TEST-START | Shutdown
 1:07.70 11 INFO Passed:  1
 1:07.70 12 INFO Failed:  0
 1:07.70 13 INFO Todo:    0

for reference, without it:

 2:19.67 604 INFO TEST-START | Shutdown
 2:19.67 605 INFO Passed:  300
 2:19.67 606 INFO Failed:  31
 2:19.67 607 INFO Todo:    0

$mach crashtest --filter dom/media
...
 0:29.13 REFTEST INFO | Discovered 14 tests, after filtering SKIP tests, we have 0

for reference, without it:

 0:09.73 REFTEST TEST-UNEXPECTED-FAIL | file:///src/mozilla-central/dom/media/tests/crashtests/780790.html | boolean preference 'media.peerconnection.enabled' not known or wrong type
 0:09.73 REFTEST TEST-UNEXPECTED-FAIL | file:///src/mozilla-central/dom/media/tests/crashtests/791270.html | boolean preference 'media.peerconnection.enabled' not known or wrong type
etc etc etc
Attachment #8419219 - Flags: feedback?(rjesup)
Comment on attachment 8418872 [details] [diff] [review]
fix disable-webrtc on linux/etc

Fixes the build issue for me on m-i tip, and also backported on top of 30.0b2.
Attachment #8418872 - Flags: feedback+
Attachment #8418872 - Flags: review?(mh+mozilla)
Attachment #8419219 - Flags: review?(drno)
Attachment #8419219 - Flags: feedback?(rjesup)
Attachment #8419219 - Flags: feedback+
Comment on attachment 8419219 [details] [diff] [review]
disable webrtc tests when webrtc is not built

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

This might need disabling steeplechase as well, as per https://pastebin.mozilla.org/5095025 (though that misses disabling the crashtests).  I'd be good with the pastebin patch instead, with disabling crashtests added to it.
Yeah but from what i understand, to disable crashtests id' still need the layout/tools/reftest/reftest.js chunk adding webrtc status to the sandbox, no ?
(In reply to Landry Breuil (:gaston) from comment #30)
> Yeah but from what i understand, to disable crashtests id' still need the
> layout/tools/reftest/reftest.js chunk adding webrtc status to the sandbox,
> no ?

Yes, I believe so.
integrating jesup's feedback
Attachment #8419219 - Attachment is obsolete: true
Attachment #8419219 - Flags: review?(drno)
Attachment #8419245 - Flags: review?(drno)
Comment on attachment 8418872 [details] [diff] [review]
fix disable-webrtc on linux/etc

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

::: build/gyp.mozbuild
@@ +6,5 @@
>  
>  gyp_vars = {
>      'build_with_mozilla': 1,
>      'build_with_chromium': 0,
> +    'use_official_google_api_keys': 0,

Sounds like this should go in its own bug?

::: media/libyuv/build/dir_exists.py
@@ +2,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/.
> +
> +# exists to keep common.gypi happy and avoid modifying it

Shouldn't this be a copy of media/webrtc/trunk/build/dir_exists.py ?
Attachment #8418872 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #33)
> >  gyp_vars = {
> >      'build_with_mozilla': 1,
> >      'build_with_chromium': 0,
> > +    'use_official_google_api_keys': 0,
> 
> Sounds like this should go in its own bug?

a) we should never have left it in (though it didn't hurt other than running a script, b) if it defaults, common.gypi needs that script and it's hard to get it to find it.

> ::: media/libyuv/build/dir_exists.py
> @@ +2,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/.
> > +
> > +# exists to keep common.gypi happy and avoid modifying it
> 
> Shouldn't this be a copy of media/webrtc/trunk/build/dir_exists.py ?

It could - but then I have to get a license patch approval from gerv, and all I want here is to make this fail to find Win32 SDK files.
Comment on attachment 8419245 [details] [diff] [review]
disable webrtc tests when webrtc is not built

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

lgtm
Attachment #8419245 - Flags: review?(drno) → review+
Attached patch fix disable-webrtc (obsolete) — Splinter Review
updated to fix mac and bounce the scripts to the ones in the right dirs
Attachment #8420376 - Flags: review?(mh+mozilla)
Attachment #8418872 - Attachment is obsolete: true
All green on try, can we move forward on this ? I'll need to get it backported to aurora/beta....
Comment on attachment 8420376 [details] [diff] [review]
fix disable-webrtc

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

::: media/libyuv/build/dir_exists.py
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# exists to keep common.gypi happy and avoid modifying it
> +def main():
> +  return subprocess.call("../../webrtc/trunk/build/dir_exists.py",sys.argv[1:])

I'd be surprised if that actually worked. subprocess.call expects a single list for executable + arguments. You're giving the executable and a list of arguments. BTW, it would also be better to use sys.executable as executable and pass it the python script and the arguments.
Attachment #8420376 - Flags: review?(mh+mozilla) → review-
Attached patch fix disable-webrtc (obsolete) — Splinter Review
Attachment #8420376 - Attachment is obsolete: true
Comment on attachment 8421534 [details] [diff] [review]
fix disable-webrtc

Hopefully better...
Attachment #8421534 - Flags: review?(mh+mozilla)
Comment on attachment 8421534 [details] [diff] [review]
fix disable-webrtc

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

::: media/libyuv/build/dir_exists.py
@@ +4,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# exists to keep common.gypi happy and avoid modifying it
> +def main():
> +  return subprocess.call([sys.executable, "../../webrtc/trunk/build/dir_exists.py", sys.argv[1:]])

[...] + sys.argv[1:], otherwise you're calling subprocess.call(['foo', 'bar', ['baz']])
Attachment #8421534 - Flags: review?(mh+mozilla) → feedback+
tested both manually and in builds.  Also https://tbpl.mozilla.org/?tree=Try&rev=f55daf727ee7
Attachment #8421534 - Attachment is obsolete: true
Attachment #8421944 - Flags: review?(mh+mozilla)
Comment on attachment 8421944 [details] [diff] [review]
fix disable-webrtc

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

::: media/libyuv/build/dir_exists.py
@@ +8,5 @@
> +
> +def main():
> +  return subprocess.call([sys.executable, "../webrtc/trunk/build/dir_exists.py"] + sys.argv[1:])
> +
> +print os.getcwd()

Do you really want this?
Attachment #8421944 - Flags: review?(mh+mozilla) → review+
fwiw, i can build m-i with --disable-webrtc fine with what was commited. Yay!
Comment on attachment 8421944 [details] [diff] [review]
fix disable-webrtc

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 970787

User impact if declined: No user impact; inability to make --disable-webrtc builds which are used by some downstream maintainers (and 1.3T)

Testing completed (on m-c, etc.): On m-c soon.  Tested by BSD maintainer

Risk to taking this patch (and alternatives if risky): none, build config only

String or IDL/UUID changes made by this patch: none
Attachment #8421944 - Flags: approval-mozilla-aurora?
Attachment #8419245 - Flags: approval-mozilla-aurora?
Attachment #8421944 - Flags: approval-mozilla-beta?
Attachment #8419245 - Flags: approval-mozilla-beta?
Attachment #8419245 - Flags: approval-mozilla-beta?
Attachment #8419245 - Flags: approval-mozilla-beta+
Attachment #8419245 - Flags: approval-mozilla-aurora?
Attachment #8419245 - Flags: approval-mozilla-aurora+
Attachment #8421944 - Flags: approval-mozilla-beta?
Attachment #8421944 - Flags: approval-mozilla-beta+
Attachment #8421944 - Flags: approval-mozilla-aurora?
Attachment #8421944 - Flags: approval-mozilla-aurora+
http://buildbot.rhaalovely.net/builders/mozilla-aurora-sparc64/builds/422
http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/801

32 & 31 confirmed building fine on sparc64 using --disable-webrtc (and a fix for 1005449)
Will test beta/30 with the next b5..
Whiteboard: [webrtc-uplift] → [webrtc-uplift] [qa-]
Whiteboard: [webrtc-uplift] [qa-] → [qa-]
FWIW, it looks like the patches that landed here may have introduced a version of the error in comment 5 ("ERROR PROCESSING MOZBUILD FILE" for libyuv), in some build configurations. See bug 982185 comment 6 and beyond for more.
You need to log in before you can comment on or make changes to this bug.