Closed Bug 1244773 Opened 4 years ago Closed 4 years ago

Fix ffvpx compilation on mingw.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jacek, Assigned: jacek)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch fix (obsolete) — Splinter Review
Building ffvpx on mingw is well supported, but we can't directly use msvc config files. We could add separated ones, but given that it requires just minor changes, I just tweaked existing ones and added its changes to changes.diff.
Attachment #8714349 - Flags: review?(ajones)
Comment on attachment 8714349 [details] [diff] [review]
fix

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

Fly by review as I've done for FreeBSD. 

These files are auto generated. Having to manually edit them makes future resync difficult. 

Changes.patch is the patch that was used to generate the ffmpeg source changes. So it doesn't apply here. 

Maintaining a new set of config files is likely not ideal, so maybe having its own patch/script that from the original self-generated config file generate this config_win*.h; with updated documentation in the README on how to run it. 

Thank you.
Attachment #8714349 - Flags: review?(ajones) → review-
Attached patch fix v2 (obsolete) — Splinter Review
I'm sorry it took so long. The attached version adds new config files that are generated from MSVC config files and instructions in README how to regenerate them.
Attachment #8714349 - Attachment is obsolete: true
Attachment #8740366 - Flags: review?(ajones)
Comment on attachment 8740366 [details] [diff] [review]
fix v2

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

Jean-Yves needs to review this.
Attachment #8740366 - Flags: review?(ajones) → review?(jyavenard)
Comment on attachment 8740366 [details] [diff] [review]
fix v2

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

I don't think we want yet another config file that no one will be able to update when needed because it's not really a supported config

Please take the same approach as I described in https://bugzilla.mozilla.org/show_bug.cgi?id=1239550#c13, that is

have your stuff in config.h
and add at the end something like:
#if defined(XP_WIN) && !defined(_MSC_VER)
#if !defined(HAVE_64BIT_BUILD)
#undef HAVE_MM_EMPTY
#define HAVE_MM_EMPTY 0
#endif
#undef HAVE_LIBC_MSVCRT
#define HAVE_LIBC_MSVCRT 0
#endif

as really, that's all you need. much simpler and easier to maintain.
Attachment #8740366 - Flags: review?(jyavenard) → review-
Attached patch fix v3Splinter Review
Thank you for the review. I attached a fix that you described, it's indeed much cleaner.
Attachment #8740366 - Attachment is obsolete: true
Attachment #8744583 - Flags: review?(jyavenard)
Attachment #8744583 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/5680a55b2ec1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.