Re: [meta-freescale] [PATCH] gstreamer1.0-plugins-base: set default videosink for IMXQM/IMXQXP

Hello Ming,

On Tue, Mar 14, 2023 at 9:56=E2=80=AFAM Ming Liu <[email protected]> wro=
te:
>
> From: Ming Liu <[email protected]>
>
> The default 'autovideosink' gives very bad performance when playing
> H264/H265 videos on IMXQM/IMXQXP machines. Let's choose
> 'imxvideoconvert_g2d ! autovideosink' as the default videosink, so the
> end users dont need manually pass 'imxvideoconvert_g2d' pipeline to
> gst-play-1.0.
>
> Signed-off-by: Ming Liu <[email protected]>
> ---
>  ...e-default-videosink-autosink-options.patch | 106 ++++++++++++++++++
>  .../gstreamer1.0-plugins-base_1.20.3.imx.bb   |  14 ++-
>  2 files changed, 118 insertions(+), 2 deletions(-)
>  create mode 100644 recipes-multimedia/gstreamer/gstreamer1.0-plugins-bas=
e/0001-Introduce-default-videosink-autosink-options.patch
>
> diff --git a/recipes-multimedia/gstreamer/gstreamer1.0-plugins-base/0001-=
Introduce-default-videosink-autosink-options.patch b/recipes-multimedia/gst=
reamer/gstreamer1.0-plugins-base/0001-Introduce-default-videosink-autosink-=
options.patch
> new file mode 100644
> index 00000000..9ad0385d
> --- /dev/null
> +++ b/recipes-multimedia/gstreamer/gstreamer1.0-plugins-base/0001-Introdu=
ce-default-videosink-autosink-options.patch
> @@ -0,0 +1,106 @@
> +From c929a3fd4a10a2d5f57933b40447aa76ce0b6c5a Mon Sep 17 00:00:00 2001
> +From: Ming Liu <[email protected]>
> +Date: Sun, 5 Mar 2023 13:52:05 +0100
> +Subject: [PATCH] Introduce default videosink/autosink options
> +
> +So the end users can choose the default videosink/autosink at build
> +time. It now supports pipeline as default videosink/autosink.
> +
> +Upstream-Status: Pending [https://github.com/nxp-imx/gst-plugins-base/pu=
ll/2]

I'm sorry, but this not the upstream URL this patch refers to.
GStreamer upstream development happens here [1], and what the patch is
pointing to is (as the description suggests) NXP fork of the upstream
GStreamer repository.

I do not think that the 'Upstream-Status:' tag can be used here like
this, as it shall be rather:
'Upstream-Status: Inappropriate [nxp-imx specific]'

> +
> +Signed-off-by: Ming Liu <[email protected]>
> +---
> + gst/playback/gstplaysink.c | 10 ++++++++--
> + meson.build                |  5 ++---
> + meson_options.txt          |  5 +++++
> + tools/gst-play.c           |  7 +++++++
> + 4 files changed, 22 insertions(+), 5 deletions(-)
> +
> +diff --git a/gst/playback/gstplaysink.c b/gst/playback/gstplaysink.c
> +index dc8bfdacd..1abb4ac7e 100755
> +--- a/gst/playback/gstplaysink.c
> ++++ b/gst/playback/gstplaysink.c
> +@@ -1770,7 +1770,10 @@ gen_video_chain (GstPlaySink * playsink, gboolean=
 raw, gboolean async)
> +       /* if default sink from config.h is different then try it too */
> +       if (strcmp (DEFAULT_VIDEOSINK, "autovideosink")) {
> +         GST_DEBUG_OBJECT (playsink, "trying " DEFAULT_VIDEOSINK);
> +-        elem =3D gst_element_factory_make (DEFAULT_VIDEOSINK, "videosin=
k");
> ++        if (strchr (DEFAULT_VIDEOSINK, ' ') !=3D NULL)
> ++            elem =3D gst_parse_bin_from_description (DEFAULT_VIDEOSINK,=
 TRUE, NULL);
> ++        else
> ++            elem =3D gst_element_factory_make (DEFAULT_VIDEOSINK, "vide=
osink");
> +         chain->sink =3D try_element (playsink, elem, TRUE);
> +       }
> +     }
> +@@ -2713,7 +2716,10 @@ gen_audio_chain (GstPlaySink * playsink, gboolean=
 raw)
> +       /* if default sink from config.h is different then try it too */
> +       if (strcmp (DEFAULT_AUDIOSINK, "autoaudiosink")) {
> +         GST_DEBUG_OBJECT (playsink, "trying " DEFAULT_AUDIOSINK);
> +-        elem =3D gst_element_factory_make (DEFAULT_AUDIOSINK, "audiosin=
k");
> ++        if (strchr (DEFAULT_AUDIOSINK, ' ') !=3D NULL)
> ++            elem =3D gst_parse_bin_from_description (DEFAULT_AUDIOSINK,=
 TRUE, NULL);
> ++        else
> ++            elem =3D gst_element_factory_make (DEFAULT_AUDIOSINK, "audi=
osink");
> +         chain->sink =3D try_element (playsink, elem, TRUE);
> +       }
> +     }
> +diff --git a/meson.build b/meson.build
> +index 5ad1ab3ad..a6280eccd 100644
> +--- a/meson.build
> ++++ b/meson.build
> +@@ -280,9 +280,8 @@ endif
> + core_conf.set_quoted('GST_PACKAGE_NAME', gst_package_name)
> + core_conf.set_quoted('GST_PACKAGE_ORIGIN', get_option('package-origin')=
)
> +
> +-# FIXME: These should be configure options
> +-core_conf.set_quoted('DEFAULT_VIDEOSINK', 'autovideosink')
> +-core_conf.set_quoted('DEFAULT_AUDIOSINK', 'autoaudiosink')
> ++core_conf.set_quoted('DEFAULT_VIDEOSINK', get_option('defaultvideosink'=
))
> ++core_conf.set_quoted('DEFAULT_AUDIOSINK', get_option('defaultaudiosink'=
))
> +
> + # Set whether the audioresampling method should be detected at runtime
> + core_conf.set('AUDIORESAMPLE_FORMAT_' + get_option('audioresample_forma=
t').to_upper(), true)
> +diff --git a/meson_options.txt b/meson_options.txt
> +index 7010b9162..49bd1a728 100644
> +--- a/meson_options.txt
> ++++ b/meson_options.txt
> +@@ -21,6 +21,11 @@ option('opengl_module_name', type : 'string', value :=
 '',
> + option('gles2_module_name', type : 'string', value : '',
> +        description : 'The file to pass to g_module_open to open the lib=
GLESv2 library (default: libGLESv2)')
> +
> ++option('defaultaudiosink', type : 'string', value : 'autoaudiosink',
> ++       description : 'The default audiosink')
> ++option('defaultvideosink', type : 'string', value : 'autovideosink',
> ++       description : 'The default videosink')
> ++
> + # Feature option for opengl plugin and integration library
> + option('gl', type : 'feature', value : 'auto', description : 'OpenGL in=
tegration library and OpenGL plugin')
> + option('gl-graphene', type : 'feature', value : 'auto', description : '=
Use Graphene in OpenGL plugin')
> +diff --git a/tools/gst-play.c b/tools/gst-play.c
> +index 0e858d282..4c1e92e42 100644
> +--- a/tools/gst-play.c
> ++++ b/tools/gst-play.c
> +@@ -205,6 +205,7 @@ play_new (gchar ** uris, const gchar * audio_sink, c=
onst gchar * video_sink,
> +     else
> +       g_warning ("Couldn't create specified audio sink '%s'", audio_sin=
k);
> +   }
> ++
> +   if (video_sink !=3D NULL) {
> +     if (strchr (video_sink, ' ') !=3D NULL)
> +       sink =3D gst_parse_bin_from_description (video_sink, TRUE, NULL);
> +@@ -1674,6 +1675,12 @@ main (int argc, char **argv)
> +     playlist_file =3D NULL;
> +   }
> +
> ++  if (audio_sink =3D=3D NULL)
> ++    audio_sink =3D g_strdup(DEFAULT_AUDIOSINK);
> ++
> ++  if (video_sink =3D=3D NULL)
> ++    video_sink =3D g_strdup(DEFAULT_VIDEOSINK);
> ++
> +   if (playlist->len =3D=3D 0 && (filenames =3D=3D NULL || *filenames =
=3D=3D NULL)) {
> +     gst_printerr (_("Usage: %s FILE1|URI1 [FILE2|URI2] [FILE3|URI3] ...=
"),
> +         "gst-play-" GST_API_VERSION);
> +--
> +2.25.1
> +
> diff --git a/recipes-multimedia/gstreamer/gstreamer1.0-plugins-base_1.20.=
3.imx.bb b/recipes-multimedia/gstreamer/gstreamer1.0-plugins-base_1.20.3.im=
x.bb
> index 0ef9daea..b3da7438 100644
> --- a/recipes-multimedia/gstreamer/gstreamer1.0-plugins-base_1.20.3.imx.b=
b
> +++ b/recipes-multimedia/gstreamer/gstreamer1.0-plugins-base_1.20.3.imx.b=
b
> @@ -121,7 +121,9 @@ SRC_URI:remove =3D " \
>      file://0003-viv-fb-Make-sure-config.h-is-included.patch \
>      file://0002-ssaparse-enhance-SSA-text-lines-parsing.patch"
>  SRC_URI:prepend =3D "${GST1.0-PLUGINS-BASE_SRC};branch=3D${SRCBRANCH} "
> -SRC_URI +=3D "file://0001-gstallocatorphymem.c-Typecast-result-of-gst_ph=
ymem_g.patch"
> +SRC_URI:append =3D " \
> +    file://0001-gstallocatorphymem.c-Typecast-result-of-gst_phymem_g.pat=
ch \
> +    file://0001-Introduce-default-videosink-autosink-options.patch"
>  GST1.0-PLUGINS-BASE_SRC ?=3D "gitsm://github.com/nxp-imx/gst-plugins-bas=
e.git;protocol=3Dhttps"
>  SRCBRANCH =3D "MM_04.07.02_2210_L5.15.y"
>  SRCREV =3D "cbf542ce3e0bad1009d5ecf72707e870c375c3f0"
> @@ -130,6 +132,9 @@ S =3D "${WORKDIR}/git"
>
>  inherit use-imx-headers
>
> +DEFAULT_AUDIOSINK ?=3D "autoaudiosink"
> +DEFAULT_VIDEOSINK ?=3D "autovideosink"
> +
>  PACKAGECONFIG_GL:imxgpu2d =3D \
>      "${@bb.utils.contains('DISTRO_FEATURES', 'opengl x11', 'opengl viv-f=
b', '', d)}"
>  PACKAGECONFIG_GL:imxgpu3d =3D \
> @@ -144,7 +149,12 @@ PACKAGECONFIG:append:imxgpu2d =3D " g2d"
>  PACKAGECONFIG[g2d] =3D ",,virtual/libg2d"
>  PACKAGECONFIG[viv-fb] =3D ",,virtual/libgles2"
>
> -EXTRA_OEMESON +=3D "-Dc_args=3D"${CFLAGS} -I${STAGING_INCDIR_IMX}""
> +DEFAULT_AUDIOSINK ?=3D "autoaudiosink"
> +DEFAULT_VIDEOSINK ?=3D "autovideosink"
> +DEFAULT_VIDEOSINK:mx8qm-nxp-bsp =3D "imxvideoconvert_g2d ! autovideosink=
"
> +DEFAULT_VIDEOSINK:mx8qxp-nxp-bsp =3D "imxvideoconvert_g2d ! autovideosin=
k"
> +
> +EXTRA_OEMESON +=3D "-Ddefaultaudiosink=3D"${DEFAULT_AUDIOSINK}" -Ddefaul=
tvideosink=3D"${DEFAULT_VIDEOSINK}" -Dc_args=3D"${CFLAGS} -I${STAGING_INCDI=
R_IMX}""

This all can be solved with PACKAGECONFIG, why it is needed to extend
the Meson options?

>
>  # links with imx-gpu libs which are pre-built for glibc
>  # gcompat will address it during runtime
> --
> 2.25.1
>

Link: [1]: https://gitlab.freedesktop.org/gstreamer/gstreamer


--
Regards,
Andrey.