Closed Bug 1148203 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
firefox-esr38 --- affected

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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}.
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.
Summary: Building with --disable-fmp4 fails with compile error → Building with combinations of --{enable,disable}-{fmp4,ffmpeg,eme} fails with compile error
Assignee: nobody → cajbir.bugzilla
Duplicate of this bug: 1148206
Attached patch Fix (obsolete) — Splinter Review
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.
Attachment #8589404 - Flags: review?(ted)
Attachment #8589404 - Flags: review?(cpearce)
Status: NEW → ASSIGNED
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.
Attached patch FixSplinter Review
Address review comment by only compiling the logging stuff if FMP4 is enabled as suggested.

:cpearce for media code, :ted for configure changes.
Attachment #8589404 - Attachment is obsolete: true
Attachment #8589404 - Flags: review?(ted)
Attachment #8589404 - Flags: review?(cpearce)
Attachment #8590024 - Flags: review?(ted)
Attachment #8590024 - Flags: review?(cpearce)
Attachment #8590024 - Flags: review?(cpearce) → review+
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.
Attachment #8590024 - Flags: review?(ted) → review+
(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.
Thanks, I'll have to try swapping my patch for yours in my iOS queue.
https://hg.mozilla.org/mozilla-central/rev/858ea94746ee
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.