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

Prepare qt 6 recipe for FFMPEG transition to QtMultimedia #256

Merged
merged 24 commits into from
May 28, 2024

Conversation

mmcauliffe
Copy link

@mmcauliffe mmcauliffe commented Apr 8, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

As of Qt 6.7, the default backend for Qt Multimedia has switched to ffmpeg rather than the native backends: https://doc.qt.io/qt-6/qtmultimedia-index.html#target-platform-and-backend-notes

I've seen some issues with gstreamer on osx_arm64, so switching to ffmpeg should resolve those as well.

hmaarrfk's comment:

This modification has evolved toward allowing ffmpeg support for qt6. However, due to the large number of dependencies that FFMPEG pulls in, we are deciding to split this off as a separate package. For Qt 6.7.1, the qt6-main package will continue to provide gstreamer support. ffmpeg support is provided by the qt6-multimedia package.

For future package, no multimedia support will be provided through the qt6-main package and end users will have to use the qt6-multimedia package to get video and audio support.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@mmcauliffe
Copy link
Author

Looks like OSX_64 is failing due to https://github.com/conda-forge/ffmpeg-feedstock/blob/main/recipe/build.sh#L92

@mmcauliffe
Copy link
Author

@conda-forge-admin, please restart ci

@mmcauliffe
Copy link
Author

@hmaarrfk thanks for merging the videotoolbox changes for ffmpeg! Hoping that should be the only change necessary to get OSX builds working, but it looks like linux_aarch64 is consistently failing for extracting the qt-everywhere tarball due to lack of space. Is this a transient known issue or is there some configuration that needs to get changed because ffmpeg and dependencies are larger than gstreamer and dependencies?

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Apr 9, 2024

it should be pretty transient for qt6. tbh, i've never seen it before. Lets try again after this fails.

recipe/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: Mark Harfouche <mark.harfouche@gmail.com>
@hmaarrfk
Copy link
Contributor

hmaarrfk commented Apr 9, 2024

The other option which I was considering. Was to build qtmultimwdia as a separate package for these very reasons.

It would be "easy" from a packaging standpoint to do in 6.7

@mmcauliffe
Copy link
Author

Hm, yeah it still is having space issues. What all would be involved in having a separate package for QtMultimedia? Would that be a separate feedstock?

As a simpler fix, I can add back in the gstreamer backend for linux_aarch64 since that worked for it and should let the rest be on ffmpeg?

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Apr 9, 2024

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Apr 9, 2024

As a simpler fix, I can add back in the gstreamer backend for linux_aarch64 since that worked for it and should let the rest be on ffmpeg?

this won't work for 6.7 so not so fun.

@mmcauliffe
Copy link
Author

As a simpler fix, I can add back in the gstreamer backend for linux_aarch64 since that worked for it and should let the rest be on ffmpeg?

this won't work for 6.7 so not so fun.

It won't? It does say that the native support is still there, just limited: https://doc.qt.io/qt-6/qtmultimedia-index.html#native-backends, I think in the future it's likely that they'll drop it though.

@Tobias-Fischer
Copy link
Contributor

Have you tried to set free_disk_space: True (https://conda-forge.org/docs/maintainer/conda_forge_yml/#azure)? I'm not sure if it affects linux-aarch64 as well, but might be worth giving it a shot.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

recipe/meta.yaml Outdated Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor

Fingers crossed this passes.

@@ -22,8 +22,6 @@ cxx_compiler_version:
- '16'
glib:
- '2'
gstreamer:
- '1.24'
Copy link
Contributor

Choose a reason for hiding this comment

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

its strange that this getting deleted. is gstreamer no longer getting pinned?

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure that the selectors using the version are at fault here:

    - gstreamer                          # [unix and version == "6.7.1"]
    - pulseaudio                         # [linux and version == "6.7.1"]

This feedstock only builds one version at a time. Why are we trying to filter dependencies on the version at all?

In any case, if you want to keep that filter, maybe you'll have more success with:

    {% if version == "6.7.1" %}
    - gstreamer                          # [unix]
    - pulseaudio                         # [linux]
    {% endif %}

Copy link
Author

Choose a reason for hiding this comment

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

I'll try rerendering here but, maybe it's because of the version check with:

    - gst-plugins-base  {{ gstreamer }}  # [unix and version == "6.7.1"]
    # gstreamer plugin not supported on win32: https://bugreports.qt.io/browse/QTBUG-107073
    - gstreamer                          # [unix and version == "6.7.1"]

Is there a way to pin this with a conditional on version?

@mmcauliffe
Copy link
Author

@conda-forge-admin please rerender

Copy link

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/qt-main-feedstock/actions/runs/9201282415.

recipe/bld.bat Outdated
@@ -47,6 +50,7 @@ cmake -LAH -G "Ninja" ^
-DFEATURE_system_sqlite=ON ^
-DFEATURE_quick3d_assimp=OFF ^
-DFEATURE_vulkan=ON ^
-DQT_MEDIA_BACKEND=ffmpeg ^
Copy link
Contributor

Choose a reason for hiding this comment

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

is ffmpeg being depended on here?

@hmaarrfk
Copy link
Contributor

@conda-forge-admin please rerender

recipe/bld.bat Outdated
Comment on lines 9 to 11
IF %PKG_VERSION%=="6.7.1" (
set "MODS=%MODS%;qtmultimedia"
)
Copy link
Contributor

@hmaarrfk hmaarrfk May 24, 2024

Choose a reason for hiding this comment

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

~~this logic doesn't match up with your tests below for windows. ~~

I think just delete the windows changes.

For windows users that want multi-media, they can install the qt6-multimedia package you made, and they will be off to the transition a version early.

Edit: I checked the logs of the last completed build, qt6multimedia.dll doesn't exist.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes there was a bug here, should be

IF "%PKG_VERSION%"=="6.7.1" (
   set "MODS=%MODS%;qtmultimedia"
)

For the conditional to check out. I can fix that and add tests for Qt6Multimedia.dll existence in 6.7.1, but if you think it's better to rely on conda-forge/staged-recipes#25992 for windows, I'm ok with that too

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its better to "not add any features" here and just to leave it to qt6-multimedia if we can.

recipe/bld.bat Outdated Show resolved Hide resolved
recipe/bld.bat Outdated Show resolved Hide resolved
@hmaarrfk hmaarrfk changed the title Use ffmpeg backend for QtMultimedia Prepare qt 6 recipe for FFMPEG transition to QtMultimedia May 24, 2024
@hmaarrfk
Copy link
Contributor

I'm not going to press the green button myself for a few days since I feel like I had too much of a heavy hand in the design of this recipe.

But i'm very eager to get this in. Thank you @mmcauliffe for persevering!

@mmcauliffe
Copy link
Author

Sounds good, thanks @hmaarrfk! I do have the one outstanding question here: #256 (comment). In terms of the timing of merging this vs conda-forge/staged-recipes#25992, is the qt-multimedia recipe dependent on the changes here or can we get that merged in before this? There's some post-merged work for setting up the cross compiled builds for qt-multimedia and I want to also start testing out the ffmpeg backend with my projects.

@hmaarrfk
Copy link
Contributor

is the qt-multimedia recipe dependent on the changes here or can we get that merged in before this?

I think it is now pretty independent, becaues we mostly made this recipe "remove gstreamer after 6.7.1"

There's some post-merged work for setting up the cross compiled builds for qt-multimedia and I want to also start testing out the ffmpeg backend with my projects.

are you on linux? you could build that package locally. and upload to your own channel during this time. it will "help" you prepare and test and maybe even find compatibility bugs that could change the my thoughts on this intermediate package.

@hmaarrfk hmaarrfk merged commit 3f1830c into conda-forge:qt6 May 28, 2024
7 checks passed
@hmaarrfk
Copy link
Contributor

Alright here goes, thanks for the patience.

hmaarrfk added a commit to hmaarrfk/admin-requests that referenced this pull request May 29, 2024
As part of a refactor of the recipe we were planning on removing
qtmultimedia from the recipe in 6.7.2 but accidentally did so in 6.7.1
build 1

conda-forge/qt-main-feedstock#256

xref: conda-forge/qt-main-feedstock#265

This is causing issues for downstream recipes such as pyside6
conda-forge/pyside2-feedstock#228

And thus likely causing issues for a subset of users

The recipe is being fixed at the moment and a test has been added to
ensure an other library isn't missed in the process
conda-forge/qt-main-feedstock#264
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants