1148203 - Building with combinations of --{enable,disable}-{fmp4,ffmpeg,eme} fails with compile error

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

On Linux building with --disable-fmp4 I get the following error:

0:21.64 In file included from obj-x86_64-unknown-linux-gnu/layout/build/Unified_cpp_layout_build0.cpp:20:0:
 0:21.64 /home/chris/src/gecko/layout/build/nsLayoutStatics.cpp:104:33: fatal error: FFmpegRuntimeLinker.h: No such file or directory
 0:21.64  #include "FFmpegRuntimeLinker.h"
 0:21.64                                  ^
 0:21.64 compilation terminated.

Comment 1

[cajbir (:cajbir)]

It looks like the fmp4 support is required so I plain to remove the disable/enable fmp4 flag then work on any remaining build errors that result from combinations of enable/disable-{ffmpeg,eme}.

Comment 2

[cajbir (:cajbir)]

Table of build options and compile failures:

fmp4 ffmpeg eme
yes  no     no   <== compiles
yes  no     yes  <== compiles
yes  yes    no   <== compiles
yes  yes    yes  <== compiles
no   yes    no   <== fail ffmpegruntimelinker
no   yes    yes  <== fail, eme requires fmp4
no   no     yes  <== fail, eme requires fmp4
no   no     no   <== fail, moof parser, deumer log

"yes" = enable- and "no" = disable- at configure time.

Comment 4

[cajbir (:cajbir)]

Attachment: Fix

Fixes build error with  all three disabled. Prevents other combinations
that cause build errors by detecting them at configure time and displaying
an error message.

Specifically, this patch changes things so fragmented mp4 is required if ffmpeg is requested.

:cpearce for media change and feedback on the FMP4 requirement for FFMPEG, :ted for configure.in change.

Comment 5

[cajbir (:cajbir)]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6d3372f2007

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8589404 [details] [diff] [review]
Fix

Review of attachment 8589404 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5331,5 @@
>      MOZ_FMP4=,
>      MOZ_FMP4=1)
>  
> +if test -n "$MOZ_FFMPEG" -a -z "$MOZ_FMP4"; then
> +    AC_MSG_ERROR([Fragmented MP4 support must be enabled if using FFMPEG])

Yes, the FFMPEG backend won't work unless MP4 support is enabled too.

::: media/libstagefright/binding/MoofParser.cpp
@@ +13,5 @@
> +static
> +PRLogModuleInfo* GetDemuxerLog() {
> +  static PRLogModuleInfo* log = nullptr;
> +  if (!log) {
> +    log = PR_NewLogModule("MP4Demuxer");

We already init another log module with the same name in the MP4Reader code... What happens when you have two log modules of the same name?

If Bad Things can happen, you could just define LOG(...) if MOZ_FMP4 is not defined.

Comment 7

[cajbir (:cajbir)]

Attachment: Fix

Address review comment by only compiling the logging stuff if FMP4 is enabled as suggested.

:cpearce for media code, :ted for configure changes.

Comment 8

[cajbir (:cajbir)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4e71fc32d87

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8590024 [details] [diff] [review]
Fix

Review of attachment 8590024 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/MoofParser.cpp
@@ +8,5 @@
>  #include <limits>
>  
>  #include "prlog.h"
>  
> +#if defined(MOZ_FMP4) && defined(PR_LOGGING)

I actually have a similar change in my tree:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/rev/24f746af8dea

I needed more than just an ifdef here to get this to build, though.

Comment 10

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/858ea94746ee

Comment 11

[cajbir (:cajbir)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> I actually have a similar change in my tree:
> http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/rev/
> 24f746af8dea
> 
> I needed more than just an ifdef here to get this to build, though.

I had a similar change originally but comment 6 noted a problem with it. This change removes logging for this file if MOZ_FMP4 is not used and builds fine in my testing. I tried all combinations of the three fmp4/ffmpeg/eme configure options.

Comment 12

[(not currently active) Ted Mielczarek]

Thanks, I'll have to try swapping my patch for yours in my iOS queue.

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/858ea94746ee