GNOME Bugzilla – Bug 785967
videoenc/dec : delay input buffers allocation until first buffer is received
Last modified: 2018-10-11 17:31:50 UTC
Delay the allocation of the OMX input buffers until we receive the first buffer so we can make more informed decisions. Also for the encoder, adjust the stride and slice of the input port accordingly to avoid copying line-by-line each input buffer.
Created attachment 357150 [details] [review] omxvideoenc: factor out enable and disable code No semantic change, just factor out the code enabling and disabling the component to their own functions. Makes the code easier to read as the set_format() method was already pretty big. Will also allow us to easily change the enabling logic.
Created attachment 357151 [details] [review] omxvideodec: factor out enable and disable code No semantic change, just factor out the code enabling and disabling the component to their own functions. Makes the code easier to read as the set_format() method was already pretty big. Will also allow us to easily change the enabling logic.
Created attachment 357152 [details] [review] omxvideodec: earlier return if downstream_flow_ret is not OK There is no point to (re)start the src thread if, for example, we are flushing.
Created attachment 357153 [details] [review] omxvideoenc: start src thread in handle_frame() Makes the code simpler as we no longer need to restart the thread in gst_omx_video_enc_flush() and It's more symetric which the omxvideodec implementation. I'm also going to move the enabling of the OMX component in handle_frame() and the src pad thread needs to be started after it.
Created attachment 357154 [details] [review] omxvideodec/enc: delay allocation after the allocation query Allocating OMX components buffers in set_format() is too early. Doing it when receiving the first buffers will allow the element to use the information from the allocation query and/or the first incoming buffer to pick to best allocation mode. Tested on raspberry pi with dynamic resolution changes on decoder and encoder input.
Created attachment 357155 [details] [review] omxvideoenc: delay buffer configuration until component is enabled No significant change for now. Just delay the input port configuration of the buffer size related fields (stride, slice height, buffer size) until the component is activated. This will allow us to use the actual stride/height of the first input and so avoid the buffer copying code path in most cases. Tested on rpi and zynqultrascaleplus.
Created attachment 357156 [details] [review] omxvideoenc: adjust stride and slice height from input Use the stride and slice height information from the first buffer meta data to adjust the settings of the input port. This will ensure that the OMX input buffers match the GStreamer ones and so will save us from having to copy line-by-line each one. This is also the first step to allow the OMX encoder to receive dmabuf. Tested on rpi and zynqultrascaleplus.
Generally makes sense to me, but didn't look through the patches in detail.
Review of attachment 357150 [details] [review]: Indeed, I can't see any functional change in there, and it helps in making the code readable. (I'll merge when review is complete, just marking for now).
Review of attachment 357151 [details] [review]: Looks good.
Review of attachment 357152 [details] [review]: Make sense.
Review of attachment 357153 [details] [review]: Looks good.
Review of attachment 357154 [details] [review]: Looks good.
Review of attachment 357155 [details] [review]: Looks good, just one nitpick. ::: omx/gstomxvideoenc.c @@ +1005,2 @@ static gboolean +configure_input_buffer (GstOMXVideoEnc * self) To be consistent, we should keep the namespace here. It's also easier for debugging, as it prevents having function name clash.
Created attachment 357198 [details] [review] omxvideoenc: delay buffer configuration until component is enabled No significant change for now. Just delay the input port configuration of the buffer size related fields (stride, slice height, buffer size) until the component is activated. This will allow us to use the actual stride/height of the first input and so avoid the buffer copying code path in most cases. Tested on rpi and zynqultrascaleplus.
Created attachment 357199 [details] [review] omxvideoenc: adjust stride and slice height from input Use the stride and slice height information from the first buffer meta data to adjust the settings of the input port. This will ensure that the OMX input buffers match the GStreamer ones and so will save us from having to copy line-by-line each one. This is also the first step to allow the OMX encoder to receive dmabuf. Tested on rpi and zynqultrascaleplus.
Review of attachment 357156 [details] [review]: Looks good.
Review of attachment 357198 [details] [review]: Good.d
Review of attachment 357199 [details] [review]: Ok, I think the original code was broken, only ever worked for formats with Y plane first. ::: omx/gstomxvideoenc.c @@ +1019,3 @@ + stride = meta->stride[0]; + if (stride == 0) + stride = info->width; Missing a multiplication ? In RGBA default stride is width * 4. In my opinion, GstVideoMeta with a 0 stride is an upstream bug, so assert instead. @@ +1020,3 @@ + if (stride == 0) + stride = info->width; + slice_height = meta->offset[1] / stride; (meta->offset[1] - meta->offset[0]) / stride. There is no guaranty that the first offset is 0. @@ +1029,3 @@ + "input buffer doesn't provide video meta, can't adjust stride and slice height"); + + stride = info->width; stride = info->stride[0]; @@ -1014,3 @@ if (port_def.nBufferAlignment) port_def.format.video.nStride = - GST_ROUND_UP_N (info->width, port_def.nBufferAlignment); Or, is the stride in OMX in pixels ?
Created attachment 357202 [details] [review] omxvideoenc: adjust stride and slice height from input Use the stride and slice height information from the first buffer meta data to adjust the settings of the input port. This will ensure that the OMX input buffers match the GStreamer ones and so will save us from having to copy line-by-line each one. This is also the first step to allow the OMX encoder to receive dmabuf. Tested on rpi and zynqultrascaleplus.
Review of attachment 357202 [details] [review]: Better.
Attachment 357150 [details] pushed as 973c6e1 - omxvideoenc: factor out enable and disable code Attachment 357151 [details] pushed as 352184d - omxvideodec: factor out enable and disable code Attachment 357152 [details] pushed as 7916bd5 - omxvideodec: earlier return if downstream_flow_ret is not OK Attachment 357153 [details] pushed as 90da3c6 - omxvideoenc: start src thread in handle_frame() Attachment 357154 [details] pushed as 0603e44 - omxvideodec/enc: delay allocation after the allocation query Attachment 357198 [details] pushed as 07e80c9 - omxvideoenc: delay buffer configuration until component is enabled Attachment 357202 [details] pushed as 0ee8213 - omxvideoenc: adjust stride and slice height from input
Thanks.
This broke very simple encoding pipeline on RPI: commit 07e80c9425237bfb38684cd255af248068657935 Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Mon Aug 7 11:45:29 2017 -0400 omxvideoenc: delay buffer configuration until component is enabled No significant change for now. Just delay the input port configuration of the buffer size related fields (stride, slice height, buffer size) until the component is activated. This will allow us to use the actual stride/height of the first input and so avoid the buffer copying code path in most cases. Tested on rpi and zynqultrascaleplus. https://bugzilla.gnome.org/show_bug.cgi?id=785967 :040000 040000 d0656b6181267d3218768f354ee2138e6713e10f 3656d708113e3eefb75645f08bfd5e4a624e3ac1 M omx Very simple encoding pipelines fail: ``` bash-4.3# GST_DEBUG="2,*omx*:4" gst-launch-1.0 videotestsrc ! omxh264enc ! fakesink Setting pipeline to PAUSED ... Pipeline is PREROLLING ... 0:00:00.087341219 1969 0x19ae950 WARN videoencoder gstvideoencoder.c:678:gst_video_encoder_setcaps:<omxh264enc-omxh264enc0> rejected caps video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive 0:00:00.089955653 1969 0x19ae950 WARN videoencoder gstvideoencoder.c:678:gst_video_encoder_setcaps:<omxh264enc-omxh264enc0> rejected caps video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive 0:00:00.090013361 1969 0x19ae950 WARN GST_PADS gstpad.c:4224:gst_pad_peer_query:<videotestsrc0:src> could not send sticky events 0:00:00.103096156 1969 0x19ae950 WARN videoencoder gstvideoencoder.c:678:gst_video_encoder_setcaps:<omxh264enc-omxh264enc0> rejected caps video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive 0:00:00.107846793 1969 0x19ae950 WARN videoencoder gstvideoencoder.c:678:gst_video_encoder_setcaps:<omxh264enc-omxh264enc0> rejected caps video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive 0:00:00.108003773 1969 0x19ae950 WARN basesrc gstbasesrc.c:3055:gst_base_src_loop:<videotestsrc0> error: Internal data stream error. 0:00:00.108068252 1969 0x19ae950 WARN basesrc gstbasesrc.c:3055:gst_base_src_loop:<videotestsrc0> error: streaming stopped, reason not-negotiated (-4) ERROR: from element /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0: Internal data stream error. Additional debug info: ../libs/gst/base/gstbasesrc.c(3055): gst_base_src_loop (): /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0: streaming stopped, reason not-negotiated (-4) ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... 0:00:00.113733840 1969 0x19ae950 WARN videoencoder gstvideoencoder.c:678:gst_video_encoder_setcaps:<omxh264enc-omxh264enc0> rejected caps video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive 0:00:00.115415563 1969 0x19af640 INFO omx gstomx.c:764:gst_omx_component_free:<omxh264enc-omxh264enc0> Unloading component 0x18e35e0 video_encode 0:00:00.115460824 1969 0x19af640 INFO omx gstomx.c:1787:gst_omx_port_deallocate_buffers_unlocked:<omxh264enc-omxh264enc0> Deallocating buffers of video_encode port 200 0:00:00.115614366 1969 0x19af640 INFO omx gstomx.c:1787:gst_omx_port_deallocate_buffers_unlocked:<omxh264enc-omxh264enc0> Deallocating buffers of video_encode port 201 ```
We are twelve months later and there is a lot of patches on top. We should open a new bug as reverting is likely not an option. This is probably a bug in profile negotiation btw, as I see not-neg and you haven't specify any in your simple pipeline.
(In reply to Nicolas Dufresne (ndufresne) from comment #25) > We are twelve months later and there is a lot of patches on top. We should > open a new bug as reverting is likely not an option. This is probably a bug > in profile negotiation btw, as I see not-neg and you haven't specify any in > your simple pipeline. Well usually we just reopen when the landed patch creates a regression, I can open another bug but I do not see the point. To me this should be a blocker for 1.14.2 also.
> To me this should be a blocker for 1.14.2 also. That seems rather ambitious seeing that 1.14.2 has already been released :)
(In reply to Tim-Philipp Müller from comment #27) > > To me this should be a blocker for 1.14.2 also. > > That seems rather ambitious seeing that 1.14.2 has already been released :) Still doable :P Let's say 1.14.3 then ;)
Just tested with gst-omx master and this pipeline is working. Did you properly configure gst-omx? $ meson configure build/ | grep omx gst-omx:header_path /opt/vc/include/IL/ An extra include directory to find the OpenMax headers gst-omx:struct_packing 0 [0, 1, 2, 4, 8] Force OpenMAX struct packing gst-omx:target rpi [none, generic, rpi, bellagio, tizonia, zynqultrascaleplus] The OMX platform to target omx enabled [enabled, disabled, auto] omx
Same thing: bash-4.3# meson configure build/ | grep omx Source dir /root/gst-omx Build dir /root/gst-omx/build with_omx_header_path /opt/vc/include/IL/ An extra include directory to find the OpenMax headers with_omx_struct_packing 0 [0, 1, 2, 4, 8] Force OpenMAX struct packing with_omx_target rpi [none, generic, rpi, bellagio, tizonia, zynqultrascaleplus] The OMX platform to target What RPI do you use? I have a RPI3 model B here.
It's a PI3 B+. I'm building gst 1.14 for testing with this version.
Yep, working fine with 1.14 as well: $ GST_DEBUG="2,*omx*:4" gst-launch-1.0 videotestsrc ! omxh264enc ! fakesink -v Setting pipeline to PAUSED ... Pipeline is PREROLLING ... /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0.GstPad:src: caps = video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive /GstPipeline:pipeline0/GstOMXH264Enc-omxh264enc:omxh264enc-omxh264enc0.GstPad:sink: caps = video/x-raw, width=(int)320, height=(int)240, framerate=(fraction)30/1, format=(string)I420, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive 0:00:00.165774746 9311 0x1982d50 INFO omx gstomx.c:2155:gst_omx_port_set_enabled_unlocked:<omxh264enc-omxh264enc0> Setting video_encode port 201 to disabled 0:00:00.166322295 9311 0x1982d50 INFO omx gstomx.c:2196:gst_omx_port_set_enabled_unlocked:<omxh264enc-omxh264enc0> Set video_encode port 201 to disabled: None (0x00000000) 0:00:00.166462346 9311 0x1982d50 INFO omx gstomx.c:2412:gst_omx_port_wait_enabled_unlocked:<omxh264enc-omxh264enc0> Waiting for video_encode port 201 to be disabled 0:00:00.166689323 9311 0x1982d50 INFO omx gstomx.c:2463:gst_omx_port_wait_enabled_unlocked:<omxh264enc-omxh264enc0> video_encode port 201 is disabled: None (0x00000000) 0:00:00.166724584 9311 0x1982d50 INFO omx gstomx.c:911:gst_omx_component_set_state:<omxh264enc-omxh264enc0> Setting video_encode state from Loaded to Idle 0:00:00.167399892 9311 0x1982d50 INFO omx gstomx.c:1761:gst_omx_port_allocate_buffers_unlocked:<omxh264enc-omxh264enc0> Allocating 1 buffers of size 115200 for video_encode port 200 0:00:00.167605463 9311 0x1982d50 INFO omx gstomx.c:261:gst_omx_component_handle_messages:<omxh264enc-omxh264enc0> video_encode state change to Idle finished 0:00:00.167746452 9311 0x1982d50 INFO omx gstomx.c:1823:gst_omx_port_allocate_buffers_unlocked:<omxh264enc-omxh264enc0> Allocated buffers for video_encode port 200: None (0x00000000) 0:00:00.167780202 9311 0x1982d50 INFO omx gstomx.c:911:gst_omx_component_set_state:<omxh264enc-omxh264enc0> Setting video_encode state from Idle to Executing 0:00:00.168351031 9311 0x1982d50 INFO omx gstomx.c:261:gst_omx_component_handle_messages:<omxh264enc-omxh264enc0> video_encode state change to Executing finished 0:00:00.170746797 9311 0x72402830 INFO omx gstomx.c:2155:gst_omx_port_set_enabled_unlocked:<omxh264enc-omxh264enc0> Setting video_encode port 201 to enabled /GstPipeline:pipeline0/GstOMXH264Enc-omxh264enc:omxh264enc-omxh264enc0.GstPad:src: caps = video/x-h264, stream-format=(string)byte-stream, alignment=(string)au, profile=(string)high, level=(string)4, chroma-format=(string)4:2:0, bit-depth-luma=(uint)8, bit-depth-chroma=(uint)8, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)30/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, chroma-site=(string)jpeg, multiview-mode=(string)mono, multiview-flags=(GstVideoMultiviewFlagsSet)0:ffffffff:/right-view-first/left-flipped/left-flopped/right-flipped/right-flopped/half-aspect/mixed-mono /GstPipeline:pipeline0/GstFakeSink:fakesink0.GstPad:sink: caps = video/x-h264, stream-format=(string)byte-stream, alignment=(string)au, profile=(string)high, level=(string)4, chroma-format=(string)4:2:0, bit-depth-luma=(uint)8, bit-depth-chroma=(uint)8, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, framerate=(fraction)30/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, chroma-site=(string)jpeg, multiview-mode=(string)mono, multiview-flags=(GstVideoMultiviewFlagsSet)0:ffffffff:/right-view-first/left-flipped/left-flopped/right-flipped/right-flopped/half-aspect/mixed-mono 0:00:00.171173096 9311 0x72402830 INFO omx gstomx.c:2196:gst_omx_port_set_enabled_unlocked:<omxh264enc-omxh264enc0> Set video_encode port 201 to enabled: None (0x00000000) 0:00:00.171281273 9311 0x72402830 INFO omx gstomx.c:1761:gst_omx_port_allocate_buffers_unlocked:<omxh264enc-omxh264enc0> Allocating 1 buffers of size 65536 for video_encode port 201 0:00:00.171585698 9311 0x72402830 INFO omx gstomx.c:1823:gst_omx_port_allocate_buffers_unlocked:<omxh264enc-omxh264enc0> Allocated buffers for video_encode port 201: None (0x00000000) 0:00:00.171694551 9311 0x72402830 INFO omx gstomx.c:2412:gst_omx_port_wait_enabled_unlocked:<omxh264enc-omxh264enc0> Waiting for video_encode port 201 to be enabled 0:00:00.171857623 9311 0x72402830 INFO omx gstomx.c:2463:gst_omx_port_wait_enabled_unlocked:<omxh264enc-omxh264enc0> video_encode port 201 is enabled: None (0x00000000) 0:00:00.172077986 9311 0x72402830 INFO omx gstomx.c:2512:gst_omx_port_mark_reconfigured:<omxh264enc-omxh264enc0> Marking video_encode port 201 is reconfigured 0:00:00.172172569 9311 0x72402830 INFO omx gstomx.c:2539:gst_omx_port_mark_reconfigured:<omxh264enc-omxh264enc0> Marked video_encode port 201 as reconfigured: None (0x00000000) Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstSystemClock
OK weird, I am gonna investigate on my side then.
I updated the rpi firmware using rpi-update to 911147a3276beee09afc4237e1b7b964e61fb88a and indeed it's no longer working. So that may be an OMX regression with the new firmware.
I can confirm that when going back to 07e80c9425237b^, so before the above change in question, omx encoding starts working again on my Rpi Zero W. So it seems that newer firmwares do not like when the input buffer is configured before the port is enabled?
This is on firmware: $ /opt/vc/bin/vcgencmd version Sep 21 2018 15:43:17 Copyright (c) 2012 Broadcom version 07f57128b8491ffdefcdfd13f7b4961b3006d9a9 (clean) (release)
$ sudo rpi-update 5315c7a2f806c123e9e4ee26980f0019c43b9f69 downgrades to a firmware with which it still works, compare https://github.com/raspberrypi/firmware/issues/1051#issuecomment-426229166
I don't think flipping init order was wanted in this change, all we wanted is to delay. Normally we allocate before enabling unless we know we have OMX 1.2 draft support.
Should be fixed according to https://github.com/raspberrypi/firmware/issues/1051#issuecomment-428233807 - I can't check easily, anyone with a setup in place, please check and close the bug :-)
Went and updated my RPI accordingly using rpi-update to latest. Works for me for a simple pipeline.
Great, thanks for testing !
I should have mentioned, I did see a strip of green tint in some resolutions/tests, perhaps about 1/6th of the height. Could that have something to do with the stride/padding issues? - but I can try to file that as a separate issue once I know more.
Best to file a specific issue for that. It's probably not a regression as per see.