1239550 - media/ffvpx/libavutil/mem.c:36:0: error: #error "<malloc.h> has been replaced by <stdlib.h>

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jan Beich]

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.

Comment 1

[Jan Beich]

Attachment: MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya

Review commit: https://reviewboard.mozilla.org/r/30857/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30857/

Comment 2

[Jan Beich]

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...

Comment 3

[Jean-Yves Avenard [:jya]]

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)

Comment 4

[Landry Breuil (:gaston)]

Fwiw, OpenBSD has removed malloc.h.

Comment 5

[Landry Breuil (:gaston)]

(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 ?

Comment 6

[Jan Beich]

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 7

[Jan Beich]

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/

Comment 8

[Jan Beich]

Attachment: MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

Review commit: https://reviewboard.mozilla.org/r/39613/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39613/

Comment 9

[Jan Beich]

Note, those are unmaintainable examples.

Comment 10

[Jean-Yves Avenard [:jya]]

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

[Landry Breuil (:gaston)]

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

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

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)

Comment 14

[Landry Breuil (:gaston)]

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 ?

Comment 15

[Jean-Yves Avenard [:jya]]

i don't want to modify the original ffmpeg code. makes resync too cumbersome.

Comment 16

[Jan Beich]

(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

Comment 17

[Landry Breuil (:gaston)]

(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

[Jean-Yves Avenard [:jya]]

(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.

Comment 19

[Landry Breuil (:gaston)]

Any hope of getting this commited 'soonish' so that it reaches beta/46 ?

Comment 20

[Jan Beich]

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

[Jean-Yves Avenard [:jya]]

(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 ?

Comment 22

[Jean-Yves Avenard [:jya]]

sorry, re-reading the original comment. maybe easiest is to remove the value completely seeing that it doesn't matter.

Comment 23

[Jean-Yves Avenard [:jya]]

(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)

Comment 24

[Jan Beich]

(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.

Comment 25

[Jan Beich]

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/

Comment 26

[Jan Beich]

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

[Jean-Yves Avenard [:jya]]

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

Comment 28

[Mike Hommey [:glandium]]

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

Comment 29

[Jan Beich]

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.

Comment 30

[Jan Beich]

Jean, how do you invoke `replace` lines after ffmpeg update?

Comment 31

[Jean-Yves Avenard [:jya]]

(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 :)

Comment 32

[Jean-Yves Avenard [:jya]]

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.

Comment 33

[Jan Beich]

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

[Jean-Yves Avenard [:jya]]

The just either drop the issue or make it as fixed. until then it can't be committed.

Comment 35

[Jan Beich]

But I did drop already via MozReview. Do you mean something else?

Comment 36

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/037893f93079

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/037893f93079

Comment 38

[Landry Breuil (:gaston)]

[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

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 46, tracking for 47+ since it sounds possible we could uplift a fix so that BSD builds will work.

Comment 40

[Jan Beich]

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

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 42

[Landry Breuil (:gaston)]

(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 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 45

[Jan Beich]

(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.

Comment 46

[Jean-Yves Avenard [:jya]]

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

Comment 47

[Jan Beich]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/936cfcbdd504

Comment 49

[Landry Breuil (:gaston)]

(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

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 46 at this point. 47 is planned to release June 7.

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.