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.

Sign up for GitHub

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

Jump to bottom

Upgrade Multimedia package base on 6.6.52 #2055

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Upgrade Multimedia package base on 6.6.52 #2055

wants to merge 6 commits into from

Conversation

zelan-nxp
Copy link
Contributor

@zelan-nxp zelan-nxp commented Jan 7, 2025 •
edited
Loading

Upstream MM packages:

dsp: 2.1.8 -> 2.1.9
opencl-converter: 0.5.0 ->0.6.0
imx-parser: 4.9.1- >4.9.2
alsa-lib: upstream patch.
imx-vpuwrapper: drop unneeded patch

otavio
otavio approved these changes Jan 7, 2025
View reviewed changes
recipes-multimedia/alsa/alsa-lib_%.bbappend Outdated Show resolved Hide resolved
@otavio
Copy link
Member

otavio commented Jan 7, 2025

I would like to clarify a comment I made earlier. It would be beneficial to improve the comments in each commit log. Specifically, I suggest describing the individual changes made to each recipe within their respective commit logs. This detailed documentation will be useful for referencing the reasons behind each change in the future.

@otavio otavio mentioned this pull request Jan 7, 2025
Closed
18 tasks
@zelan-nxp zelan-nxp force-pushed the multimedia-package-6.6.52 branch from 876ea2c to bf8e076 Compare January 8, 2025 09:49
thochstein
thochstein reviewed Jan 8, 2025
View reviewed changes
recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad_1.24.7.imx.bb Outdated
# it uses nested functions sadly, in ext/wayland/gstwaylandsink.c for GST_ELEMENT_REGISTER_DEFINE
#
TOOLCHAIN = "gcc"

Copy link
Contributor

@thochstein thochstein Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure whether this can be removed. It appears that we have been building without this change all along, which makes me think that we do not build whatever causes the problem. It might be better to not upstream this and instead to align meta-imx with this change.

Copy link
Member

Choose a reason for hiding this comment

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

This is likely to avoid using other toolchain in case it is chosen as default. For example, it is likely to fix a problem if the distribution uses CLang as default toolchain.

Copy link
Contributor

@thochstein thochstein Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, right, so bottom line this is backwards again. @zelan-nxp, please remove this change here and instead add the lines to meta-imx.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zelan-nxp, I tried to clarify my comment. We need to take these lines into meta-imx, not remove them from meta-freescale.

Copy link
Contributor

Choose a reason for hiding this comment

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

@otavio, this item is marked resolved but is not. Can you Unresolve it?

@zelan-nxp zelan-nxp force-pushed the multimedia-package-6.6.52 branch 2 times, most recently from 2049471 to 671393d Compare January 9, 2025 11:11
otavio
otavio reviewed Jan 9, 2025
View reviewed changes
recipes-multimedia/gstreamer/gstreamer1.0-plugins-good_1.24.7.imx.bb Outdated
file://gst/replaygain/rganalysis.c;beginline=1;endline=23;md5=b60ebefd5b2f5a8e0cab6bfee391a5fe \
"
# Enable pulsesink in gstreamer
PACKAGECONFIG:append = "${@bb.utils.contains('DISTRO_FEATURES', 'pulseaudio', 'pulseaudio', '', d)}"
PACKAGECONFIG:append = " pulseaudio"
Copy link
Member

Choose a reason for hiding this comment

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

This is a wrong change because that would cause this feature to be enabled independently if it is supported by the distribution or not.

Copy link
Member

@otavio otavio Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
PACKAGECONFIG:append = " pulseaudio"
PACKAGECONFIG:append = "${@bb.utils.contains('DISTRO_FEATURES', 'pulseaudio', ' pulseaudio', '', d)}"

@zelan-nxp zelan-nxp force-pushed the multimedia-package-6.6.52 branch from 671393d to 73aec8c Compare January 9, 2025 12:39
@otavio otavio self-requested a review January 13, 2025 20:14
@otavio
Copy link
Member

otavio commented Jan 13, 2025

Please rebase this pull request because I made the change to remove the checksum across all the files to a single commit. So you will need also to remove this from this and other pull requests.

@zelan-nxp zelan-nxp force-pushed the multimedia-package-6.6.52 branch from 73aec8c to b45e4fc Compare January 14, 2025 02:17
otavio
otavio reviewed Jan 14, 2025
View reviewed changes
recipes-multimedia/imx-dsp/imx-dsp_2.1.9.bb Outdated

inherit fsl-eula-unpack autotools pkgconfig
inherit fsl-eula-recent autotools pkgconfig fsl-eula2-unpack2
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor

Choose a reason for hiding this comment

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

@zelan-nxp, please don't upstream fsl-eula-recent and fsl-eula2 usage. Update the recipe without these classes. I need to work on it before upstreaming.

@otavio otavio requested review from otavio and thochstein January 14, 2025 11:09
quaresmajose
quaresmajose reviewed Jan 14, 2025
View reviewed changes
recipes-multimedia/gstreamer/gstreamer1.0_1.24.7.imx.bb Outdated

S = "${WORKDIR}/git"

PACKAGECONFIG[tests] = "-Dtests=enabled,-Dtests=disabled"
Copy link
Member

Choose a reason for hiding this comment

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

If the extra patchset for tests is been removed we should also explicitly disable the tests because they won't work.

PACKAGECONFIG[tests] = ""
EXTRA_OEMESON += "-Dtests=disabled"

@zelan-nxp zelan-nxp force-pushed the multimedia-package-6.6.52 branch from b45e4fc to 44d3478 Compare January 15, 2025 11:29
@zelan-nxp
imx-dsp: Update to L6.6.52_2.2.0 release
b285aac 
Bump version 2.1.8 -> 2.1.9

Signed-off-by: Zelan Zou <[email protected]>
@zelan-nxp
imx-opencl-converter: Update to L6.6.52_2.2.0 release
ec9730b 
Bump version 0.5.0 -> 0.6.0

Signed-off-by: Zelan Zou <[email protected]>
@zelan-nxp
imx-parser: Update to L6.6.52_2.2.0 release
7404455 
Bump version 4.9.1 -> 4.9.2

Signed-off-by: Zelan Zou <[email protected]>
@zelan-nxp zelan-nxp force-pushed the multimedia-package-6.6.52 branch from 44d3478 to ee407ee Compare January 15, 2025 11:33
angolini
angolini reviewed Jan 29, 2025
View reviewed changes
recipes-multimedia/alsa/alsa-lib/0001-pcm-dmix-Fix-resume-with-multiple-instances.patch
@@ -0,0 +1,64 @@
From 2adc30e9838daf62312f32a0a93129d64b1668b3 Mon Sep 17 00:00:00 2001
Copy link
Member

Choose a reason for hiding this comment

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

for this commit log I would use:
alsa-lib: Update to L6.6.52-2.2.0 release

While at that, add a fix for resume with multiple instances and remove the overrides for the toolchain as they are not needed any more.

Copy link
Member

Choose a reason for hiding this comment

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

Has this been validated against the master branch? I have the impression that this has been not. I believe this same fix is included in the newest version of the recipe and the build fails because the patch does not apply.

Copy link
Contributor

@thochstein thochstein Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can confirm that the patch does not apply on master. It appears that the source now may contain the idea of the patch, though it is not exact. Shall we drop the patch?

angolini
angolini reviewed Jan 29, 2025
View reviewed changes
recipes-multimedia/alsa/imx-alsa-plugins_git.bb
@@ -20,7 +20,7 @@ inherit autotools pkgconfig use-imx-headers
PV = "1.0.26+${SRCPV}"

SRC_URI = "git://github.com/nxp-imx/imx-alsa-plugins.git;protocol=https;branch=${SRCBRANCH}"
SRCBRANCH = "MM_04.09.01_2408_L6.6.y"
SRCBRANCH = "MM_04.09.02_2410_L6.6.y"
SRCREV = "b2ba082e70333f187972ee4e85f63f9d2f608331"
Copy link
Member

Choose a reason for hiding this comment

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

the branch changes but the SRCREV does not change?

recipes-multimedia/imx-dsp/imx-dsp_2.1.9.bb

DESCRIPTION = "i.MX DSP Wrapper, Firmware Binary, Codec Libraries"
LICENSE = "Proprietary"
LIC_FILES_CHKSUM = "file://COPYING;md5=10c0fda810c63b052409b15a5445671a"

LIC_FILES_CHKSUM = "file://COPYING;md5=ca53281cc0caa7e320d4945a896fb837"
Copy link
Member

Choose a reason for hiding this comment

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

say in the commit log hat the license has been changed

otavio
otavio requested changes Feb 4, 2025
View reviewed changes
recipes-multimedia/alsa/alsa-lib/0001-pcm-dmix-Fix-resume-with-multiple-instances.patch
@@ -0,0 +1,64 @@
From 2adc30e9838daf62312f32a0a93129d64b1668b3 Mon Sep 17 00:00:00 2001
Copy link
Member

Choose a reason for hiding this comment

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

Has this been validated against the master branch? I have the impression that this has been not. I believe this same fix is included in the newest version of the recipe and the build fails because the patch does not apply.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiagofranco hiagofranco hiagofranco left review comments

@quaresmajose quaresmajose quaresmajose left review comments

@angolini angolini angolini left review comments

@otavio otavio otavio requested changes

@thochstein thochstein Awaiting requested review from thochstein

Assignees
No one assigned
Labels
backport scarthgap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zelan-nxp @otavio @angolini @quaresmajose @hiagofranco @thochstein