Created attachment 352629 [details] [review]
h264: factor out profile and level parsing

Will allow to re-use them in the decoder element.

Created attachment 352630 [details] [review]
h264dec: pass profile and level to OMX

This information may be useful to some decoders. For example, they
could perform some optimization or try reducing the latency depending of
the profile.

The decoder can get this information from the SPS, no?

Ah could be, don't know.

Receiving it through the caps is earlier though which can be useful to configure the decoder before it's actually running.

The only purpose of having these in the caps is to be able to fallback to another decoder if one does not support the level/profile found. It is really strange that a decoder, specially a bytestream decoder, would require this information to be parsed by an external source.

So I checked and our OMX implementation needs to know this info to properly allocate its internal buffers, so this has to be done before it receives any data.

If you think that's not something useful for other implementations maybe I can enable this code path only for our platform? Or add a hack for it?

It should be fine to always pass it, it shouldn't hurt.

However what I saw in other OMX implementations is that they had some configuration for passing in a complete avcC struct (i.e. the whole SPS/PPS).

This sounds all highly unusual to me. Perhaps Nicolas could take a look as well to verify this is really needed.

Review of attachment 352630 [details] [review]:

::: omx/gstomxh264dec.c
@@ +138,3 @@
+  if (err != OMX_ErrorNone) {
+    GST_WARNING_OBJECT (self,
+        "Setting profile/level not supported by component");

This message is wrong, you call get_parameter and warn the setting the profile is not supported. Also, calling get_parameter here without first providing PPS/SPS is useless, if by chance you get a value, it's just going to be random. If what you wanted is to iterate the supported value, you'd need to try OMX_IndexParamVideoProfileLevel
QuerySupported instead.

@@ +143,3 @@
+
+  peercaps = gst_pad_peer_query_caps (GST_VIDEO_DECODER_SINK_PAD (dec),
+      gst_pad_get_pad_template_caps (GST_VIDEO_DECODER_SINK_PAD (dec)));

Maybe you can use gst_pad_get_allowed_caps() to simplify ?

@@ +175,3 @@
+  if (err == OMX_ErrorUnsupportedIndex) {
+    GST_WARNING_OBJECT (self,
+        "Setting profile/level not supported by component");

Just to illustrate how unusual this is, see OMX spec, table 4-61, and notice that they say this is for "encoding". The usage of this paremeter for decoding is not prohibited, but is a bit of a gray zone. It's pretty much undefined, and I'm worried we'll endup issuing warnings on all the other platforms with hat patch.

"OMX_IndexParamVideoProfileLevel:
Current eCompressionFormat, eProfile and eLevel will contain the requested settiins to be used as part of the **encoding.** The nProfileIndex parameter is an ignored parameter."

p.s. the settiins typo is from the spec, I think they wanted settings

Guillaume just brought up that it's actually against the spec, see 8.8.1.2, OMX_IndexParamVideoProfileLevelCurrent is read-only for decoder. So that must live as a platform specific code, or as a configured hack.

So, what do we do, mark as invalid ?

I see no reason why it wouldn't be a quirk. GStreamer already does this by having the parser give you the profile/level in the caps, so passing it to the OMX element sounds like a totally valid quirk.

For me the main question was whether this decoder really actually needs this and can make use of it before the SPS/PPS is being passed, which seems unusual to me. I wouldn't want to see this merged unless this has been confirmed.

Their decoder does some expensive initialization, so doing it before the first frame improves the startup latency quite a bit. We already had this discussion with them.

Well, it's against the spec, so make this patch a target specific hack, or a run-time hack, not the default path and then we can consider the patch.

I'm discussing with the team to see if we can avoid relying on this as suggested. Will update the patch if/when needed.

Ping.

Created attachment 355011 [details] [review]
omxh264enc: move profile and level parsing functions to their own files

Will allow to re-use them in the decoder element.

Created attachment 355012 [details] [review]
h264dec: pass profile and level to OMX

This information can be useful to zynqultrascaleplus decoders. They may
use this information to reduce startup latency by configuring itself
before receiving the first frames.

We also have a custom OMX extension allowing the decoder to report the
latency. The profile/level information helps it reporting a more
accurate latency earlier.

Review of attachment 355012 [details] [review]:

I still think this is horrible, the component should figure-out that info from the bitstream. In any case, if we merge this, it should be in the form of a "hack" (i.e. run-time quirk) and enabled in the configuration. We have enough of these ifdef, not need to add more.

Created attachment 355247 [details] [review]
h264dec: add hack to pass profile and level to OMX

This information can be useful to zynqultrascaleplus decoders. They may
use this information to reduce startup latency by configuring itself
before receiving the first frames.

We also have a custom OMX extension allowing the decoder to report the
latency. The profile/level information helps it reporting a more
accurate latency earlier.

Created attachment 355248 [details] [review]
h264dec: add hack to pass profile and level to OMX

This information can be useful to zynqultrascaleplus decoders. They may
use this information to reduce startup latency by configuring itself
before receiving the first frames.

We also have a custom OMX extension allowing the decoder to report the
latency. The profile/level information helps it reporting a more
accurate latency earlier.

Review of attachment 355248 [details] [review]:

Looks good to me

::: omx/gstomxh264dec.c
@@ +123,3 @@
+  GST_OMX_INIT_STRUCT (&param);
+  param.nPortIndex = GST_OMX_VIDEO_DEC (self)->dec_in_port->index;
+

Why not do gst_pad_get_current_caps() instead? Ie, doing it from the caps event. The result of this query will often not yield a fixed caps.

Review of attachment 355248 [details] [review]:

The caps is directly available in the CodeState, please use that.

Created attachment 355314 [details] [review]
h264dec: add hack to pass profile and level to OMX

This information can be useful to zynqultrascaleplus decoders. They may
use this information to reduce startup latency by configuring itself
before receiving the first frames.

We also have a custom OMX extension allowing the decoder to report the
latency. The profile/level information helps it reporting a more
accurate latency earlier.

Review of attachment 355314 [details] [review]:

Looks good now!

Review of attachment 355011 [details] [review]:

Good too

Merged

commit 0aa4c9db4e3263c1cc70eb871a9d30b330c98e61
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Tue Mar 28 16:27:10 2017 +0200

    h264dec: add hack to pass profile and level to OMX
    
    This information can be useful to zynqultrascaleplus decoders. They may
    use this information to reduce startup latency by configuring itself
    before receiving the first frames.
    
    We also have a custom OMX extension allowing the decoder to report the
    latency. The profile/level information helps it reporting a more
    accurate latency earlier.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783114

commit 1e570fed17a2cf3ecfbca3b04b33d5bf3fe30b47
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Mon Jul 3 13:17:11 2017 +0200

    omxh264enc: move profile and level parsing functions to their own files
    
    Will allow to re-use them in the decoder element.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783114