GNOME Bugzilla – Bug 792043
include all OMX extension headers if present
Last modified: 2018-01-30 11:55:28 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.
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.
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.
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 :)
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.
(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).
I would say, at least make an effort to not split the check and the if.
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.
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 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.
Indeed. Let's use the same change but keep the CFLAGS part then.
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.
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.
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.
Attachment 367633 [details] pushed as 13a4375 - meson: simplify OMX extensions detection Attachment 367634 [details] pushed as ab181a4 - include all OMX extension headers if present