Closed Bug 983953 Opened 6 years ago Closed 6 years ago

--with-system-libvpx fails at link: xpcom/build/Unified_cpp_xpcom_build2.cpp: undefined reference to `vpx_mem_set_functions'


(Firefox Build System :: General, defect)

Not set


(firefox30 fixed, firefox31 fixed)

Tracking Status
firefox30 --- fixed
firefox31 --- fixed


(Reporter: jbeich, Assigned: jbeich)




(2 files, 4 obsolete files)

Attached patch skip (obsolete) — 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/ | fgrep vpx_mem
Exit 1

$ nm --defined-only /usr/local/lib/libvpx.a | fgrep vpx_mem
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)
Comment on attachment 8391668 [details] [diff] [review]

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 check for that function.
Attachment #8391668 - Flags: review?(mh+mozilla) → feedback+
(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
Attached patch unhide vpx_mem.h (obsolete) — Splinter Review
May coexist with attachment 8391668 [details] [diff] [review].
Attachment #8399168 - Flags: review?(mh+mozilla)
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 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 on attachment 8404464 [details] [diff] [review]
auto-disable vpx_mem at configure

Review of attachment 8404464 [details] [diff] [review]:

@@ +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)

Attachment #8404464 - Flags: review?(mh+mozilla) → review+
Macro renamed as requested, also rebased after bug 993546 and bug 682216. Carrying over r+ from previous version.
Attachment #8404464 - Attachment is obsolete: true
Only attachment 8410126 [details] [diff] [review] to land. Ignore the patch for upstream repo used for testing.
Keywords: checkin-needed
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?
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Any reason this didn't get a review before landing?
Flags: needinfo?(jbeich)
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)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.