| Message ID | 1414636344-8152-1-git-send-email-bisson.gary@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Gary, thanks for you resubmission. Gary Bisson wrote: > Open-source GStreamer 1.0 plugins for i.MX platforms. > More info at https://github.com/Freescale/gstreamer-imx > > Signed-off-by: Gary Bisson <bisson.gary@gmail.com> > --- > Modifications v1->v2: > - Use of github helper macro > - Modify package comment with Peter S. original patch > - Add comment for eglibc dependency (due to GPU libs) > - Remove comments on package dependencies > > One thing I wasn't sure about, should all new package contain a .hash file? If > so I can submit a v3 right away. It is not mandatory. AFAIK the hash is recommended for security-related packages only. [...] > diff --git a/package/gstreamer1/gst1-plugins-imx/Config.in b/package/gstreamer1/gst1-plugins-imx/Config.in > new file mode 100644 > index 0000000..d499874 > --- /dev/null > +++ b/package/gstreamer1/gst1-plugins-imx/Config.in > @@ -0,0 +1,28 @@ > +comment "gst1-plugins-imx needs an imx-specific Linux kernel to be built" > + depends on BR2_arm && !BR2_LINUX_KERNEL > + > +# Required by gpu-viv-bin-mx6q > +comment "gst1-plugins-imx needs an (e)glibc toolchain" > + depends on BR2_arm > + depends on !BR2_TOOLCHAIN_USES_GLIBC These messages are much cleaner than in version 1 of your patch. However, for an ARM configuration with a musl or uClibc toolchain and without a kernel build activated, two lines are displayed. This is not coherent with the general Buildroot behavior. It should be: comment "gst1-plugins-imx needs an (e)glibc toolchain and an imx-specific Linux kernel to be built" depends on BR2_arm && (!BR2_LINUX_KERNEL || !BR2_TOOLCHAIN_USES_GLIBC) Although that's a bit lengthy... With that fixed (which can be done by the committer): Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> I'll try to test it during the coming week and give my Tested-by tag, or report any issues.
On 02/11/14 23:13, Luca Ceresoli wrote: > Dear Gary, > > thanks for you resubmission. > > Gary Bisson wrote: >> Open-source GStreamer 1.0 plugins for i.MX platforms. >> More info at https://github.com/Freescale/gstreamer-imx >> >> Signed-off-by: Gary Bisson <bisson.gary@gmail.com> >> --- >> Modifications v1->v2: >> - Use of github helper macro >> - Modify package comment with Peter S. original patch >> - Add comment for eglibc dependency (due to GPU libs) >> - Remove comments on package dependencies >> >> One thing I wasn't sure about, should all new package contain a .hash file? If >> so I can submit a v3 right away. > > It is not mandatory. AFAIK the hash is recommended for security-related > packages only. We discussed it last BR meeting but didn't come to a real conclusion. For sure, the hash file isn't mandatory. > > [...] >> diff --git a/package/gstreamer1/gst1-plugins-imx/Config.in >> b/package/gstreamer1/gst1-plugins-imx/Config.in >> new file mode 100644 >> index 0000000..d499874 >> --- /dev/null >> +++ b/package/gstreamer1/gst1-plugins-imx/Config.in >> @@ -0,0 +1,28 @@ >> +comment "gst1-plugins-imx needs an imx-specific Linux kernel to be built" >> + depends on BR2_arm && !BR2_LINUX_KERNEL >> + >> +# Required by gpu-viv-bin-mx6q >> +comment "gst1-plugins-imx needs an (e)glibc toolchain" >> + depends on BR2_arm >> + depends on !BR2_TOOLCHAIN_USES_GLIBC > > These messages are much cleaner than in version 1 of your patch. > However, for an ARM configuration with a musl or uClibc toolchain and > without a kernel build activated, two lines are displayed. This is not > coherent with the general Buildroot behavior. Well, there are a few other packages like that. Because... > > It should be: > > comment "gst1-plugins-imx needs an (e)glibc toolchain and an imx-specific Linux > kernel to be built" ... this line is really too long. Regards, Arnout > depends on BR2_arm && (!BR2_LINUX_KERNEL || !BR2_TOOLCHAIN_USES_GLIBC) > > Although that's a bit lengthy... > > With that fixed (which can be done by the committer): > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > I'll try to test it during the coming week and give my Tested-by tag, or > report any issues. >
Hi Gary, Thanks for your continued effort on this package. I still have some feedback, however. On 30/10/14 03:32, Gary Bisson wrote: > Open-source GStreamer 1.0 plugins for i.MX platforms. > More info at https://github.com/Freescale/gstreamer-imx > > Signed-off-by: Gary Bisson <bisson.gary@gmail.com> > --- > Modifications v1->v2: > - Use of github helper macro > - Modify package comment with Peter S. original patch > - Add comment for eglibc dependency (due to GPU libs) > - Remove comments on package dependencies > > One thing I wasn't sure about, should all new package contain a .hash file? If > so I can submit a v3 right away. > > This patch has been tested using an i.MX6Q SabreLite (nitrogen6x config) along > with the usual Tears of Steel movie in 1080p: > http://media.xiph.org/mango/tears_of_steel_1080p.webm > > Below are the commands used to test the different sinks: > > $ gst-launch-1.0 playbin uri=file:///root/tears_of_steel_1080p.webm > $ gst-launch-1.0 filesrc location=/root/tears_of_steel_1080p.webm ! \ > matroskademux ! imxvpudec ! imxipusink > $ gst-launch-1.0 filesrc location=/root/tears_of_steel_1080p.webm ! \ > matroskademux ! imxvpudec ! imxeglvivsink > > The toolchain used was the Sourcery CodeBench 2014.05. The video above also > requires the following extra configuration in order to demux the content: > BR2_PACKAGE_GST1_PLUGINS_GOOD=y > BR2_PACKAGE_GST1_PLUGINS_GOOD_PLUGIN_MATROSKA=y > > Thanks, > Gary > > --- > package/gstreamer1/Config.in | 1 + > package/gstreamer1/gst1-plugins-imx/Config.in | 28 +++++++++++ > .../gst1-plugins-imx/gst1-plugins-imx.mk | 56 ++++++++++++++++++++++ > 3 files changed, 85 insertions(+) > create mode 100644 package/gstreamer1/gst1-plugins-imx/Config.in > create mode 100644 package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk > > diff --git a/package/gstreamer1/Config.in b/package/gstreamer1/Config.in > index 5f08faf..9dbbed4 100644 > --- a/package/gstreamer1/Config.in > +++ b/package/gstreamer1/Config.in > @@ -6,6 +6,7 @@ source "package/gstreamer1/gst1-plugins-base/Config.in" > source "package/gstreamer1/gst1-plugins-good/Config.in" > source "package/gstreamer1/gst1-plugins-bad/Config.in" > source "package/gstreamer1/gst1-plugins-ugly/Config.in" > +source "package/gstreamer1/gst1-plugins-imx/Config.in" I'm sorry to return to the naming issue, but shouldn't the package be called gst1-imx instead of gst1-plugins-imx? We don't use 'plugins' in any other gst package (except the official plugin bundles), and upstream also doesn't have 'plugins' in the name. > source "package/gstreamer1/gst1-libav/Config.in" > source "package/gstreamer1/gst1-validate/Config.in" > source "package/gstreamer1/gst-omx/Config.in" > diff --git a/package/gstreamer1/gst1-plugins-imx/Config.in b/package/gstreamer1/gst1-plugins-imx/Config.in > new file mode 100644 > index 0000000..d499874 > --- /dev/null > +++ b/package/gstreamer1/gst1-plugins-imx/Config.in > @@ -0,0 +1,28 @@ > +comment "gst1-plugins-imx needs an imx-specific Linux kernel to be built" > + depends on BR2_arm && !BR2_LINUX_KERNEL > + > +# Required by gpu-viv-bin-mx6q > +comment "gst1-plugins-imx needs an (e)glibc toolchain" > + depends on BR2_arm > + depends on !BR2_TOOLCHAIN_USES_GLIBC > + > +config BR2_PACKAGE_GST1_PLUGINS_IMX > + bool "gst1-plugins-imx" > + depends on BR2_LINUX_KERNEL Put here a # libfslvpuwrap to make the reason explicit. > + depends on BR2_arm # Only relevant for i.MX > + depends on BR2_TOOLCHAIN_USES_GLIBC # gpu-viv-bin-mx6q > + depends on BR2_PACKAGE_GPU_VIV_BIN_MX6Q This should be a select. > + depends on BR2_PACKAGE_LIBFSLVPUWRAP This should be a select. > + select BR2_PACKAGE_GST1_PLUGINS_BASE > + help > + This is a set of GStreamer 1.0 plugins for plugins for Freescale's > + i.MX platform, with emphasis on video en/decoding using the i.MX > + VPU engine. > + > + Currently, this software has been tested only with the i.MX6 SoC > + family and requires a kernel that includes the i.MX6 specific > + headers to be built. As far as I understand, it's _only_ for i.MX6 and i.MX7, not for the older ones. So perhaps that should be clarified. > + > + The software as a whole is currently in beta stage. > + > + More info at https://github.com/Freescale/gstreamer-imx Leave out the 'More info at'. > diff --git a/package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk b/package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk > new file mode 100644 > index 0000000..56b6fdf > --- /dev/null > +++ b/package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk > @@ -0,0 +1,56 @@ > +################################################################################ > +# > +# gst1-plugins-imx > +# > +################################################################################ > + > +GST1_PLUGINS_IMX_VERSION = 0.9.9 > +GST1_PLUGINS_IMX_SITE = $(call github,Freescale,gstreamer-imx,$(GST1_PLUGINS_IMX_VERSION)) > + > +GST1_PLUGINS_IMX_LICENSE = LGPLv2+ > +GST1_PLUGINS_IMX_LICENSE_FILES = LICENSE > + > +GST1_PLUGINS_IMX_INSTALL_STAGING = YES > + > +GST1_PLUGINS_IMX_DEPENDENCIES += host-pkgconf host-python \ > + gpu-viv-bin-mx6q gstreamer1 gst1-plugins-base libfslvpuwrap > + > +# needs access to imx-specific kernel headers > +GST1_PLUGINS_IMX_DEPENDENCIES += linux > +GST1_PLUGINS_IMX_CONF_OPTS += --prefix="/usr" \ > + --kernel-headers="$(LINUX_DIR)/include" > + > +ifeq ($(BR2_PACKAGE_XLIB_LIBX11),y) > +GST1_PLUGINS_IMX_DEPENDENCIES += xlib_libX11 > +GST1_PLUGINS_IMX_CONF_OPTS += --egl-platform=x11 > +else > +ifeq ($(BR2_PACKAGE_WAYLAND),y) > +GST1_PLUGINS_IMX_DEPENDENCIES += wayland > +GST1_PLUGINS_IMX_CONF_OPTS += --egl-platform=wayland > +else > +GST1_PLUGINS_IMX_CONF_OPTS += --egl-platform=fb > +endif > +endif > + > +define GST1_PLUGINS_IMX_CONFIGURE_CMDS > + (cd $(@D); \ > + $(TARGET_CONFIGURE_OPTS) \ > + $(HOST_DIR)/usr/bin/python2 ./waf configure \ > + $(GST1_PLUGINS_IMX_CONF_OPTS) \ > + ) I realize that this is probably just copy-paste from somewhere else, but there is no need at all for the ()s. It just forks another shell with no good reason. Regards, Arnout > +endef > + > +define GST1_PLUGINS_IMX_BUILD_CMDS > + (cd $(@D); \ > + $(HOST_DIR)/usr/bin/python2 ./waf build -j $(PARALLEL_JOBS) \ > + ) > +endef > + > +define GST1_PLUGINS_IMX_INSTALL_TARGET_CMDS > + (cd $(@D); \ > + $(HOST_DIR)/usr/bin/python2 ./waf --destdir=$(TARGET_DIR) \ > + install \ > + ) > +endef > + > +$(eval $(generic-package)) >
Arnout, Luca, All, On Mon, Nov 3, 2014 at 1:51 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > Hi Gary, > > Thanks for your continued effort on this package. I still have some > feedback, > however. No problem ;-) > [snip] > > source "package/gstreamer1/gst1-plugins-good/Config.in" > > source "package/gstreamer1/gst1-plugins-bad/Config.in" > > source "package/gstreamer1/gst1-plugins-ugly/Config.in" > > +source "package/gstreamer1/gst1-plugins-imx/Config.in" > > I'm sorry to return to the naming issue, but shouldn't the package be > called > gst1-imx instead of gst1-plugins-imx? We don't use 'plugins' in any other > gst > package (except the official plugin bundles), and upstream also doesn't > have > 'plugins' in the name. I see your point and really changing the name doesn't bother me, we just need to agree on something. How do we organize a vote? irc? ;-) So far Peter K suggested "gstreamer1-imx", Arnout "gst1-imx" and mine "gst1-plugins-imx". As a reminder the git repo name is "gstreamer-imx" and the Yocto recipe is "gstreamer1.0-plugins-imx". Since the first version of this patch have been integrated two plugins:gst1-libav and gst1-validate which tends towards a gst1-<name> convention. > [snip] > > +config BR2_PACKAGE_GST1_PLUGINS_IMX > > + bool "gst1-plugins-imx" > > + depends on BR2_LINUX_KERNEL > > Put here a # libfslvpuwrap to make the reason explicit. Ok > > + depends on BR2_arm # Only relevant for i.MX > > + depends on BR2_TOOLCHAIN_USES_GLIBC # gpu-viv-bin-mx6q > > + depends on BR2_PACKAGE_GPU_VIV_BIN_MX6Q > > This should be a select. > We unfortunately discussed that before and Yann came to the conclusion it wasn't possible for now [1]. Otherwise this requires reworking all the iMX platforms selections in freescale-imx packages. > + depends on BR2_PACKAGE_LIBFSLVPUWRAP > > This should be a select. > As the previous one must be a "depends", this one has to be the same. Otherwise it gives a "recursive dependency detected!" error: package/freescale-imx/gpu-viv-bin-mx6q/Config.in:5: symbol BR2_PACKAGE_GPU_VIV_BIN_MX6Q depends on BR2_PACKAGE_FREESCALE_IMX package/freescale-imx/Config.in:1: symbol BR2_PACKAGE_FREESCALE_IMX is selected by BR2_PACKAGE_LIBFSLVPUWRAP package/libfslvpuwrap/Config.in:5: symbol BR2_PACKAGE_LIBFSLVPUWRAP is selected by BR2_PACKAGE_GST1_PLUGINS_IMX package/gstreamer1/gst1-plugins-imx/Config.in:9: symbol BR2_PACKAGE_GST1_PLUGINS_IMX depends on BR2_PACKAGE_GPU_VIV_BIN_MX6Q > > + select BR2_PACKAGE_GST1_PLUGINS_BASE > > + help > > + This is a set of GStreamer 1.0 plugins for plugins for > Freescale's > > + i.MX platform, with emphasis on video en/decoding using the i.MX > > + VPU engine. > > + > > + Currently, this software has been tested only with the i.MX6 SoC > > + family and requires a kernel that includes the i.MX6 specific > > + headers to be built. > > As far as I understand, it's _only_ for i.MX6 and i.MX7, not for the older > ones. So perhaps that should be clarified. > Ok. This came from the Git repo readme, I will clarify on iMX6 only but I'd rather not state iMX7 not for people to be confused. > + > > + The software as a whole is currently in beta stage. > > + > > + More info at https://github.com/Freescale/gstreamer-imx > > Leave out the 'More info at'. Ok. [snip] > > +define GST1_PLUGINS_IMX_CONFIGURE_CMDS > > + (cd $(@D); \ > > + $(TARGET_CONFIGURE_OPTS) \ > > + $(HOST_DIR)/usr/bin/python2 ./waf configure \ > > + $(GST1_PLUGINS_IMX_CONF_OPTS) \ > > + ) > > I realize that this is probably just copy-paste from somewhere else, but > there > is no need at all for the ()s. It just forks another shell with no good > reason. Ok. Once we all agree on the naming + dependency issue I will submit a v3. Thanks, Gary [1] http://lists.busybox.net/pipermail/buildroot/2014-October/109264.html
>>>>> "Gary" == Gary Bisson <bisson.gary@gmail.com> writes: Hi, > I'm sorry to return to the naming issue, but shouldn't the package be > called > gst1-imx instead of gst1-plugins-imx? We don't use 'plugins' in any other > gst > package (except the official plugin bundles), and upstream also doesn't > have > 'plugins' in the name. > > I see your point and really changing the name doesn't bother me, we just need > to agree on something. How do we organize a vote? irc? ;-) So far Peter K > suggested "gstreamer1-imx", Arnout "gst1-imx" and mine "gst1-plugins-imx". As a > reminder the git repo name is "gstreamer-imx" and the Yocto recipe is > "gstreamer1.0-plugins-imx". Since the first version of this patch have been > integrated two plugins:gst1-libav and gst1-validate which tends towards a gst1- > <name> convention. Yes, so lets go with gst1-imx. >> + depends on BR2_arm # Only relevant for i.MX >> + depends on BR2_TOOLCHAIN_USES_GLIBC # gpu-viv-bin-mx6q >> + depends on BR2_PACKAGE_GPU_VIV_BIN_MX6Q > This should be a select. > We unfortunately discussed that before and Yann came to the conclusion it > wasn't possible for now [1]. Otherwise this requires reworking all the iMX > platforms selections in freescale-imx packages. Ahh yes, so lets leave it as depends on (for both).
diff --git a/package/gstreamer1/Config.in b/package/gstreamer1/Config.in index 5f08faf..9dbbed4 100644 --- a/package/gstreamer1/Config.in +++ b/package/gstreamer1/Config.in @@ -6,6 +6,7 @@ source "package/gstreamer1/gst1-plugins-base/Config.in" source "package/gstreamer1/gst1-plugins-good/Config.in" source "package/gstreamer1/gst1-plugins-bad/Config.in" source "package/gstreamer1/gst1-plugins-ugly/Config.in" +source "package/gstreamer1/gst1-plugins-imx/Config.in" source "package/gstreamer1/gst1-libav/Config.in" source "package/gstreamer1/gst1-validate/Config.in" source "package/gstreamer1/gst-omx/Config.in" diff --git a/package/gstreamer1/gst1-plugins-imx/Config.in b/package/gstreamer1/gst1-plugins-imx/Config.in new file mode 100644 index 0000000..d499874 --- /dev/null +++ b/package/gstreamer1/gst1-plugins-imx/Config.in @@ -0,0 +1,28 @@ +comment "gst1-plugins-imx needs an imx-specific Linux kernel to be built" + depends on BR2_arm && !BR2_LINUX_KERNEL + +# Required by gpu-viv-bin-mx6q +comment "gst1-plugins-imx needs an (e)glibc toolchain" + depends on BR2_arm + depends on !BR2_TOOLCHAIN_USES_GLIBC + +config BR2_PACKAGE_GST1_PLUGINS_IMX + bool "gst1-plugins-imx" + depends on BR2_LINUX_KERNEL + depends on BR2_arm # Only relevant for i.MX + depends on BR2_TOOLCHAIN_USES_GLIBC # gpu-viv-bin-mx6q + depends on BR2_PACKAGE_GPU_VIV_BIN_MX6Q + depends on BR2_PACKAGE_LIBFSLVPUWRAP + select BR2_PACKAGE_GST1_PLUGINS_BASE + help + This is a set of GStreamer 1.0 plugins for plugins for Freescale's + i.MX platform, with emphasis on video en/decoding using the i.MX + VPU engine. + + Currently, this software has been tested only with the i.MX6 SoC + family and requires a kernel that includes the i.MX6 specific + headers to be built. + + The software as a whole is currently in beta stage. + + More info at https://github.com/Freescale/gstreamer-imx diff --git a/package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk b/package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk new file mode 100644 index 0000000..56b6fdf --- /dev/null +++ b/package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk @@ -0,0 +1,56 @@ +################################################################################ +# +# gst1-plugins-imx +# +################################################################################ + +GST1_PLUGINS_IMX_VERSION = 0.9.9 +GST1_PLUGINS_IMX_SITE = $(call github,Freescale,gstreamer-imx,$(GST1_PLUGINS_IMX_VERSION)) + +GST1_PLUGINS_IMX_LICENSE = LGPLv2+ +GST1_PLUGINS_IMX_LICENSE_FILES = LICENSE + +GST1_PLUGINS_IMX_INSTALL_STAGING = YES + +GST1_PLUGINS_IMX_DEPENDENCIES += host-pkgconf host-python \ + gpu-viv-bin-mx6q gstreamer1 gst1-plugins-base libfslvpuwrap + +# needs access to imx-specific kernel headers +GST1_PLUGINS_IMX_DEPENDENCIES += linux +GST1_PLUGINS_IMX_CONF_OPTS += --prefix="/usr" \ + --kernel-headers="$(LINUX_DIR)/include" + +ifeq ($(BR2_PACKAGE_XLIB_LIBX11),y) +GST1_PLUGINS_IMX_DEPENDENCIES += xlib_libX11 +GST1_PLUGINS_IMX_CONF_OPTS += --egl-platform=x11 +else +ifeq ($(BR2_PACKAGE_WAYLAND),y) +GST1_PLUGINS_IMX_DEPENDENCIES += wayland +GST1_PLUGINS_IMX_CONF_OPTS += --egl-platform=wayland +else +GST1_PLUGINS_IMX_CONF_OPTS += --egl-platform=fb +endif +endif + +define GST1_PLUGINS_IMX_CONFIGURE_CMDS + (cd $(@D); \ + $(TARGET_CONFIGURE_OPTS) \ + $(HOST_DIR)/usr/bin/python2 ./waf configure \ + $(GST1_PLUGINS_IMX_CONF_OPTS) \ + ) +endef + +define GST1_PLUGINS_IMX_BUILD_CMDS + (cd $(@D); \ + $(HOST_DIR)/usr/bin/python2 ./waf build -j $(PARALLEL_JOBS) \ + ) +endef + +define GST1_PLUGINS_IMX_INSTALL_TARGET_CMDS + (cd $(@D); \ + $(HOST_DIR)/usr/bin/python2 ./waf --destdir=$(TARGET_DIR) \ + install \ + ) +endef + +$(eval $(generic-package))
Open-source GStreamer 1.0 plugins for i.MX platforms. More info at https://github.com/Freescale/gstreamer-imx Signed-off-by: Gary Bisson <bisson.gary@gmail.com> --- Modifications v1->v2: - Use of github helper macro - Modify package comment with Peter S. original patch - Add comment for eglibc dependency (due to GPU libs) - Remove comments on package dependencies One thing I wasn't sure about, should all new package contain a .hash file? If so I can submit a v3 right away. This patch has been tested using an i.MX6Q SabreLite (nitrogen6x config) along with the usual Tears of Steel movie in 1080p: http://media.xiph.org/mango/tears_of_steel_1080p.webm Below are the commands used to test the different sinks: $ gst-launch-1.0 playbin uri=file:///root/tears_of_steel_1080p.webm $ gst-launch-1.0 filesrc location=/root/tears_of_steel_1080p.webm ! \ matroskademux ! imxvpudec ! imxipusink $ gst-launch-1.0 filesrc location=/root/tears_of_steel_1080p.webm ! \ matroskademux ! imxvpudec ! imxeglvivsink The toolchain used was the Sourcery CodeBench 2014.05. The video above also requires the following extra configuration in order to demux the content: BR2_PACKAGE_GST1_PLUGINS_GOOD=y BR2_PACKAGE_GST1_PLUGINS_GOOD_PLUGIN_MATROSKA=y Thanks, Gary --- package/gstreamer1/Config.in | 1 + package/gstreamer1/gst1-plugins-imx/Config.in | 28 +++++++++++ .../gst1-plugins-imx/gst1-plugins-imx.mk | 56 ++++++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 package/gstreamer1/gst1-plugins-imx/Config.in create mode 100644 package/gstreamer1/gst1-plugins-imx/gst1-plugins-imx.mk