-
Notifications
You must be signed in to change notification settings - Fork 169
gstreamer1.0-plugins-base: Add bbappend to apply Qualcomm patches #1349
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
base: master
Are you sure you want to change the base?
gstreamer1.0-plugins-base: Add bbappend to apply Qualcomm patches #1349
Conversation
|
I'm sorry, this is a nice hack, but no. Please follow the usual process: add the patches to the layer repo instead of storing them in the external repo. Contribute all the patches to the upstream project, UBWC compression should be supported upstream, it doesn't differ any other compression schemes. |
|
@lumag As canonical suggesting hosting in separate repo instead of putting patches along with distribution and we would like to maintain patches in one central repo for all distributions and our team had initial discussion with Nico and explained the reason to host in Github @ndechesne could you please comment your feedback here. |
|
There are complications in upstreaming patches related to the UBWC (Universal Bandwidth Compression) format. Since UBWC is a proprietary format, we cannot provide the pack/unpack logic in open source. The upstream community does not accept a packed format without corresponding pack/unpack implementations. We are actively collaborating with the community to find a way forward, but this will take time to reach alignment. |
|
hi. this is indeed aligned with what we discussed (privately), but let's discuss that here now. I proposed to manage gstreamer in a different way because it has , I think, different challenges.
I understand it is not the standard way to host patches for OE recipes, but it's a practical solution to help us overall. |
| SRCREV_qti_patches = "${AUTOREV}" | ||
|
|
||
| qcom_do_patch() { | ||
| PATCHREPO="${WORKDIR}/sources/patchrepo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start as do_patch_
Can you name this more appropriately in the purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed dependency on quilt and added the raw URIs directly in the SRC_URI.
Comment not applicable now.
| esac | ||
|
|
||
| if [ -f "${PATCHREPO}/${p}" ]; then | ||
| quilt import "${PATCHREPO}/${p}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to depend on quit-native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, Yocto uses quilt as the default PATCHTOOL.
Since this function runs inside do_patch:append, the environment already ensures quilt-native is available.
Therefore, we do not need to explicitly add DEPENDS += "quilt-native" in the .bbappend file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course you need to do it. If something is changed in OE-Core then your recipe gets broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed dependency on quilt and added the raw URIs directly in the SRC_URI.
Comment not applicable now.
| git://github.com/Raja-Ganapathi-Busam/gst-qti-oss-patches.git;protocol=ssh;user=git;branch=${PV};name=qti_patches;destsuffix=patchrepo;subpath=${BPN} \ | ||
| " | ||
|
|
||
| SRCREV_qti_patches = "${AUTOREV}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no AUTOREV, we only rely on fixed content/metadata in meta-qcom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our intention is to avoid making changes in meta-qcom repo if there is any update in patches repo.
Please suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no other way around. You need to make changes to meta-qcom when you change the version of the software we build in meta-qcom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 here. If you make changes to your repo, it is a different version, a different recipe revision and a different set of packages. The item will go away once you drop the custom do_patch implementation and use OE's one (together with using raw URIs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed dependency on quilt and added the raw URIs directly in the SRC_URI.
| @@ -0,0 +1,37 @@ | |||
| # To apply patches to support Qualcomm specific formats and fixes | |||
| SRC_URI:append:qcom = " \ | |||
| git://github.com/Raja-Ganapathi-Busam/gst-qti-oss-patches.git;protocol=ssh;user=git;branch=${PV};name=qti_patches;destsuffix=patchrepo;subpath=${BPN} \ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the patches are available in a public http link, perhaps we could list them here directly. that might be a better approach that strictly depending on an external git tree.
a link like this one (this is just an example)
At least you wouldn't need to have a custom do_patch implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in process of making git://github.com/qualcomm/gst-qti-oss-patches public. Meanwhile, we would like to have a review of our approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in the process of making the patches repository public.
Once it becomes publicly accessible, I will update the link accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, and I made the review in this context. I am not saying that the link is not public, but suggesting a different approach. Instead of downloading the whole tree and manually applying the patches yourself in the qcom_do_patch() function, you can list all the patches as http:// links SRC_URI.
That way, you still preserver your tree with standalone patches which is convenient for what we discussed, and we still integrate patches 'normally' (almost) from an OE metadata perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, and I made the review in this context. I am not saying that the link is not public, but suggesting a different approach. Instead of downloading the whole tree and manually applying the patches yourself in the qcom_do_patch() function, you can list all the patches as http:// links SRC_URI.
That way, you still preserver your tree with standalone patches which is convenient for what we discussed, and we still integrate patches 'normally' (almost) from an OE metadata perspective
I think that is also what as suggested by me in the meetings before the solstice break. Having a custom do_patch is very fragile and rarely required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still point out the raw URIs inside your repo (and change them later to the official one once it's public).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested, removed dependency on quilt and added the raw URIs directly in the SRC_URI.
|
Okay.
This will not work in a long term. Ubuntu, Debian, Fedora and OE will have different versions of the GStreamer. At some point your patches won't be unuquelly applicable to those versions, so you will have to have separate branches or revisions. Having that in mind it is much easier to incorporate those patches into the distro package as it is usually done. I definitely don't expect Debian / Ubuntu packages fetching additional repo during the build stage.
UBWC format has been reverse engineered. There was another study of the compressed formats, which I can find quickly at this moment. As such, I can only recommend actually defining the compress and decompress functions and following Gstreamer requirements. Other projects would also benefit from it. We have hard time testing the UBWC support on the display side because there is no defined way to create or validate generated UBWC images. |
recipes-multimedia/gstreamer/gstreamer1.0-plugins-base_%.bbappend
Outdated
Show resolved
Hide resolved
Fetched additional patches from external Git repository using raw URL Signed-off-by: Raja Ganapathi Busam <rbusam@qti.qualcomm.com>
615af4c to
51801cf
Compare
In addition to this, since we now have meta-qcom running on YP autobuilder, every gstreamer version update will break master-next build. |
I forgot we are building base image. It won't break on every update but will fail to parse if bbappend is version specific or if we add an image build that includes gstreamer there. |
|
When we branch off QLI 2.x then both Yocto and Ubuntu uses same version of GStreamer in my opinion. So, instead of maintaining patches in multiple places, we would like to maintain in one place. Also, I got to know that Canonical does not want attach patches in distribution and instead they asking us to host patches in a repo. |
There is no guarantee that Yocto and Ubuntu will use the same version of Gstreamer. It might happen, but maybe not. |
|
@ndechesne Got it. Can you please suggest whether we can go ahead with HTTP URI for the patch location and we will apply patches for other GStreamer packages as well in similar way. |
yes, that's the only way to get them, I think. @anujm1 brought up a good point though. now that we committed to supporting Meta-qcom in upstream Yocto CI, making such unexpected/bad gstreamer tweaks which might break Yocto CI is problematic.. but that's a different problem, we might need to find a way to limit when we apply the patches. |
| @@ -0,0 +1,15 @@ | |||
| # To apply patches to support Qualcomm specific formats and fixes | |||
|
|
|||
| PATCH_BASE = "https://raw.githubusercontent.com/Raja-Ganapathi-Busam/gst-qti-oss-patches/refs/heads/${PV}/${BPN}" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you open the tree so that we can have a look? at least share with the meta-qcom maintainers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndechesne we have shared patches via email as patches repo still not public due to legal review
|
|
||
| PATCH_BASE = "https://raw.githubusercontent.com/Raja-Ganapathi-Busam/gst-qti-oss-patches/refs/heads/${PV}/${BPN}" | ||
|
|
||
| SRC_URI:append:qcom = " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it is more work on your side to manage the patches, but it really look much better than the first patch, thanks!
| SRC_URI[q08c_patch.sha256sum] = "7455df8252fc1655184a4659bdc8ce1479595ed3bd788578c3e909582af04803" | ||
| SRC_URI[q10c_patch.sha256sum] = "2d7f448abc9da7843959b193861d1266a11a71365908900d7b4a31d8e20c19e6" | ||
| SRC_URI[vmeta_patch.sha256sum] = "76ba2ae3e40d1cbb016561ad7fb8027cacd120f67a6fde4c2a8d992be3ae27ba" | ||
| SRC_URI[vrate_patch.sha256sum] = "393cdd2ede68080db6e6f969373358d66dc92e6025cddc4307595aca650d6ef5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't play well with the Yocto Compatibility requirements. While we probably can come up with a different scheme, I would ask to vendor in all patches here rather than pulling them from the external repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can observe the issue as a failure of the yocto-run-checks step.
| @@ -0,0 +1,15 @@ | |||
| # To apply patches to support Qualcomm specific formats and fixes | |||
|
|
|||
| PATCH_BASE = "https://raw.githubusercontent.com/Raja-Ganapathi-Busam/gst-qti-oss-patches/refs/heads/${PV}/${BPN}" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still allows updating the repo in a way that the patches will be changed, breaking the builds. The names of the patches don't incorporate version of the recipe (nor the revision of the patch), so when the patch is updated in the repo, it will not be redownloaded, triggering the checksum errors for the builders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is always a problem. if 'upstream' changes the tarball we use, it breaks the build.. here if we do crazy things on gst-qti-oss-patches and rewrite commits, then yes, it's bad and it will break things. i don't see how it's different here. we have checksums for the files, so we check for such issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difference between checking for upstream misbehaviour and knowingly introducing a change that will report an error once the repo gets updated.
Also, mainline (master here) will continue, and it is expected that the versions will differ. You should not make any version assumption here as the decision is up for the distros. |
I don't really like this approach, so NAK from my side, there is nothing special here and the patches should be maintained as part of this layer, so we can also enforce a proper level of quality there. Also, we do want to allow any developer to fix these patches once there is a version update in oe-core that could potentially break them, hosting externally will cause additional friction. |
ricardosalveti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAK from my side, extra patches should be hosted and maintained as part of this repository, so we can review the quality of the patches and also support accepting fixes that could be proposed by anyone.
Summary
This PR adds
bbappendfile to thegstreamer1.0-plugins-baserecipe to append additional patches from an external series file after the base recipe patches. The patches are imported usingquiltin the correct order and applied after existing patches.Key Changes
seriesfile from external Git repository#in the series file (treated as comments).Temporary Setup
There is a separate pull request raised on the patches repository which is pending review and merge.
Until that PR is merged,
SRC_URIin this recipe points to my forked repository to fetch the patches.Merge Condition
Please merge this PR only after the patches PR is merged in the patches repository,
gst-qti-oss-patches, to ensure consistency.Testing
seriesfile.#) in the series file, if any, are skipped.