Closed
Bug 983953
Opened 10 years ago
Closed 10 years ago
--with-system-libvpx fails at link: xpcom/build/Unified_cpp_xpcom_build2.cpp: undefined reference to `vpx_mem_set_functions'
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox30 fixed, firefox31 fixed)
RESOLVED
FIXED
mozilla31
People
(Reporter: jbeich, Assigned: jbeich)
References
Details
Attachments
(2 files, 4 obsolete files)
4.98 KB,
patch
|
Details | Diff | Splinter Review | |
3.80 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
libvpx (unlike ICU or sqlite) doesn't expose an API for memory functions replacement with shared library. vpx_mem_set_functions can still be used with static library if vpx_mem.h is manually installed. $ nm -D /usr/local/lib/libvpx.so | fgrep vpx_mem Exit 1 $ nm --defined-only /usr/local/lib/libvpx.a | fgrep vpx_mem vpx_mem.c.o: 0000000000000000 T vpx_mem_get_version 00000000000001c0 T vpx_mem_set_functions 0000000000000010 T vpx_mem_set_heap_size 00000000000001d0 T vpx_mem_unset_functions 0000000000000020 T vpx_memalign 0000000000000160 T vpx_memcpy 00000000000001a0 T vpx_memmove 0000000000000180 T vpx_memset nm: x86_abi_support.asm.o: no symbols nm: x86inc.asm.o: no symbols $ ls /usr/local/include/**/vpx_mem.h zsh: no matches found
Attachment #8391668 -
Flags: review?(mh+mozilla)
status-firefox30:
--- → affected
status-firefox31:
--- → affected
Comment 1•10 years ago
|
||
Comment on attachment 8391668 [details] [diff] [review] skip Review of attachment 8391668 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/build/nsXPComInit.cpp @@ +708,2 @@ > // And for VPX. > vpx_mem_set_functions(VPXReporter::Alloc, That this function is not public makes no sense. Please file an upstream bug to make it so and add a configure.in check for that function.
Attachment #8391668 -
Flags: review?(mh+mozilla) → feedback+
Comment 2•10 years ago
|
||
(This happens on Linux, too; setting platform to "All".) needinfo=Jan for comment 1.
Flags: needinfo?(jbeich)
OS: FreeBSD → All
ENOTIME to deal with privacy-invasive upstream i.e., anything Google-hosted. Blocking Tor, requiring phone verification and signing CLA may be acceptable for a committer but only drives away a volunteer like me.
Flags: needinfo?(jbeich)
A draft fix to get an idea how it may look like upstream. about:memory should report > 0 amount on vp8/9 content: {"process": "Main Process (pid 83952)", "path": "explicit/media/libvpx", "kind": 1, "units": 0, "amount": 2479488, "description": "Memory allocated through libvpx for WebM media files."}, 204.67 MB (100.0%) -- explicit ├────5.72 MB (02.80%) -- media │ ├──2.36 MB (01.16%) ── libvpx
May coexist with attachment 8391668 [details] [diff] [review].
Attachment #8399168 -
Flags: review?(mh+mozilla)
Comment 6•10 years ago
|
||
Comment on attachment 8399168 [details] [diff] [review] unhide vpx_mem.h Review of attachment 8399168 [details] [diff] [review]: ----------------------------------------------------------------- You need a configure flag, as already said in comment 1.
Attachment #8399168 -
Flags: review?(mh+mozilla) → feedback+
vpx_mem_set_functions may still not work e.g., if libvpx is built without USE_GLOBAL_FUNCTION_POINTERS. Testing that would require AC_TRY_RUN plus exposing vpx_*alloc functions or initializing decoder but... (Ted Mielczarek [:ted.mielczarek] says in bug 810716 comment 5) > We don't like to use AC_TRY_RUN because it doesn't work in cross-compiles. may or may not apply to --{with,enable}-system-foo. Also, do we wait for upstream to land this bug?
Attachment #8391668 -
Attachment is obsolete: true
Attachment #8399168 -
Attachment is obsolete: true
Attachment #8400170 -
Flags: review?(mh+mozilla)
Comment 8•10 years ago
|
||
Comment on attachment 8400170 [details] [diff] [review] check vpx_mem.h and vpx_mem_set_functions via configure Review of attachment 8400170 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather see this check disabling the vpx memory tracking in gecko, instead of system-libvpx altogether.
Attachment #8400170 -
Flags: review?(mh+mozilla) → review-
This also fixes a bug of not passing MOZ_LIBVPX_* flags before checking VPX header/lib.
Attachment #8400170 -
Attachment is obsolete: true
Attachment #8404464 -
Flags: review?(mh+mozilla)
Comment 10•10 years ago
|
||
Comment on attachment 8404464 [details] [diff] [review] auto-disable vpx_mem at configure Review of attachment 8404464 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +5412,5 @@ > + MOZ_CHECK_HEADER([vpx_mem/vpx_mem.h], > + [AC_CHECK_FUNC(vpx_mem_set_functions)]) > + if test "$ac_cv_header_vpx_mem_vpx_mem_h" = no -o \ > + "$ac_cv_func_vpx_mem_set_functions" = no; then > + AC_DEFINE(MOZ_VPX_NO_MEM) MOZ_VPX_NO_MEM_REPORTING
Attachment #8404464 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Macro renamed as requested, also rebased after bug 993546 and bug 682216. Carrying over r+ from previous version.
Attachment #8404464 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Only attachment 8410126 [details] [diff] [review] to land. Ignore the patch for upstream repo used for testing.
Keywords: checkin-needed
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8410126 [details] [diff] [review] auto-disable vpx_mem at configure [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 682215 regression User impact if declined: --with-system-libvpx broken build Testing completed (on m-c, etc.): Try build in bug 847568 comment 19, m-i soon Risk to taking this patch (and alternatives if risky): Low. In case of typos may cause either broken build or disabled vpx memory reporter. String or IDL/UUID changes made by this patch: None
Attachment #8410126 -
Flags: approval-mozilla-aurora?
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31233cc1cdd9
Assignee: nobody → jbeich
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/31233cc1cdd9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•10 years ago
|
Comment 16•10 years ago
|
||
Any reason this didn't get a review before landing?
Flags: needinfo?(jbeich)
Comment 17•10 years ago
|
||
Comment on attachment 8410126 [details] [diff] [review] auto-disable vpx_mem at configure I see now that there was a review on an earlier patch and assume you intended to carry that forward - please specify that next time
Attachment #8410126 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(jbeich)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•