Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ffmpeg7: fix compilation for OS X 10.7 through 10.12 #25416

Conversation

erikbs
Copy link
Contributor

@erikbs erikbs commented Aug 22, 2024

Description

Add patch that adds a type alias for OS X <= 10.12 from AVMediaType to NSString* (introduced in the 10.13 SDK, but with an extra specifier). I have submitted the same patch to ffmpeg. Before 10.13 these constants were declared directly as NSString*.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.9.5 13F1911 x86_64
Xcode 6.2 6C131e

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@macportsbot
Copy link

Notifying maintainers:
@barracuda156 for port ffmpeg7.
@mascguy for port ffmpeg7.

@macportsbot macportsbot added type: bugfix maintainer: open Affects an openmaintainer port labels Aug 22, 2024
@jmroot
Copy link
Member

jmroot commented Aug 23, 2024

Please rebase against master in order to pick up c83c328.

@erikbs erikbs force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch 2 times, most recently from 29738ce to 05faf0e Compare August 23, 2024 09:46
@erikbs erikbs force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch 2 times, most recently from 2813e8b to 56893ea Compare September 23, 2024 18:57
@barracuda156
Copy link
Contributor

Did the build fail otherwise? If yes, then no need to revbump it.

@RobK88
Copy link
Contributor

RobK88 commented Sep 27, 2024

Did the build fail otherwise? If yes, then no need to revbump it.

I agree with you. It looks like there is no need to revbump it.

@erikbs erikbs force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch from 56893ea to a88599b Compare September 28, 2024 14:27
@reneeotten
Copy link
Contributor

@erikbs did you submit this patch upstream? If so, please include a link to the issue/PR; if not, please do so we can see what they say about it. Also the latest upstream version is 7.1 please verify that the issue you're trying to solve here hasn't been addressed yet.

@erikbs
Copy link
Contributor Author

erikbs commented Oct 2, 2024

@reneeotten I did and got some feedback (before I opened this PR), but it has not been merged: https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/ . As far as I can see the issue is still there on master.

@RobK88
Copy link
Contributor

RobK88 commented Oct 3, 2024

I recommend that this PR be merged for ffmpeg7 (release 7.01) with a note in the Portfile that the patch can be removed in the future when the patch is incorporated upstream in the source code in a future release of ffmpeg7.

There are ports that depend on ffmpeg7 such as cmus that are currently broken on older macOS systems.

@erikbs erikbs force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch from a88599b to a0955d3 Compare October 3, 2024 16:54
@erikbs
Copy link
Contributor Author

erikbs commented Oct 3, 2024

I recommend that this PR be merged for ffmpeg7 (release 7.01) with a note in the Portfile that the patch can be removed in the future when the patch is incorporated upstream in the source code in a future release of ffmpeg7.

There are ports that depend on ffmpeg7 such as cmus that are currently broken on older macOS systems.

Good idea, I pushed an update with a note added.

@reneeotten
Copy link
Contributor

@erikbs upstream has released version 7.1 - please try the updated version and check if this issues has been resolved.

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

@reneeotten still not fixed in upstream: https://trac.macports.org/ticket/71218

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

So can we merge this? #26404 contains a fix for building on macOS <10.15, but it won't work because timg depends on broken ffmpeg7.

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't look upstream is going to respond anytime soon. I am fine with merging this PR provided that you apply the patch conditionally to only the systems that need it.

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

@erikbs ping

@erikbs erikbs force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch from a0955d3 to f1f70fb Compare November 23, 2024 18:55
@erikbs
Copy link
Contributor Author

erikbs commented Nov 23, 2024

Pushed a new commit:

  • Rebased on master to pick up 7.1.
  • Apply patch only on 10.12 and older.
  • Restored underscore in macro for consistency with existing style.

Tested okay on 10.9.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mascguy mascguy force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch from f1f70fb to 3a354b0 Compare November 29, 2024 15:31
Add typedef for AVMediaType when compiling against older SDK versions
that use NSString* directly.

Fixes: https://trac.macports.org/ticket/70519
@mascguy mascguy force-pushed the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch from 3a354b0 to cbccf1c Compare November 29, 2024 15:35
Copy link
Member

@mascguy mascguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for this contribution!

@mascguy mascguy merged commit c9524df into macports:master Nov 29, 2024
2 of 3 checks passed
@erikbs erikbs deleted the feature/ffmpeg7-fix-compilation-for-osx107-1012 branch November 29, 2024 16:21
@mascguy
Copy link
Member

mascguy commented Nov 29, 2024

While 10.7, 10.8, 10.9, 10.10, and 10.12 are all fixed, 10.11 fails with the following:

libavutil/hwcontext_videotoolbox.c:628:9: error: use of unknown builtin '__builtin_available' [-Wimplicit-function-declaration]
    if (__builtin_available(macOS 10.8, iOS 10, *)) {
        ^
libavutil/hwcontext_videotoolbox.c:628:29: error: use of undeclared identifier 'macOS'
    if (__builtin_available(macOS 10.8, iOS 10, *)) {
                            ^
2 errors generated.

https://build.macports.org/builders/ports-10.11_x86_64-builder/builds/282228/steps/install-port/logs/stdio

Still, this is a big improvement!

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

@mascguy https://trac.macports.org/ticket/64469

patch-avutil-builtin-available.diff should be updated or just blacklist clang < 900.

@mascguy
Copy link
Member

mascguy commented Nov 30, 2024

@mascguy https://trac.macports.org/ticket/64469

patch-avutil-builtin-available.diff should be updated or just blacklist clang < 900.

Just curious, has that been done for ffmoeg-devel?

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

@mascguy nope, it encounter the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer: open Affects an openmaintainer port type: bugfix
Development

Successfully merging this pull request may close these issues.

8 participants