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

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: jesup)

Tracking

({regression})

Trunk
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 unaffected, firefox30 fixed, firefox31 verified, firefox32 verified, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [qa-] )

Attachments

(5 attachments, 7 obsolete attachments)

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
Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
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.
Reporter

Comment 2

5 years ago
> /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
Reporter

Comment 5

5 years ago
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
}
Reporter

Comment 6

5 years ago
(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.
Reporter

Comment 8

5 years ago
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
Reporter

Updated

5 years ago
Attachment #8389283 - Attachment description: fix-toolkit.mozbuild → strawman #1 (busted in --disable-webrtc builds)
Reporter

Updated

5 years ago
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?

Comment 11

5 years ago
Posted 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?

Comment 13

5 years ago
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.

Updated

5 years ago
Status: NEW → ASSIGNED

Updated

5 years ago
Duplicate of this bug: 986067
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)
Reporter

Comment 23

5 years ago
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.)

Comment 24

5 years ago
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+
Posted 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-
Posted 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+
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-]
Reporter

Comment 54

5 years ago
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.
Duplicate of this bug: 982185
Duplicate of this bug: 982185
You need to log in before you can comment on or make changes to this bug.