[meta-xilinx] [metx-xilinx][dev-2017.1] gstreamer1.0-plugins-bad related bbapend file and patches.

[Nathan Rossi]

On 3 February 2017 at 21:39, Dhaval Shah <dhaval.shah at xilinx.com> wrote:
> All the patches from the 2016.4 maintained earlier as locally.
> Now, ported to the 2017.1 and related bbapend file is also added.

Hi Dhaval,

So I am a little confused. Are these patches for meta-xilinx? or are
these internal patches? or something else entirely? My below comments
assume your intention was to have this in meta-xilinx master.

If these are for meta-xilinx are they intended for master? because
oe-core already has gstreamer 1.10.2, which makes a number of the
backported patches unnecessary.

Also "Never" is not a standard Upstream-Status value (I know Manju
said it was in his other email).
http://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines#Patch_Header_Recommendations

However if it is intended to never submit the patches upstream you
will need to provide a reason why, secondly this reason must provide a
good explanation why meta-xilinx should maintain the patches, since by
not upstreaming to gstreamer you are placing the maintenance burden in
meta-xilinx.

Also note, some of the comments for this patch also apply as comments
for your other patch -omx bbappend.

>
> Signed-off-by: Dhaval Shah <dshah at xilinx.com>
> ---
>  ...1-gst-plugins-bad-Copy-kmssink-from-1.9.2.patch | 2550 ++++++++++++++++++++
>  .../0002-Compile-kms.patch                         |   80 +
>  ...03-gst-kmssink-Add-support-for-xilinx-drm.patch |   31 +
>  ...sink-override-stride-if-defined-in-driver.patch |   54 +
>  ...05-kmssink-Fix-selection-of-source-region.patch |   88 +
>  ...-kmssink-Scale-up-to-the-screen-dimension.patch |   31 +
>  .../0007-kmssink-experimentation.patch             |   89 +
>  .../gstreamer/gstreamer1.0-plugins-bad_%.bbappend  |   18 +
>  8 files changed, 2941 insertions(+)
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0001-gst-plugins-bad-Copy-kmssink-from-1.9.2.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0002-Compile-kms.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0003-gst-kmssink-Add-support-for-xilinx-drm.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0004-kmssink-override-stride-if-defined-in-driver.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0005-kmssink-Fix-selection-of-source-region.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0006-kmssink-Scale-up-to-the-screen-dimension.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad/0007-kmssink-experimentation.patch
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad_%.bbappend
>

... snip ...

> +--
> +2.7.4
> +
> diff --git a/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad_%.bbappend b/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad_%.bbappend
> new file mode 100644
> index 0000000..463cebd
> --- /dev/null
> +++ b/recipes-multimedia/gstreamer/gstreamer1.0-plugins-bad_%.bbappend
> @@ -0,0 +1,18 @@
> +PACKAGECONFIG_GL = "${@bb.utils.contains('DISTRO_FEATURES', 'opengl', ' opengl gles2', '', d)}"

Why? also why does it need to be a global change? affecting every
machine/board/distro/etc. (when meta-xilinx is in bblayers)?

Also a bit confused here since ZynqMP does not have opengl hardware
acceleration, only opengl es...?

> +PACKAGECONFIG_append = "faad"

This looks like distro config not bsp config? Also this introduces a
dependency on meta-openembedded since "faad2" is not in oe-core. Why
is this option needed?

> +
> +FILESEXTRAPATHS_prepend := "${THISDIR}/gstreamer1.0-plugins-bad:"
> +
> +#
> +# Need to make this conditional to gstreamer1
> +#
> +SRC_URI_append_zynqmp = " \
> +    file://0001-gst-plugins-bad-Copy-kmssink-from-1.9.2.patch \
> +    file://0002-Compile-kms.patch \
> +    file://0003-gst-kmssink-Add-support-for-xilinx-drm.patch \
> +    file://0004-kmssink-override-stride-if-defined-in-driver.patch \
> +    file://0005-kmssink-Fix-selection-of-source-region.patch \
> +    file://0006-kmssink-Scale-up-to-the-screen-dimension.patch \
> +    file://0007-kmssink-experimentation.patch \
> +"

Since these patches are being applied to a recipe that is not MACHINE
specific, the patches should not be appended as a machine specific
override. If they conflict with the existing source for other targets
then it would be better to provide a additional custom version of the
recipe as opposed to a machine specific version. Or at the least set
the PACKAGE_ARCH to MACHINE_ARCH.

Regards,
Nathan