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 791211 - omxvideodec/enc: update nBufferCountActual before allocating
omxvideodec/enc: update nBufferCountActual before allocating
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal enhancement
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 795746
 
 
Reported: 2017-12-04 14:51 UTC by Guillaume Desmottes
Modified: 2018-06-08 07:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
omxvideoenc/dec: factor out input buffers allocation (4.79 KB, patch)
2017-12-04 14:51 UTC, Guillaume Desmottes
committed Details | Review
omxvideoenc: factor out output buffers allocation (2.16 KB, patch)
2017-12-04 14:51 UTC, Guillaume Desmottes
none Details | Review
omxvideodec/enc: update nBufferCountActual before allocating (4.41 KB, patch)
2017-12-04 14:51 UTC, Guillaume Desmottes
rejected Details | Review
omxvideodec/enc: add hack updating nBufferCountActual before allocating (8.81 KB, patch)
2018-05-17 13:35 UTC, Guillaume Desmottes
committed Details | Review
zynqultrascaleplus: enable 'ensure-buffer-count-actual' hack (1.50 KB, patch)
2018-05-17 13:35 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2017-12-04 14:51:30 UTC
.
Comment 1 Guillaume Desmottes 2017-12-04 14:51:46 UTC
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
Comment 2 Guillaume Desmottes 2017-12-04 14:51:52 UTC
Created attachment 364915 [details] [review]
omxvideoenc: factor out output buffers allocation

No semantic change.
Comment 3 Guillaume Desmottes 2017-12-04 14:51:58 UTC
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.
Comment 4 Nicolas Dufresne (ndufresne) 2017-12-04 15:13:48 UTC
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 ?
Comment 5 Guillaume Desmottes 2017-12-04 15:23:20 UTC
(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.
Comment 6 Guillaume Desmottes 2017-12-05 12:05:14 UTC
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.
Comment 7 Guillaume Desmottes 2017-12-05 12:10:37 UTC
Hum any suggestion about how to fix that? Add a GST_OMX_HACK_NO_BUFFER_COUNT_ACTUAL_UPDATE ? :\
Comment 8 Olivier Crête 2017-12-05 14:59:06 UTC
(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 :-(
Comment 9 Julien Isorce 2017-12-05 22:04:56 UTC
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 !
Comment 10 Guillaume Desmottes 2018-05-17 13:35:15 UTC
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.
Comment 11 Guillaume Desmottes 2018-05-17 13:35:21 UTC
Created attachment 372151 [details] [review]
zynqultrascaleplus: enable 'ensure-buffer-count-actual' hack
Comment 12 Guillaume Desmottes 2018-05-17 13:35:55 UTC
(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.
Comment 13 Nicolas Dufresne (ndufresne) 2018-06-07 19:01:18 UTC
Review of attachment 372150 [details] [review]:

Ok.
Comment 14 Nicolas Dufresne (ndufresne) 2018-06-07 19:01:51 UTC
Review of attachment 372151 [details] [review]:

Ok.
Comment 15 Guillaume Desmottes 2018-06-08 07:45:20 UTC
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