After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 792043 - include all OMX extension headers if present
include all OMX extension headers if present
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-12-29 13:32 UTC by Guillaume Desmottes
Modified: 2018-01-30 11:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
include all OMX extension headers if present (5.87 KB, patch)
2017-12-29 13:32 UTC, Guillaume Desmottes
none Details | Review
meson: use include_directories() with external OMX headers path (5.19 KB, patch)
2018-01-30 09:52 UTC, Guillaume Desmottes
none Details | Review
include all OMX extension headers if present (4.75 KB, patch)
2018-01-30 09:52 UTC, Guillaume Desmottes
none Details | Review
meson: simplify OMX extensions detection (4.96 KB, patch)
2018-01-30 11:04 UTC, Guillaume Desmottes
committed Details | Review
include all OMX extension headers if present (4.75 KB, patch)
2018-01-30 11:04 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2017-12-29 13:32:22 UTC
Actually I just need OMX_CoreExt.h (used on the zynqultrascaleplus) but I added support for all extension headers while I was on it.
Comment 1 Guillaume Desmottes 2017-12-29 13:32:48 UTC
Created attachment 366076 [details] [review]
include all OMX extension headers if present

The OMX specs defines 8 headers that implementations can use to define
their custom extensions. We were checking and including 3 and ignoring
the other ones.
Comment 2 Nicolas Dufresne (ndufresne) 2018-01-29 11:47:34 UTC
Review of attachment 366076 [details] [review]:

Looks correct, even though I think the way the meson script is made is not nice.

::: meson.build
@@ +298,3 @@
+if have_core_ext
+  cdata.set ('HAVE_CORE_EXT', 1)
+endif

Why such an expansion ? (I guess was there before though).

if (cc.has_header ('my.h', args: gst_omx_args, requires: false)
  cdata.set ('...
endif

Would be denser and plenty readable.
Comment 3 Tim-Philipp Müller 2018-01-29 11:58:50 UTC
For what it's worth, you can also do

  cdata.set('HAVE_FOO', cc.has_header())

or (if it must be defined to 1 or 0)

  cdata.set10('HAVE_FOO', cc.has_header())

though in this case Nicolas's suggestion from comment #2 might be more readable :)
Comment 4 Nicolas Dufresne (ndufresne) 2018-01-29 12:11:44 UTC
Ah, I was worried that would only implement what set10 is doing, thanks for letting us know.

Any of these two ways are better, I think it's worth converting sooner then later.
Comment 5 Guillaume Desmottes 2018-01-29 13:42:41 UTC
(In reply to Nicolas Dufresne (stormer) from comment #2)
> Review of attachment 366076 [details] [review] [review]:
> 
> Looks correct, even though I think the way the meson script is made is not
> nice.
> 
> ::: meson.build
> @@ +298,3 @@
> +if have_core_ext
> +  cdata.set ('HAVE_CORE_EXT', 1)
> +endif
> 
> Why such an expansion ? (I guess was there before though).

Becase we actually check if those headers are present if 'have_external_omx == true'. If not we hardcode the ones present in our copy of the OMX headers (hence the have_*_ext = true/false initialization).

One option could be to change the logic to always detect which files are present (whether using external OMX headers or not).
Comment 6 Nicolas Dufresne (ndufresne) 2018-01-29 17:54:58 UTC
I would say, at least make an effort to not split the check and the if.
Comment 7 Guillaume Desmottes 2018-01-30 09:52:11 UTC
Created attachment 367619 [details] [review]
meson: use include_directories() with external OMX headers path

It seems cleaner to use the proper meson tools to include this path
rather than manually tweak the build flags.

This also allows us to simplify the OMX extensions detection code. We
are now always checking which files are present, even when using our
internal copy of OMX, rather than hardcoding the ones present in it.
Comment 8 Guillaume Desmottes 2018-01-30 09:52:17 UTC
Created attachment 367620 [details] [review]
include all OMX extension headers if present

The OMX specs defines 8 headers that implementations can use to define
their custom extensions. We were checking and including 3 and ignoring
the other ones.
Comment 9 Tim-Philipp Müller 2018-01-30 10:10:25 UTC
Comment on attachment 367619 [details] [review]
meson: use include_directories() with external OMX headers path

I think include_directories() is only for local paths in the source tree, not for external include paths.
Comment 10 Guillaume Desmottes 2018-01-30 11:01:28 UTC
Indeed. Let's use the same change but keep the CFLAGS part then.
Comment 11 Guillaume Desmottes 2018-01-30 11:04:10 UTC
Created attachment 367633 [details] [review]
meson: simplify OMX extensions detection

We are now always checking which files are present or not, even when using our
internal copy of OMX, rather than hardcoding the ones present in it.
Comment 12 Guillaume Desmottes 2018-01-30 11:04:15 UTC
Created attachment 367634 [details] [review]
include all OMX extension headers if present

The OMX specs defines 8 headers that implementations can use to define
their custom extensions. We were checking and including 3 and ignoring
the other ones.
Comment 13 Nicolas Dufresne (ndufresne) 2018-01-30 11:15:41 UTC
Review of attachment 367633 [details] [review]:

::: meson.build
@@ +173,1 @@
   gst_omx_args += ['-I' + omx_header_path]

There is a merge conflict with upstream on that one. Apparently, upstream does omx_inc = include_directories (omx_header_path), which already seems correct.
Comment 14 Nicolas Dufresne (ndufresne) 2018-01-30 11:54:47 UTC
Attachment 367633 [details] pushed as 13a4375 - meson: simplify OMX extensions detection
Attachment 367634 [details] pushed as ab181a4 - include all OMX extension headers if present