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

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
mozilla40
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox40 fixed, firefox-esr38 affected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Fix
1.64 KB, patch
cpearce
: review+
ted
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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}.
(Assignee)

Comment 2

3 years ago
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.
(Assignee)

Updated

3 years ago
Summary: Building with --disable-fmp4 fails with compile error → Building with combinations of --{enable,disable}-{fmp4,ffmpeg,eme} fails with compile error
(Assignee)

Updated

3 years ago
Assignee: nobody → cajbir.bugzilla
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1148206
(Assignee)

Comment 4

3 years ago
Created attachment 8589404 [details] [diff] [review]
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.
Attachment #8589404 - Flags: review?(ted)
Attachment #8589404 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8590024 [details] [diff] [review]
Fix

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

Comment 11

3 years ago
(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
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

2 years ago
status-firefox-esr38: --- → affected
You need to log in before you can comment on or make changes to this bug.