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 789476 - vaapi plugins: append allocator in allocation query
vaapi plugins: append allocator in allocation query
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
1.12.3
Other Linux
: Normal normal
: 1.12.5
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 792034
 
 
Reported: 2017-10-25 12:13 UTC by Florent Thiéry
Modified: 2018-03-01 23:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdpdepay: don't allocate query if caps are ANY (1.21 KB, patch)
2018-01-29 12:00 UTC, Víctor Manuel Jáquez Leal
none Details | Review
gdpdepay: don't use allocator if it has custom alloc (1.41 KB, patch)
2018-01-29 12:00 UTC, Víctor Manuel Jáquez Leal
none Details | Review
gdpdepay: don't allocation query if caps aren't fixed (1.22 KB, patch)
2018-01-31 13:01 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
gdpdepay: don't use allocator if it has custom alloc (1.41 KB, patch)
2018-01-31 13:01 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
plugins: don't share allocator in query (2.35 KB, patch)
2018-02-06 14:30 UTC, Víctor Manuel Jáquez Leal
rejected Details | Review
vaapivideomemory: remove unused macro (1.07 KB, patch)
2018-02-07 08:22 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
plugins: handle vaapi allocator in allocation query (4.46 KB, patch)
2018-02-07 08:22 UTC, Víctor Manuel Jáquez Leal
none Details | Review
plugins: handle vaapi allocator in allocation query (4.49 KB, patch)
2018-02-08 09:01 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
vaapivideobufferpool: fix regression (1.22 KB, patch)
2018-03-01 23:52 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Florent Thiéry 2017-10-25 12:13:27 UTC
$ gst-launch-1.0 videotestsrc num-buffers=1 ! "video/x-raw, format=(string)NV12, width=(int)320, height=(int)240, framerate=(fraction)30/1, multiview-mode=(string)mono, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive" ! gdppay ! filesink location=test.nv12

$ gst-launch-1.0 filesrc location=nv12.gdp ! gdpdepay ! imagefreeze ! vaapisink
Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...

** (gst-launch-1.0:8183): CRITICAL **: gst_video_info_from_caps: assertion 'gst_caps_is_fixed (caps)' failed
Got context from element 'vaapisink0': gst.vaapi.Display=context, gst.vaapi.Display=(GstVaapiDisplay)"\(GstVaapiDisplayGLX\)\ vaapidisplayglx1";
Caught SIGSEGV
Spinning.  Please run 'gdb gst-launch-1.0 8183' to continue debugging, Ctrl-C to quit, or Ctrl-\ to dump core.
Comment 1 Florent Thiéry 2017-10-25 12:14:10 UTC
Obviously test.nv12 should read nv12.gdp in the first pipeline, sorry.
Comment 2 Víctor Manuel Jáquez Leal 2018-01-29 11:58:59 UTC
The problem is not vaapisink, but gdpdepay: it assumes that downstream allocator always use the virtual alloc method, and that's not true. Thus, when requesting a new buffer, it got NULL and then a segmentation fault.
Comment 3 Víctor Manuel Jáquez Leal 2018-01-29 12:00:10 UTC
Created attachment 367571 [details] [review]
gdpdepay: don't allocate query if caps are ANY

When querying downstream for allocation, and the source caps hasn't
set a caps, using ANY by default, it raises a critical message in
console:

CRITICAL **: gst_video_info_from_caps: assertion 'gst_caps_is_fixed (caps)' failed

This patch bails out the decide_allocation() if the caps are still
ANY.
Comment 4 Víctor Manuel Jáquez Leal 2018-01-29 12:00:15 UTC
Created attachment 367572 [details] [review]
gdpdepay: don't use allocator if it has custom alloc

gdpdepay element uses the decide_allocation to fetch the downstream
allocator. Nonetheless it is possible that allocate uses a custom
alloc function, which is not usable by gdpdepay, crashing later the
application when the allocater buffer is NULL.

This patch checks for the allocator flags and reset it if the
allocator has a custom alloc function.
Comment 5 Nicolas Dufresne (ndufresne) 2018-01-29 12:13:11 UTC
Review of attachment 367572 [details] [review]:

Seems correct.
Comment 6 Florent Thiéry 2018-01-29 16:54:49 UTC
Tested here, works great.
Comment 7 Víctor Manuel Jáquez Leal 2018-01-31 13:01:37 UTC
Created attachment 367691 [details] [review]
gdpdepay: don't allocation query if caps aren't fixed

When querying downstream for allocation, and the source caps hasn't
set its caps, using ANY by default, it raises a critical message in
console:

CRITICAL **: gst_video_info_from_caps: assertion 'gst_caps_is_fixed (caps)' failed

This patch bails out decide_allocation() if the caps aren't fixed.
Comment 8 Víctor Manuel Jáquez Leal 2018-01-31 13:01:42 UTC
Created attachment 367692 [details] [review]
gdpdepay: don't use allocator if it has custom alloc

gdpdepay element uses the decide_allocation to fetch the downstream
allocator. Nonetheless it is possible that allocate uses a custom
alloc function, which is not usable by gdpdepay, crashing later the
application when the allocater buffer is NULL.

This patch checks for the allocator flags and reset it if the
allocator has a custom alloc function.
Comment 9 Víctor Manuel Jáquez Leal 2018-01-31 13:06:04 UTC
Attachment 367691 [details] pushed as f04b20e - gdpdepay: don't allocation query if caps aren't fixed
Attachment 367692 [details] pushed as f6cb16a - gdpdepay: don't use allocator if it has custom alloc
Comment 10 Víctor Manuel Jáquez Leal 2018-01-31 13:20:43 UTC
for branch 1.12

* b8ef897d0 gdpdepay: don't use allocator if it has custom alloc
* 198b3703e gdpdepay: don't allocation query if caps aren't fixed
Comment 11 Olivier Crête 2018-02-01 11:15:43 UTC
This patch is really horrible. Can't you just not return this custom allocator in the allocation query if you can't use it anyway?
Comment 12 Sebastian Dröge (slomo) 2018-02-01 13:57:01 UTC
Other elements upstream could potentially understand that custom allocator and do something meaningful with it.

IIRC the idea is that if you only have a custom allocator (that does not work normally, like this one here) to return, you *first* put the sysmem allocator in the query response... and only afterwards the custom one.

The first allocator in the query response must always be sysmem/etc compatible unless custom caps features are negotiated.
Comment 13 Nicolas Dufresne (ndufresne) 2018-02-01 18:31:22 UTC
My apology, I totally failed to notice the context of this. Indeed, without caps feature, first allocator cannot be custom. Even the second being custom is probably not the best idea.
Comment 14 Víctor Manuel Jáquez Leal 2018-02-02 14:02:22 UTC
D'accord.

What would be plan then?

If I understand correctly, it would be

1. Revert this patch
2. Change the summary of this bug and reassign it to gstreamer-vaapi
3. Now gstreamer-vaapi should set three) allocators in the propose_allocation()'s query, if the upstream negotiated caps is raw video: system's, dmabuf and the vaapi one.
4. but my question is how the vaapi buffer pool will know which allocator will use when at set_config() ???
Comment 15 Víctor Manuel Jáquez Leal 2018-02-02 17:57:54 UTC
According to bufferpool.md

> The allocation query can also return an allocator object when the buffers are
> of different sizes and can't be allocated from a pool.

So, the simplest approach is NOT sharing any allocator in the allocation query from the gstremaer-vaapi perspective.
Comment 16 Nicolas Dufresne (ndufresne) 2018-02-03 12:00:06 UTC
V4L side seems to hardcore it's allocator internally.
Comment 17 Víctor Manuel Jáquez Leal 2018-02-06 14:30:21 UTC
Created attachment 367959 [details] [review]
plugins: don't share allocator in query

VA-API allocator uses a custom alloc function, which only can be
used by VA-API elements which know the API.

When an allocator is shared, it must behaive as the system allocator
so other elements could use it. Otherwise, to share it doesn't make
sense.

This patch remove the VA-API allocator sharing.
Comment 18 Sebastian Dröge (slomo) 2018-02-06 14:44:18 UTC
I disagree with this change. Sharing the allocator can be useful for elements that actually understand VAAPI. But you must not put it as the first allocator, the first allocator always must be sysmem compatible.
Comment 19 Víctor Manuel Jáquez Leal 2018-02-07 08:22:36 UTC
Created attachment 367974 [details] [review]
vaapivideomemory: remove unused macro

GST_VAAPI_VIDEO_ALLOCATOR_NAME was added in commit 5b11b8332 but it
was never used, since the native VA-API allocator name has been
GST_VAAPI_VIDEO_MEMORY_NAME.

This patch removes GST_VAAPI_VIDEO_ALLOCATOR_NAME macro.
Comment 20 Víctor Manuel Jáquez Leal 2018-02-07 08:22:42 UTC
Created attachment 367975 [details] [review]
plugins: handle vaapi allocator in allocation query

In propose_allocation() if the numer of allocation params is zero, the
system's allocator is added first, and lastly the native VA-API
allocator.

In decide_allocation(), the allocations params in query are travered,
looking for a native VA-API allocator. If it is found, it is reused as
src pad allocator. Otherwise, a new allocator is instantiated and
appended in the query.
Comment 21 Víctor Manuel Jáquez Leal 2018-02-08 09:01:01 UTC
Created attachment 368131 [details] [review]
plugins: handle vaapi allocator in allocation query

In propose_allocation() if the numer of allocation params is zero, the
system's allocator is added first, and lastly the native VA-API
allocator.

In decide_allocation(), the allocations params in query are travered,
looking for a native VA-API allocator. If it is found, it is reused as
src pad allocator. Otherwise, a new allocator is instantiated and
appended in the query.
Comment 22 Víctor Manuel Jáquez Leal 2018-02-08 12:31:34 UTC
Attachment 367974 [details] pushed as 8855a92 - vaapivideomemory: remove unused macro
Attachment 368131 [details] pushed as 4481055 - plugins: handle vaapi allocator in allocation query
Comment 23 Víctor Manuel Jáquez Leal 2018-02-08 12:33:56 UTC
gdpdepay commits were also reverted
Comment 24 Víctor Manuel Jáquez Leal 2018-02-08 16:08:46 UTC
branch 1.12

* c0826fdc plugins: handle vaapi allocator in allocation query
* b31b773a vaapivideomemory: remove unused macro
Comment 25 Tomas Rataj 2018-02-27 17:35:32 UTC
This commit breaks vaapipostproc:

gst-launch-1.0 videotestsrc ! video/x-raw,format=BGRx ! vaapipostproc ! video/x-raw,format=NV12 ! fakesink

Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...
Got context from element 'vaapipostproc0': gst.gl.GLDisplay=context, gst.gl.GLDisplay=(GstGLDisplay)"\(GstGLDisplayX11\)\ gldisplayx11-0";
Got context from element 'vaapipostproc0': gst.vaapi.Display=context, gst.vaapi.Display=(GstVaapiDisplay)"\(GstVaapiDisplayDRM\)\ vaapidisplaydrm1";
Caught SIGSEGV
Comment 26 Víctor Manuel Jáquez Leal 2018-02-27 18:29:46 UTC
(In reply to Tomas Rataj from comment #25)
> This commit breaks vaapipostproc:
> 
> gst-launch-1.0 videotestsrc ! video/x-raw,format=BGRx ! vaapipostproc !
> video/x-raw,format=NV12 ! fakesink
> 
> Setting pipeline to PAUSED ...
> Pipeline is PREROLLING ...
> Got context from element 'vaapipostproc0': gst.gl.GLDisplay=context,
> gst.gl.GLDisplay=(GstGLDisplay)"\(GstGLDisplayX11\)\ gldisplayx11-0";
> Got context from element 'vaapipostproc0': gst.vaapi.Display=context,
> gst.vaapi.Display=(GstVaapiDisplay)"\(GstVaapiDisplayDRM\)\
> vaapidisplaydrm1";
> Caught SIGSEGV

Hi Tomas,

What backend are you using? libva version?
Why DRM display is used?
Can you upload the backtrace?
Comment 27 Tomas Rataj 2018-02-27 19:07:29 UTC
GstVideoInfo * old in gst_video_info_changed is NULL.

==11868== Thread 4 videotestsrc0:sr:
==11868== Invalid read of size 8
==11868==    at 0x7E745B0: gst_video_info_changed (gstvaapipluginutil.c:754)
==11868==    by 0x7E7EA60: gst_vaapi_video_buffer_pool_set_config (gstvaapivideobufferpool.c:174)
==11868==    by 0x4E79D01: gst_buffer_pool_set_config (gstbufferpool.c:687)
==11868==    by 0x7C279C3: gst_base_src_decide_allocation_default (gstbasesrc.c:3071)
==11868==    by 0x776FC61: gst_video_test_src_decide_allocation (gstvideotestsrc.c:860)
==11868==    by 0x7C2BC7F: gst_base_src_prepare_allocation (gstbasesrc.c:3134)
==11868==    by 0x7C2BC7F: gst_base_src_negotiate (gstbasesrc.c:3272)
==11868==    by 0x7C2BC7F: gst_base_src_loop (gstbasesrc.c:2691)
==11868==    by 0x4EDEB88: gst_task_func (gsttask.c:335)
==11868==    by 0x563800F: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.5400.3)
==11868==    by 0x5637644: ??? (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.5400.3)
==11868==    by 0x5CEC519: start_thread (pthread_create.c:465)
==11868==    by 0x5FF83EE: clone (clone.S:95)
==11868==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==11868== 
Caught SIGSEGV

libva info: VA-API version 1.0.0
libva info: va_getDriverName() returns 0
libva info: Trying to open /usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so
libva info: Found init function __vaDriverInit_1_0
libva info: va_openDriver() returns 0
vainfo: VA-API version: 1.0 (libva 2.0.0)
vainfo: Driver version: Intel i965 driver for Intel(R) Ivybridge Mobile - 2.0.0
vainfo: Supported profile and entrypoints
      VAProfileMPEG2Simple            :	VAEntrypointVLD
      VAProfileMPEG2Simple            :	VAEntrypointEncSlice
      VAProfileMPEG2Main              :	VAEntrypointVLD
      VAProfileMPEG2Main              :	VAEntrypointEncSlice
      VAProfileH264ConstrainedBaseline:	VAEntrypointVLD
      VAProfileH264ConstrainedBaseline:	VAEntrypointEncSlice
      VAProfileH264Main               :	VAEntrypointVLD
      VAProfileH264Main               :	VAEntrypointEncSlice
      VAProfileH264High               :	VAEntrypointVLD
      VAProfileH264High               :	VAEntrypointEncSlice
      VAProfileH264StereoHigh         :	VAEntrypointVLD
      VAProfileVC1Simple              :	VAEntrypointVLD
      VAProfileVC1Main                :	VAEntrypointVLD
      VAProfileVC1Advanced            :	VAEntrypointVLD
      VAProfileNone                   :	VAEntrypointVideoProc
      VAProfileJPEGBaseline           :	VAEntrypointVLD
Comment 28 Víctor Manuel Jáquez Leal 2018-03-01 23:52:09 UTC
Created attachment 369166 [details] [review]
vaapivideobufferpool: fix regression

The allocator in the config may be not VAAPI (videotestsrc, for
example) and it will not have a video info configuration.
Comment 29 Víctor Manuel Jáquez Leal 2018-03-01 23:54:30 UTC
pushed for branch 1.12:

* c00b35fe vaapivideobufferpool: fix regression