Closed
Bug 1239550
Opened 8 years ago
Closed 8 years ago
media/ffvpx/libavutil/mem.c:36:0: error: #error "<malloc.h> has been replaced by <stdlib.h>
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jbeich, Unassigned)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
jya
:
review+
glandium
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
Same as bug 1228230. Hardcoding stuff detected by top-level is fragile: - arc4random() is available on all BSDs (but unused in media/ffvpx) - malloc.h is not available on DragonFly, #error on FreeBSD and #warning on OpenBSD In file included from media/ffvpx/libavutil/mem.c:29: In file included from media/ffvpx/config.h:23: media/ffvpx/config_unix64.h:272:9: warning: 'HAVE_ARC4RANDOM' macro redefined [-Wmacro-redefined] #define HAVE_ARC4RANDOM 0 ^ mozilla-config.h:42:9: note: previous definition is here #define HAVE_ARC4RANDOM 1 ^ In file included from media/ffvpx/libavutil/mem.c:36: /usr/include/malloc.h:3:2: error: "<malloc.h> has been replaced by <stdlib.h>" #error "<malloc.h> has been replaced by <stdlib.h>" ^ 1 warning and 1 error generated.
Review commit: https://reviewboard.mozilla.org/r/30857/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30857/
Attachment #8707739 -
Flags: review?(mh+mozilla)
Attachment #8707739 -
Flags: review?(ajones)
Comment on attachment 8707739 [details] MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya The diff is generated after running: $ sed -Ei '' '/HAVE_MALLOC_H|HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN/d' media/libav/config* Also, the second reviewer was supposed to be assignee of bug 1214462. Not sure how to change via reviewboard UI...
Attachment #8707739 -
Flags: review?(ajones) → review?(jyavenard)
Comment 3•8 years ago
|
||
Comment on attachment 8707739 [details] MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya Those files are self generated, so the next time you do an update the changes will be lost. I would much prefer that either: 1-you set the value to 0 (we don't need them anyway) 2-have a dedicated config_freebsd.h/asm And in any case update the README_MOZILLA to document what was done, bonus point for making a script that automate the steps described in README_MOZILLA just so that when I next update media/ffvpx (very soon and often: the code is receiving a lot of fixes as we're currently fuzzing ffmpeg)
Attachment #8707739 -
Flags: review?(jyavenard) → review-
Updated•8 years ago
|
Attachment #8707739 -
Flags: review?(mh+mozilla)
Comment 4•8 years ago
|
||
Fwiw, OpenBSD has removed malloc.h.
Comment 5•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #3) > I would much prefer that either: > 1-you set the value to 0 (we don't need them anyway) But if you set the value to 0, wouldnt the warning about #define being redefined be triggered ?
Is MOZ_FFVPX limited to MSE or any VP8/9 content? (In reply to Jean-Yves Avenard [:jya] from comment #3) > I would much prefer that either: > 1-you set the value to 0 (we don't need them anyway) It wouldn't address bug 1214462 comment 19 or bug 1228230 comment 6. For one, arc4random() warning affects Android x86 but not Linux x86. > 2-have a dedicated config_freebsd.h/asm For a Tier3 platform a separate config often means bitrot due to ENOTIME e.g., forget to regen after AVX2 support was added half a year ago. And what about other BSDs? Each has enough difference in the config to make sharing slightly risky without testing or if more files are added in the future. config_unix* is also a misnomer, maybe rename to config_linux*. About the only not Unix-like system Mozilla still supports is Windows. Of those only OS X is allowed to use UNIX trademark. POSIX would also be wrong after replacing posix_memalign() with memalign(). Other options worth considering: 3-run ffmpeg configure as sub-configure during build similar to ICU, jemalloc, freetype 4-don't build media/ffvpx on Tier3 but always rely on system ffmpeg providing vp8/vp9 decoders > And in any case update the README_MOZILLA to document what was done, yasm 1.1 issue doesn't affect Tier 3 because of NPOTB. However, FreeBSD needs '--cc=cc' due to lack of 'gcc' hardlink to 'clang'. > bonus point for making a script that automate the steps described in README_MOZILLA How that would help if a Mozilla contributor doesn't have a FreeBSD box around?
Comment on attachment 8707739 [details] MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30857/diff/1-2/
Attachment #8707739 -
Attachment description: MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?k17e → MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya
Attachment #8707739 -
Flags: review- → review?(jyavenard)
Review commit: https://reviewboard.mozilla.org/r/39613/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39613/
Attachment #8729951 -
Flags: review?(mh+mozilla)
Attachment #8729951 -
Flags: review?(jyavenard)
Updated•8 years ago
|
Attachment #8707739 -
Flags: review?(jyavenard) → review+
Comment 10•8 years ago
|
||
Comment on attachment 8707739 [details] MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya https://reviewboard.mozilla.org/r/30857/#review36445
Comment 11•8 years ago
|
||
Fwiw, because of this i cant build 46.0b5 on OpenBSD when malloc.h is missing. Will have to figure out what to put in those damn os-specific config files...
Comment 12•8 years ago
|
||
I'm wondering if it wouldn't be simpler to have a special header added to CFLAGS doing something like CFLAGS -include bsd.h that would do something like: #undef HAVE_MALLOC_H #define HAVE_MALLOC_H 0 would be much easier and also much easier to maintain
Comment 13•8 years ago
|
||
Actually, much more easily, in config.h Add at the end: #if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) // or whaever is required for openbsd) #undef HAVE_MALLOC_H #define HAVE_MALLOC_H 0 #endif :gaston could you give this a try? (can wrap a patch for you if needed)
Flags: needinfo?(landry)
Comment 14•8 years ago
|
||
I had a slightly different hack (cf https://cgit.rhaalovely.net/mozilla-firefox/commit/?h=beta&id=0bbd84875372e45aad60a3a2d9371a0af6e00884) but i'll try this. While here, shouldnt we handle HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN the same ? jan ?
Flags: needinfo?(landry) → needinfo?(jbeich)
Comment 15•8 years ago
|
||
i don't want to modify the original ffmpeg code. makes resync too cumbersome.
Reporter | ||
Comment 16•8 years ago
|
||
(In reply to Landry Breuil (:gaston) from comment #14) > I had a slightly different hack (cf > https://cgit.rhaalovely.net/mozilla-firefox/commit/ > ?h=beta&id=0bbd84875372e45aad60a3a2d9371a0af6e00884) but i'll try this. I plan to use comment 1 fix for our firefox46 update. comment 7 is too cumbersome to backport. media/ffvpx can be disabled as well as it's a fallback for when system ffmpeg is not available. > While here, shouldnt we handle > HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN the same > ? jan ? Are you referring to comment 13? HAVE_LOCALTIME_R value is the same as on Linux. While ugly it'd be acceptable. Maybe also check defined(__Bitrig__) in new diffs[1]. [1] if using Rust bootstraps as an indicator of how alive the community is ;) https://github.com/bitrig/bitrig/commit/e040863a6a32f6e676e046aac31b823bac563a84
Flags: needinfo?(jbeich)
Comment 17•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #13) > Actually, much more easily, in config.h > > Add at the end: > #if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || > defined(__OpenBSD__) // or whaever is required for openbsd) > #undef HAVE_MALLOC_H > #define HAVE_MALLOC_H 0 > #endif > > :gaston could you give this a try? (can wrap a patch for you if needed) This works here. (In reply to Jan Beich from comment #16) > Are you referring to comment 13? HAVE_LOCALTIME_R value is the same as on > Linux. While ugly it'd be acceptable. Maybe also check defined(__Bitrig__) > in new diffs[1]. Yeah, i was mostly thinking about HAVE_ARC4_RANDOM since it was yielding warnings.. Bitrig is more or less dead.
Comment 18•8 years ago
|
||
(In reply to Jan Beich from comment #16) > I plan to use comment 1 fix for our firefox46 update. comment 7 is too > cumbersome to backport. media/ffvpx can be disabled as well as it's a > fallback for when system ffmpeg is not available. not in 47 and later. media/ffvpx is always used to decode vp8 and vp9. We prefer it over the system ffmpeg as we control it (and it's the latest version) if :gaston is reporting that it works, that I may as well just do that. Easy to write, and easy to maintain.
Updated•8 years ago
|
Attachment #8729951 -
Flags: review?(mh+mozilla)
Attachment #8729951 -
Flags: review?(jyavenard)
Attachment #8729951 -
Flags: review-
Comment 19•8 years ago
|
||
Any hope of getting this commited 'soonish' so that it reaches beta/46 ?
Flags: needinfo?(jyavenard)
Reporter | ||
Comment 20•8 years ago
|
||
comment 13 approach wouldn't fix HAVE_ARC4RANDOM warning unless you do #undef before *and* after #include "config_*.h". (In reply to Jean-Yves Avenard [:jya] from comment #3) > Those files are self generated, so the next time you do an update the changes will be lost. For comment 1 the following in README_MOZILLA would be enough config*: replace: /HAVE_MALLOC_H|HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN/d maybe next to config_unix32.h: add to configure command: --disable-asm --disable-yasm --cc='clang -m32' replace: s/HAVE_SYSCTL 1/HAVE_SYSCTL 0/ and s/HAVE_MEMALIGN 1/HAVE_MEMALIGN 0/ and s/HAVE_POSIX_MEMALIGN 1/HAVE_POSIX_MEMALIGN 0/
Comment 21•8 years ago
|
||
(In reply to Jan Beich from comment #20) > comment 13 approach wouldn't fix HAVE_ARC4RANDOM warning unless you do > #undef before *and* after #include "config_*.h". HAVE_ARC4RANDOM is unused in the ffvpx code, and only defined in the config headers. So why would its value matter ?
Flags: needinfo?(jyavenard)
Comment 22•8 years ago
|
||
sorry, re-reading the original comment. maybe easiest is to remove the value completely seeing that it doesn't matter.
Comment 23•8 years ago
|
||
(In reply to Jan Beich from comment #20) > comment 13 approach wouldn't fix HAVE_ARC4RANDOM warning unless you do > #undef before *and* after #include "config_*.h". > > (In reply to Jean-Yves Avenard [:jya] from comment #3) > > Those files are self generated, so the next time you do an update the changes will be lost. > > For comment 1 the following in README_MOZILLA would be enough > > config*: > replace: > / > HAVE_MALLOC_H|HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMAL > IGN/d I'm a bit confused. Are you saying HAVE_MALLOC_H should be removed completely on all config files ? otherwise, if so yes, this may be easier for all systems (And also easier to create the files)
Reporter | ||
Comment 24•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #23) > I'm a bit confused. Are you saying HAVE_MALLOC_H should be removed > completely on all config files ? Correct. old-configure.in via mozilla-config.h already defines HAVE_MALLOC_H if <malloc.h> exists. mozilla-config.h is forced upon everything within Gecko even if it only causes namespace pollution. HAVE_ARC4RANDOM and a few others are also defined by old-configure.in.
Reporter | ||
Comment 25•8 years ago
|
||
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39613/diff/1-2/
Attachment #8729951 -
Attachment description: MozReview Request: Bug 1239550 - Add media/ffvpx/ configs for DragonFly, FreeBSD, NetBSD. r?jya r?glandium → MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya
Attachment #8729951 -
Flags: review?(mh+mozilla)
Attachment #8729951 -
Flags: review?(jyavenard)
Attachment #8729951 -
Flags: review-
Attachment #8707739 -
Attachment is obsolete: true
Reporter | ||
Comment 26•8 years ago
|
||
MozReview didn't notice but this is a rebased version from comment 1 with comment 3 addressed by a comment in README_MOZILLA. The replace command requires passing -r or -E flag to sed(1) to enable extended regular expressions: /HAVE_(MALLOC_H|ARC4RANDOM|LOCALTIME_R|MEMALIGN|POSIX_MEMALIGN)/d unless you prefer longer version /HAVE_MALLOC_H/d; /HAVE_ARC4RANDOM/d; /HAVE_LOCALTIME_R/d; /HAVE_MEMALIGN/d; /HAVE_POSIX_MEMALIGN/d
Comment 27•8 years ago
|
||
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya https://reviewboard.mozilla.org/r/39613/#review41863
Attachment #8729951 -
Flags: review?(jyavenard) → review+
Comment 28•8 years ago
|
||
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya https://reviewboard.mozilla.org/r/39613/#review42165 ::: media/ffvpx/README_MOZILLA:18 (Diff revision 2) > > configuration files were generated as follow using the configure script: > ./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm > > +config*: > +replace: /HAVE_(MALLOC_H|ARC4RANDOM|LOCALTIME_R|MEMALIGN|POSIX_MEMALIGN)/d That's not replace, that's delete
Attachment #8729951 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 29•8 years ago
|
||
https://reviewboard.mozilla.org/r/39613/#review42165 > That's not replace, that's delete Syntax of various fields in README_MOZILLA is underdocumented. `replace` can be either sed(1), perl(1) or ad hoc expression. Technically, any modification is a "replacement" of the existing content.
Reporter | ||
Comment 30•8 years ago
|
||
Jean, how do you invoke `replace` lines after ffmpeg update?
Flags: needinfo?(jyavenard)
Comment 31•8 years ago
|
||
(In reply to Jan Beich from comment #30) > Jean, how do you invoke `replace` lines after ffmpeg update? not sure what you mean... I do it all manually. so here I'll be using sed :)
Flags: needinfo?(jyavenard)
Comment 32•8 years ago
|
||
https://reviewboard.mozilla.org/r/39613/#review42165 > Syntax of various fields in README_MOZILLA is underdocumented. `replace` can be either sed(1), perl(1) or ad hoc expression. Technically, any modification is a "replacement" of the existing content. I think it's okay as is, or maybe just update the comment giving the GNU sed syntax (as the command will be run from linux). I can't push while this issue is opened. Once marked as fixed, I can autoland it.
Reporter | ||
Comment 33•8 years ago
|
||
https://reviewboard.mozilla.org/r/39613/#review42165 > I think it's okay as is, or maybe just update the comment giving the GNU sed syntax (as the command will be run from linux). I can't push while this issue is opened. Once marked as fixed, I can autoland it. The syntax used here is the same as POSIX except for extended (i.e. not basic) regular expressions. Both GNU sed and BSD sed support it using `-r` or `-E` flag. I'm not sure how to prepend command-line flags for `replace` commands.
Comment 34•8 years ago
|
||
The just either drop the issue or make it as fixed. until then it can't be committed.
Reporter | ||
Comment 35•8 years ago
|
||
But I did drop already via MozReview. Do you mean something else?
Comment 37•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/037893f93079
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 38•8 years ago
|
||
[Tracking Requested - why for this release]: Do we want to backport this to beta/aurora to fix build on BSDs ? Guess we'll let the rev bake on m-c for some time first..
Comment 39•8 years ago
|
||
Too late for 46, tracking for 47+ since it sounds possible we could uplift a fix so that BSD builds will work.
tracking-firefox48:
--- → +
Reporter | ||
Comment 40•8 years ago
|
||
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya This could have made into to 46.0.1. ;( Now is really too late but we're not even halfway to 47.0, so 46.0.2 may still happen (like with 40.x). Approval Request Comment [Feature/regressing bug #]: bug 1214462 regression [User impact if declined]: broken build Bitrig, DragonFly, FreeBSD, OpenBSD [Describe test coverage new/current, TreeHerder]: m-c [Risks and why]: Low, broken build if too out of sync with autoconf changes [String/UUID change made/needed]: None
Attachment #8729951 -
Flags: approval-mozilla-release?
Attachment #8729951 -
Flags: approval-mozilla-beta?
Comment 41•8 years ago
|
||
It's great that we have a fix now! Some ways to avoid this situation: add the keyword tag "regression" to the bug. Request tracking earlier than the last week of beta cycle. And explain the impact clearly. Do we have test coverage now so we don't break BSD builds like this without noticing?
Flags: needinfo?(jyavenard)
Flags: needinfo?(jbeich)
Updated•8 years ago
|
Comment 42•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #41) > Do we have test coverage now so we don't break BSD builds like this without > noticing? I'm not sure there's particular test coverage for this issue, but we have an out-of-mozilla BSD buildbot... http://buildbot.rhaalovely.net/one_line_per_build i dunno why freebsd is red those days, but openbsd went definitely green after this issue was fixed in central (and aurora turned green at the last central->aurora merge)
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya Fix broken BSD builds, Aurora48+, Beta47+
Attachment #8729951 -
Flags: approval-mozilla-release?
Attachment #8729951 -
Flags: approval-mozilla-release+
Attachment #8729951 -
Flags: approval-mozilla-beta?
Attachment #8729951 -
Flags: approval-mozilla-beta+
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya Oops, I thought I was approving for Aurora. Did *not* mean to approve for release. It's Liz's decision.
Attachment #8729951 -
Flags: approval-mozilla-release+ → approval-mozilla-release?
Reporter | ||
Comment 45•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #41) > add the keyword tag "regression" to the bug Thanks for clearing the keyword usage for Tier3 build issues. > Do we have test coverage now so we don't break BSD builds like this without > noticing? All BSDs are Tier3 platforms i.e., no Treeherder support. Landry's buildbot doesn't attempt to run tests. For FFmpeg upstream - yes, see http://fate.ffmpeg.org/ (In reply to Landry Breuil (:gaston) from comment #42) > i dunno why freebsd is red those days libc++ issues. bug 1260208, bug 1246743 should improve test coverage. Aurora is currently broken by bug 1259537 and Nightly also by bug 1269171.
Flags: needinfo?(jbeich)
Keywords: regression
Comment 46•8 years ago
|
||
Doesn't the BSD port system have a set of patched to apply to the current tree anyway? why can't this simply be added to the BSD port instead? at least you're guaranteed the outcome. I haven't used FreeBSD since version 9, but that's how it used to work at least
Flags: needinfo?(jyavenard)
Reporter | ||
Comment 47•8 years ago
|
||
Less patches makes it easier for upstream to review how downstream ships Firefox to the end users. When such users file a bug that's believed to be an upstream issue being able to build unpatched source without jumping through hoops to fix it also helps.
> why can't this simply be added to the BSD port instead?
PkgSrc lacks the fix. While mainly used on NetBSD (unaffected) it strives to support everything.
Comment 48•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/936cfcbdd504
Comment 49•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #46) > Doesn't the BSD port system have a set of patched to apply to the current > tree anyway? There are some 'integration patches' in FreeBSD/OpenBSD/NetBSD ports-trees but we strive to be able to build mozilla-central *unpatched* on our buildbot. > > why can't this simply be added to the BSD port instead? at least you're > guaranteed the outcome. That's already the case since 46 didnt build.. cf http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-media_ffvpx_config_h?rev=1.1&content-type=text/x-cvsweb-markup - i went the easy route instead of a jumbo patch
Comment 50•8 years ago
|
||
Wontfix for 46 at this point. 47 is planned to release June 7.
Comment on attachment 8729951 [details] MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya Clearing the m-r flag as 46 will EOL in a week and this fix is already in Beta47.
Attachment #8729951 -
Flags: approval-mozilla-release?
You need to log in
before you can comment on or make changes to this bug.
Description
•