Closed Bug 1244773 Opened 4 years ago Closed 4 years ago

Fix ffvpx compilation on mingw.


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




Tracking Status
firefox47 --- affected
firefox48 --- fixed


(Reporter: jacek, Assigned: jacek)




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

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, 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)
#define HAVE_MM_EMPTY 0

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