Created attachment 364914 [details] [review]
omxvideoenc/dec: factor out input buffers allocation

No semantic change so far. I'm going to add an alternate way to allocate
input buffers.

https://bugzilla.gnome.org/show_bug.cgi?id=787093

Created attachment 364915 [details] [review]
omxvideoenc: factor out output buffers allocation

No semantic change.

Created attachment 364916 [details] [review]
omxvideodec/enc: update nBufferCountActual before allocating

The OMX specs states that the nBufferCountActual of a port has to default
to its nBufferCountMin. If we don't change nBufferCountActual we purely rely
on this default. But in some cases, OMX may change nBufferCountMin before we
allocate buffers. Like for example when configuring the input ports with the
actual format, it may decrease the number of minimal buffers required.
This method checks this and update nBufferCountActual if needed so we'll use
less buffers than the worst case in such scenarios.

Don't do this for the decoder output as
gst_omx_video_dec_allocate_output_buffers() already check
nBufferCountMin when computing the number of output buffers.

Review of attachment 364916 [details] [review]:

You always call this before gst_omx_port_allocate_buffers(), wouldn't it make it less complex to embed it into that function ?

(In reply to Nicolas Dufresne (stormer) from comment #4)
> You always call this before gst_omx_port_allocate_buffers(), wouldn't it
> make it less complex to embed it into that function ?

That would prevent elements from increasing the number of buffers like we do for the decoder output.

Review of attachment 364916 [details] [review]:

::: omx/gstomxvideodec.c
@@ +2137,3 @@
 gst_omx_video_dec_allocate_in_buffers (GstOMXVideoDec * self)
 {
+  if (!gst_omx_port_ensure_buffer_count_actual (self->dec_in_port))

Humm looks like this breaks omxh264dec on the pi :(
Its input port has nBufferCountActual=20 as default but nBufferCountMin=1 (which is a violation of the spec). And the component isn't working with only one buffer.

Hum any suggestion about how to fix that? Add a GST_OMX_HACK_NO_BUFFER_COUNT_ACTUAL_UPDATE ? :\

(In reply to Guillaume Desmottes from comment #7)
> Hum any suggestion about how to fix that? Add a
> GST_OMX_HACK_NO_BUFFER_COUNT_ACTUAL_UPDATE ? :\

I guess so :-(

Unless I am mistaken, 1.1.2 spec says "no less than" and 1.2.0 spec says "equal". So I think nBufferCountActual=2 >= nBufferCountMin=1 falls ok in the 1.1.2 requirements.

That said are you sure that it cannot work with one input buffer ? Maybe it is just a limitation on how gst-omx uses omx. It is not optimal at all but I think it should work with one buffer.

I think we should just set an optimal value. Value computed depending if the pool is proposed to upstream element, if not it would depend on the size of the coming buffer and the max size omx can allocate.

Thx for working on this !

Created attachment 372150 [details] [review]
omxvideodec/enc: add hack updating nBufferCountActual before allocating

The OMX specs states that the nBufferCountActual of a port has to default
to its nBufferCountMin. If we don't change nBufferCountActual we purely rely
on this default. But in some cases, OMX may change nBufferCountMin before we
allocate buffers. Like for example when configuring the input ports with the
actual format, it may decrease the number of minimal buffers required.
This method checks this and update nBufferCountActual if needed so we'll use
less buffers than the worst case in such scenarios.

SetParameter() needs to be called when the port is either disabled or
the component in the Loaded state.

Don't do this for the decoder output as
gst_omx_video_dec_allocate_output_buffers() already check
nBufferCountMin when computing the number of output buffers.

On some platform, like rpi, the default nBufferCountActual is much
higher than nBufferCountMin so only enable this using a specific gst-omx
hack.

Created attachment 372151 [details] [review]
zynqultrascaleplus: enable 'ensure-buffer-count-actual' hack

(In reply to Olivier Crête from comment #8)
> (In reply to Guillaume Desmottes from comment #7)
> > Hum any suggestion about how to fix that? Add a
> > GST_OMX_HACK_NO_BUFFER_COUNT_ACTUAL_UPDATE ? :\
> 
> I guess so :-(

Updated the patches adding a gst-omx hack.

Review of attachment 372150 [details] [review]:

Ok.

Review of attachment 372151 [details] [review]:

Ok.

Attachment 372150 [details] pushed as 9a8e863 - omxvideodec/enc: add hack updating nBufferCountActual before allocating
Attachment 372151 [details] pushed as 2b48338 - zynqultrascaleplus: enable 'ensure-buffer-count-actual' hack