[Buildroot] [PATCH v3] gst1-imx: add package

[Buildroot] [PATCH v3] gst1-imx: add package

[Rostislav Lisovy]

Hello everyone;
Is there any chance the patch (in $subject)
https://www.marc.info/?l=buildroot&m=141550615919434
will be merged into the mainline Buildroot?

After changing the GST1_IMX_VERSION to the newest 0.10.1
everything compiles fine (the patch applies to the master as well).

Best regards;
Rostislav Lisovy

[Buildroot] [PATCH v3] gst1-imx: add package

[Luca Ceresoli]

Dear Rostislav,

Rostislav Lisovy wrote:
> Hello everyone;
> Is there any chance the patch (in $subject)
> https://www.marc.info/?l=buildroot&m=141550615919434
> will be merged into the mainline Buildroot?
>
> After changing the GST1_IMX_VERSION to the newest 0.10.1
> everything compiles fine (the patch applies to the master as well).

I agree it should be merged. Or rejected, it there's a reason to do so.

Did you run-test it? Please specify what you tested, and formally add
your Tested-by tag. This makes it more likely for a patch to be merged.
See the manual [1] for details about reviewers tags.

Then Gary or you could resend the patch with the version updated.

[1] 
http://buildroot.net/downloads/manual/manual.html#_reviewing_and_testing_patches

-- 
Luca

[Buildroot] [PATCH v3] gst1-imx: add package

[Eric Nelson]

Hi Luca,

On 03/18/2015 03:07 AM, Luca Ceresoli wrote:
> Dear Rostislav,
> 
> Rostislav Lisovy wrote:
>> Hello everyone;
>> Is there any chance the patch (in $subject)
>> https://www.marc.info/?l=buildroot&m=141550615919434
>> will be merged into the mainline Buildroot?
>>
>> After changing the GST1_IMX_VERSION to the newest 0.10.1
>> everything compiles fine (the patch applies to the master as well).
> 
> I agree it should be merged. Or rejected, it there's a reason to do so.
> 
> Did you run-test it? Please specify what you tested, and formally add
> your Tested-by tag. This makes it more likely for a patch to be merged.
> See the manual [1] for details about reviewers tags.
> 
> Then Gary or you could resend the patch with the version updated.
> 

I believe Gary is out of pocket this week.

Regards,


Eric

[Buildroot] [PATCH v3] gst1-imx: add package

[Gary Bisson]

Luca, Rostislav, All,

On 03/18/2015 11:07 AM, Luca Ceresoli wrote:
> Dear Rostislav,
>
> Rostislav Lisovy wrote:
>> Hello everyone;
>> Is there any chance the patch (in $subject)
>> https://www.marc.info/?l=buildroot&m=141550615919434
>> will be merged into the mainline Buildroot?
>>
>> After changing the GST1_IMX_VERSION to the newest 0.10.1
>> everything compiles fine (the patch applies to the master as well).
>
> I agree it should be merged. Or rejected, it there's a reason to do so.
>
> Did you run-test it? Please specify what you tested, and formally add
> your Tested-by tag. This makes it more likely for a patch to be merged.
> See the manual [1] for details about reviewers tags.
>
> Then Gary or you could resend the patch with the version updated.
>
> [1]
> http://buildroot.net/downloads/manual/manual.html#_reviewing_and_testing_patches

I will resend the updated patch today.

Thanks,
Gary

[Buildroot] [PATCH v3] gst1-imx: add package

[Gary Bisson]

Open-source GStreamer 1.0 plugins for i.MX6 platforms.
More info at https://github.com/Freescale/gstreamer-imx

Signed-off-by: Gary Bisson <bisson.gary@gmail.com>
---
Modifications v2->v3:
- Change package name from gst1-plugins-imx to gst1-imx
- Remove commands unnecessary parentheses
- Modify package comment to clarify it is only working on i.MX6

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

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-imx/Config.in   | 25 ++++++++++++++++
 package/gstreamer1/gst1-imx/gst1-imx.mk | 52 +++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+)
 create mode 100644 package/gstreamer1/gst1-imx/Config.in
 create mode 100644 package/gstreamer1/gst1-imx/gst1-imx.mk

diff --git a/package/gstreamer1/Config.in b/package/gstreamer1/Config.in
index 5f08faf..ea35ecc 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-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-imx/Config.in b/package/gstreamer1/gst1-imx/Config.in
new file mode 100644
index 0000000..6bdbb6c
--- /dev/null
+++ b/package/gstreamer1/gst1-imx/Config.in
@@ -0,0 +1,25 @@
+comment "gst1-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-imx needs an (e)glibc toolchain"
+	depends on BR2_arm
+	depends on !BR2_TOOLCHAIN_USES_GLIBC
+
+config BR2_PACKAGE_GST1_IMX
+	bool "gst1-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.MX6 platforms, with emphasis on video en/decoding using the VPU
+	  engine.
+
+	  This software supports only 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.
diff --git a/package/gstreamer1/gst1-imx/gst1-imx.mk b/package/gstreamer1/gst1-imx/gst1-imx.mk
new file mode 100644
index 0000000..4a3b5a2
--- /dev/null
+++ b/package/gstreamer1/gst1-imx/gst1-imx.mk
@@ -0,0 +1,52 @@
+################################################################################
+#
+# gst1-imx
+#
+################################################################################
+
+GST1_IMX_VERSION = 0.9.9
+GST1_IMX_SITE = $(call github,Freescale,gstreamer-imx,$(GST1_IMX_VERSION))
+
+GST1_IMX_LICENSE = LGPLv2+
+GST1_IMX_LICENSE_FILES = LICENSE
+
+GST1_IMX_INSTALL_STAGING = YES
+
+GST1_IMX_DEPENDENCIES += host-pkgconf host-python \
+	gpu-viv-bin-mx6q gstreamer1 gst1-plugins-base libfslvpuwrap
+
+# needs access to imx-specific kernel headers
+GST1_IMX_DEPENDENCIES += linux
+GST1_IMX_CONF_OPTS += --prefix="/usr" \
+	--kernel-headers="$(LINUX_DIR)/include"
+
+ifeq ($(BR2_PACKAGE_XLIB_LIBX11),y)
+GST1_IMX_DEPENDENCIES += xlib_libX11
+GST1_IMX_CONF_OPTS += --egl-platform=x11
+else
+ifeq ($(BR2_PACKAGE_WAYLAND),y)
+GST1_IMX_DEPENDENCIES += wayland
+GST1_IMX_CONF_OPTS += --egl-platform=wayland
+else
+GST1_IMX_CONF_OPTS += --egl-platform=fb
+endif
+endif
+
+define GST1_IMX_CONFIGURE_CMDS
+	cd $(@D); \
+		$(TARGET_CONFIGURE_OPTS) \
+		$(HOST_DIR)/usr/bin/python2 ./waf configure $(GST1_IMX_CONF_OPTS)
+endef
+
+define GST1_IMX_BUILD_CMDS
+	cd $(@D); \
+		$(HOST_DIR)/usr/bin/python2 ./waf build -j $(PARALLEL_JOBS)
+endef
+
+define GST1_IMX_INSTALL_TARGET_CMDS
+	cd $(@D); \
+		$(HOST_DIR)/usr/bin/python2 ./waf --destdir=$(TARGET_DIR) \
+		install
+endef
+
+$(eval $(generic-package))
-- 
2.1.1

[Buildroot] [PATCH v3] gst1-imx: add package

[Luca Ceresoli]

Dear Gary,

Gary Bisson wrote:
> Open-source GStreamer 1.0 plugins for i.MX6 platforms.
> More info at https://github.com/Freescale/gstreamer-imx
>
> Signed-off-by: Gary Bisson <bisson.gary@gmail.com>
> ---
> Modifications v2->v3:
> - Change package name from gst1-plugins-imx to gst1-imx
> - Remove commands unnecessary parentheses
> - Modify package comment to clarify it is only working on i.MX6
>
> 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
>
> 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

I tested it on i.MX6DL SABRESD (not Quad), based on
freescale_imx6dlsabresd_defconfig, but with libc changed to glibc.
All the three pipelines you provided work, with the first one eating up
~30% of CPU time, while the other two ones take only 2~3% CPU only (but
play no audio of course).

A couple of comments below.

> diff --git a/package/gstreamer1/gst1-imx/Config.in b/package/gstreamer1/gst1-imx/Config.in
> new file mode 100644
> index 0000000..6bdbb6c
> --- /dev/null
> +++ b/package/gstreamer1/gst1-imx/Config.in
> @@ -0,0 +1,25 @@
> +comment "gst1-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-imx needs an (e)glibc toolchain"
> +	depends on BR2_arm
> +	depends on !BR2_TOOLCHAIN_USES_GLIBC

Sorry for coming back on the comments issue, but...

I simulated a newbie wanting to use gst1-imx:
  - make freescale_imx6dlsabresd_defconfig
  - make menuconfig
  - go to target packages -> audio/video, look for gst1
    -> "gstreamer 1.x needs a toolchain w/ wchar, threads" is shown
  - change toolchain settings to enable wchar
  - go to target packages -> audio/video, enable gst1
    -> "gst1-imx needs an (e)glibc toolchain" is shown
  - change toolchain settings to use glibc
  - go back to target packages -> audio/video
    -> gst1-imx disappeared, no comments shown
       -> user panic!

Well, I don't remember exactly how we came to the conclusion that some
of the comments should be removed, but I think they should have not.

The BR2_PACKAGE_LIBFSLVPUWRAP and BR2_PACKAGE_GPU_VIV_BIN_MX6Q
dependencies are software packages, not architecture features, so they
are user-selectable and thus should show a comment when not satisfied.

However, to me this patch is an improvement and could be merged as-is,
and the comments re-added later if discussions on this point does not
settle quickly. Gary seems very responsive and willing to have a good
support for this package, so I guess he will keep on.

> +
> +config BR2_PACKAGE_GST1_IMX
> +	bool "gst1-imx"
> +	depends on BR2_LINUX_KERNEL

As per Arnout's suggestion on v2, there should be a # libfslvpuwrap
here.

With that fixed:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

-- 
Luca

[Buildroot] [PATCH v3] gst1-imx: add package

[Gary Bisson]

Luca, All,

On 11/10/2014 09:31 AM, Luca Ceresoli wrote:
> Dear Gary,
>
> Gary Bisson wrote:
>> [snip]
>> $ 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
>
> I tested it on i.MX6DL SABRESD (not Quad), based on
> freescale_imx6dlsabresd_defconfig, but with libc changed to glibc.
> All the three pipelines you provided work, with the first one eating up
> ~30% of CPU time, while the other two ones take only 2~3% CPU only (but
> play no audio of course).

Good to hear everything worked out. Thanks for checking CPU consumption, 
although 30% seems high for audio handling we can clearly see the video 
is decoded through the VPU.

> [snip]
> Sorry for coming back on the comments issue, but...
>
> I simulated a newbie wanting to use gst1-imx:
>  - make freescale_imx6dlsabresd_defconfig
>  - make menuconfig
>  - go to target packages -> audio/video, look for gst1
>    -> "gstreamer 1.x needs a toolchain w/ wchar, threads" is shown
>  - change toolchain settings to enable wchar
>  - go to target packages -> audio/video, enable gst1
>    -> "gst1-imx needs an (e)glibc toolchain" is shown
>  - change toolchain settings to use glibc
>  - go back to target packages -> audio/video
>    -> gst1-imx disappeared, no comments shown
>       -> user panic!
>
> Well, I don't remember exactly how we came to the conclusion that some
> of the comments should be removed, but I think they should have not.
>
> The BR2_PACKAGE_LIBFSLVPUWRAP and BR2_PACKAGE_GPU_VIV_BIN_MX6Q
> dependencies are software packages, not architecture features, so they
> are user-selectable and thus should show a comment when not satisfied.

Yes that is exactly why I've put comments in the first place. Yann 
wasn't a huge fan of that solution and I understand it [1]. The best 
would be to find another approach to be able to do the select.

> However, to me this patch is an improvement and could be merged as-is,
> and the comments re-added later if discussions on this point does not
> settle quickly. Gary seems very responsive and willing to have a good
> support for this package, so I guess he will keep on.
>
>> +
>> +config BR2_PACKAGE_GST1_IMX
>> +    bool "gst1-imx"
>> +    depends on BR2_LINUX_KERNEL
>
> As per Arnout's suggestion on v2, there should be a # libfslvpuwrap
> here.

Actually I think this should not be added as this package also needs the 
kernel headers for IPU use (see source code under src/ipu).

> With that fixed:
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

Thanks,
Gary

[1] http://lists.busybox.net/pipermail/buildroot/2014-October/109264.html

[Buildroot] [PATCH v3] gst1-imx: add package

[Luca Ceresoli]

Dear Gary,

Gary Bisson wrote:
> Luca, All,
>
> On 11/10/2014 09:31 AM, Luca Ceresoli wrote:
>> Dear Gary,
>>
>> Gary Bisson wrote:
>>> [snip]
>>> $ 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
>>
>> I tested it on i.MX6DL SABRESD (not Quad), based on
>> freescale_imx6dlsabresd_defconfig, but with libc changed to glibc.
>> All the three pipelines you provided work, with the first one eating up
>> ~30% of CPU time, while the other two ones take only 2~3% CPU only (but
>> play no audio of course).
>
> Good to hear everything worked out. Thanks for checking CPU consumption,
> although 30% seems high for audio handling we can clearly see the video
> is decoded through the VPU.

Isn't the first pipeline simply not using the hardware decoder?

>>> +
>>> +config BR2_PACKAGE_GST1_IMX
>>> +    bool "gst1-imx"
>>> +    depends on BR2_LINUX_KERNEL
>>
>> As per Arnout's suggestion on v2, there should be a # libfslvpuwrap
>> here.
>
> Actually I think this should not be added as this package also needs the
> kernel headers for IPU use (see source code under src/ipu).

I see. Thanks for the clarification.

-- 
Luca

[Buildroot] [PATCH v3] gst1-imx: add package

[Gary Bisson]

Luca, All,

On Tue, Nov 11, 2014 at 1:14 AM, Luca Ceresoli <luca@lucaceresoli.net> wrote:
> Dear Gary,
>
> Gary Bisson wrote:
>>
>> Luca, All,
>>
>> On 11/10/2014 09:31 AM, Luca Ceresoli wrote:
>>>
>>> Dear Gary,
>>>
>>> Gary Bisson wrote:
>>>>
>>>> [snip]
>>>> $ 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
>>>
>>>
>>> I tested it on i.MX6DL SABRESD (not Quad), based on
>>> freescale_imx6dlsabresd_defconfig, but with libc changed to glibc.
>>> All the three pipelines you provided work, with the first one eating up
>>> ~30% of CPU time, while the other two ones take only 2~3% CPU only (but
>>> play no audio of course).
>>
>>
>> Good to hear everything worked out. Thanks for checking CPU consumption,
>> although 30% seems high for audio handling we can clearly see the video
>> is decoded through the VPU.
>
>
> Isn't the first pipeline simply not using the hardware decoder?
>

No, decoding both 1080p video plus audio in software would be using
even more CPU. Plus the traces suggest the use of imxvpudec.

>>>> +
>>>> +config BR2_PACKAGE_GST1_IMX
>>>> +    bool "gst1-imx"
>>>> +    depends on BR2_LINUX_KERNEL
>>>
>>>
>>> As per Arnout's suggestion on v2, there should be a # libfslvpuwrap
>>> here.
>>
>>
>> Actually I think this should not be added as this package also needs the
>> kernel headers for IPU use (see source code under src/ipu).
>
>
> I see. Thanks for the clarification.
>

Let me know if there's any modification you'd like on this patch.

Thanks,
Gary

[Buildroot] [PATCH v3] gst1-imx: add package

[Luca Ceresoli]

Dear Gary, All,

Gary Bisson wrote:
> Let me know if there's any modification you'd like on this patch.

I haven't (that's why I gave you my Reviewed-by and Acked-by tags).

As I wrote in my first reply, I am not that happy with the absence of
kconfig comments about missing dependencies. Nevertheless I would
commit this patch, because it is an improvement anyway. Comments may
be added later if desired.

Yann, you commented on this topic in past submissions of this patch,
are you satisfied with the current approach?

Now that the stabilization month is over, hopefully this patch will
get merged. BTW, it cleanly applies on current master.

-- 
Luca