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 766940 - vaapipostproc: race condition when updating src caps
vaapipostproc: race condition when updating src caps
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
1.8.2
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-27 12:03 UTC by Thomas Scheuermann
Modified: 2016-10-31 14:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Reproducer of the bug (11.08 KB, text/x-csrc)
2016-05-27 12:03 UTC, Thomas Scheuermann
  Details
[PATCH] GstVaapiPostproc: take ref on srccaps during transform (1.60 KB, patch)
2016-06-07 04:21 UTC, Scott D Phillips
none Details | Review
[PATCH] vaapipostproc: add postproc_lock to protect data members (5.67 KB, patch)
2016-06-07 21:32 UTC, Scott D Phillips
committed Details | Review

Description Thomas Scheuermann 2016-05-27 12:03:02 UTC
Created attachment 328633 [details]
Reproducer of the bug

When I continuously resize a running pipeline which decodes an H264 stream from the network, the pipline crashes after some time below libgstvaapi.so. If I don't use vaapi elements, the pipeline doesn't crash.
I've written a reproducer which generate an H264 stream with the following pipeline:
gst-launch-1.0 videotestsrc ! video/x-raw,width=1920,height=1080 ! vaapih264enc ! h264parse config-interval=2 ! rtph264pay ! udpsink
The program opens an X11 window which displays the video and always resizes it. After some time the program crashes.
The decoding pipeline which is used looks like:
udpsrc ! application/x-rtp,encoding-name=H264 ! rtph264depay ! decodebin ! vaapipostproc ! capsfilter ! appsink
The resizing is done by inserting a probe at the source pad of decodebin and in the callback width and height of the capsfilter and vaapipostproc are changed.

Regards,
Thomas
Comment 1 Víctor Manuel Jáquez Leal 2016-06-03 15:09:50 UTC
Note to myself:

To compile this 

gcc test.c -o test `jhbuild run pkg-config --cflags --libs gstreamer-1.0 gstreamer-app-1.0 gstreamer-video-1.0 x11`

Now, I have tested the code and the code crashes in different places randomly, with several warning in the pad probes. I don't know what exactly to do. 

If you could to corner the issue would be very helpful.
Comment 2 Thomas Scheuermann 2016-06-06 15:02:08 UTC
To compile the source I use:

gcc -o livescale livescale.c -g `pkg-config --libs --cflags gstreamer-1.0 gstreamer-video-1.0 gstreamer-app-1.0` -Wall -lX11

here is the backtrace of the crash. It looks always the same and happens only if vaapi is involved:

(gdb) bt
  • #0 g_type_check_value
    at /home/barco/glib2.0-2.46.0/./gobject/gtype.c line 4201
  • #1 gst_value_init_and_copy
    at gstvalue.c line 5227
  • #2 copy_garray_of_gstvalue
    at gstvalue.c line 296
  • #3 gst_value_copy_list_or_array
    at gstvalue.c line 307
  • #4 gst_structure_copy
    at gststructure.c line 353
  • #5 _gst_caps_copy
    at gstcaps.c line 174
  • #6 0x00007ffff30fa4e3 in
  • #7 0x00007ffff30fa58e in
  • #8 gst_base_transform_transform_caps
  • #9 gst_base_transform_default_query
    at gstbasetransform.c line 757
  • #10 gst_base_transform_default_query
  • #11 gst_pad_query
    at gstpad.c line 3921
  • #12 gst_pad_peer_query
    at gstpad.c line 4053
  • #13 query_caps_func
    at gstutils.c line 2557
  • #14 gst_pad_forward
    at gstpad.c line 2935
  • #15 gst_pad_proxy_query_caps
    at gstutils.c line 2607
  • #16 gst_pad_query_default
    at gstpad.c line 3113
  • #17 gst_pad_query_default
    at gstpad.c line 3341
  • #18 0x00007ffff339c956 in
  • #19 gst_pad_query
    at gstpad.c line 3921
  • #20 gst_pad_peer_query
    at gstpad.c line 4053
  • #21 query_caps_func
    at gstutils.c line 2557
  • #22 gst_pad_forward
    at gstpad.c line 2935
  • #23 gst_pad_proxy_query_caps
    at gstutils.c line 2607
  • #24 gst_pad_query_default
    at gstpad.c line 3113
  • #25 gst_pad_query_default
    at gstpad.c line 3341
  • #26 gst_pad_query
    at gstpad.c line 3921
  • #27 gst_pad_peer_query
    at gstpad.c line 4053
  • #28 gst_pad_peer_query_caps
    at gstutils.c line 2887
  • #29 0x00007ffff30f8610 in
  • #30 0x00007ffff30f775b in
  • #31 gst_base_transform_setcaps
    at gstbasetransform.c line 1012
  • #32 gst_base_transform_setcaps
    at gstbasetransform.c line 1392
  • #33 gst_base_transform_reconfigure
    at gstbasetransform.c line 1472
  • #34 default_submit_input_buffer
    at gstbasetransform.c line 2026
  • #35 gst_base_transform_chain
    at gstbasetransform.c line 2326
  • #36 gst_pad_push_data
    at gstpad.c line 4176
  • #37 gst_pad_push_data
    at gstpad.c line 4428
  • #38 gst_pad_push
    at gstpad.c line 4547
  • #39 gst_queue_loop
    at gstqueue.c line 1356
  • #40 gst_queue_loop
    at gstqueue.c line 1503
  • #41 gst_task_func
    at gsttask.c line 332
  • #42 g_thread_pool_thread_proxy
    at /home/barco/glib2.0-2.46.0/./glib/gthreadpool.c line 307
  • #43 g_thread_proxy
    at /home/barco/glib2.0-2.46.0/./glib/gthread.c line 778
  • #44 start_thread
    at pthread_create.c line 309
  • #45 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 111

Comment 3 Scott D Phillips 2016-06-07 04:21:17 UTC
Created attachment 329236 [details] [review]
[PATCH] GstVaapiPostproc: take ref on srccaps during transform

There seems to be a race between gst_vaapipostproc_set_caps which releases postproc->allowed_srcpad_caps and gst_vaapipostproc_transform_caps which uses postproc->allowed_srcpad_caps. To see that this is in fact the case, here is a patch which takes a ref to the allowed caps in transform. With the patch I was able to run the test program for 4 hours without crashing.

I believe the correct fix to the issue here is to add locking around the access to all of postproc's data members (there doesn't seem to be any locking in postproc currently).
Comment 4 Víctor Manuel Jáquez Leal 2016-06-07 08:06:24 UTC
(In reply to Scott D Phillips from comment #3)
> Created attachment 329236 [details] [review] [review]
> [PATCH] GstVaapiPostproc: take ref on srccaps during transform

Nice! Thanks Scott!
Comment 5 Víctor Manuel Jáquez Leal 2016-06-07 09:58:18 UTC
(In reply to Scott D Phillips from comment #3)

> I believe the correct fix to the issue here is to add locking around the
> access to all of postproc's data members (there doesn't seem to be any
> locking in postproc currently).

Are you going to work on it?

Perhaps using GST_OBJECT_LOCK() would be enough.
Comment 6 Scott D Phillips 2016-06-07 17:22:37 UTC
(In reply to Víctor Manuel Jáquez Leal from comment #5)
> Are you going to work on it?
> 
> Perhaps using GST_OBJECT_LOCK() would be enough.

Yep, I'll take a crack at it today.
Comment 7 Scott D Phillips 2016-06-07 21:32:18 UTC
Created attachment 329345 [details] [review]
[PATCH] vaapipostproc: add postproc_lock to protect data members

(In reply to Víctor Manuel Jáquez Leal from comment #5)
> 
> Perhaps using GST_OBJECT_LOCK() would be enough.

In the end locking the element was going to be more complicated because ensure_display() also wants to lock the element when iterating over pads during the context query.

So my patch introduces a new mutex instead (which seems to be what the other transform elements do for their caps transforming/setting functions).
Comment 8 Víctor Manuel Jáquez Leal 2016-06-08 08:09:35 UTC
Comment on attachment 329345 [details] [review]
[PATCH] vaapipostproc: add postproc_lock to protect data members

LGTM