GNOME Bugzilla – Bug 761163
videocrop: Access violation reading
Last modified: 2018-11-03 15:07:31 UTC
1. DESCRIPTION: If set properties (left, top, right, bottom) for videocrop while pipeline playing, it crashes sometimes with "Access violation reading". 2. HOW TO REPRODUCE: I ofen reproduce this bug with next several elements: videotestsrc ! video/x-raw, framerate=25/1, width=1920, height=1088 ! videocrop ! autovideosink When the pipeline is on PLAYING state I set random values for videocrop properties (left, top, right, bottom) several times per second. After some time program crashes with "Access violation reading". It can be reproduced only with one setting while playing. But that rarely happens. It is reproduced on RGB and YUV frame format both. 3. STACK msvcrt.dll!000007fefea511be() libgstvideocrop.dll!00000000150e1e1c() libgstvideo-1.0-0.dll!000000006d41d61f() libgstbase-1.0-0.dll!00000000198e6930() libgstbase-1.0-0.dll!00000000198e63ba() libgstreamer-1.0-0.dll!0000000061481054() libgstbase-1.0-0.dll!00000000198e638d() libgstreamer-1.0-0.dll!0000000061481054() libgstbase-1.0-0.dll!00000000198e2436() libgstreamer-1.0-0.dll!00000000614af514() libglib-2.0-0.dll!00000000686154b5() libglib-2.0-0.dll!0000000068614cc9() libglib-2.0-0.dll!000000006863103a() [External Code]
One more thing to add: I use crop values from intervals: left=right=[0, width/2-1], top=bottom=[0, height/2-1]
Can you produce a simple C program to reproduce ? I believe we should try and fix that crash, though the provided stack trace does not help in any ways. To produce a backtrace, you need to use gdb for window. It's known the videocrop is not safe to live changes of the crop properties. There is some way around, like only settings the properties from a probe on the same streaming thread.
Created attachment 319893 [details] a simple Java code to reproduce
(In reply to Nicolas Dufresne (stormer) from comment #2) > Can you produce a simple C program to reproduce ? I believe we should try > and fix that crash, though the provided stack trace does not help in any > ways. To produce a backtrace, you need to use gdb for window. > > It's known the videocrop is not safe to live changes of the crop properties. > There is some way around, like only settings the properties from a probe on > the same streaming thread. I've attached a simple Java code to reproduce. I hope it's not hard to rewrite it into C program.
(In reply to Nicolas Dufresne (stormer) from comment #2) Thank you Nicolas for the advice about setting the properties on the videocrop streaming thread. My test program has been running for several hours without crashing.
Created attachment 320156 [details] C code to produce Also reproduced on Linux.
Crash in a memcpy in here, as expected, some dimension get changed regardless of the state.
+ Trace 235941
Created attachment 320865 [details] [review] fix crash when setting properties at runtime
Comment on attachment 320865 [details] [review] fix crash when setting properties at runtime This looks weird to me, I don't think it should be needed if everything else is done correctly? So what's actually going wrong where?
AFAICT, that buffer is allocated before with the crop properties that were set at the time. Not having this failsafe wound mean the caller would have to ensure the properties could not change between the allocation of the frame and the actual use of it (ie, lock the videocrop's lock itself, which is iffy too).
In _transform_frame, can move the call to _set_info after the blitting, which avoids most of the crashes, but not all: _set_info is not only called from _transform_frame, but as a virtual function, and videocrop doesn't have control over when this is called. I suppose one could try to change _set_info to only schedule che change, and perform it only on the next call to _transform_frame (after the blitting). Since _set_info calls gst_base_transform_set_passthrough, this would also delay that, which might or might not cause trouble but it seems it's worth a shot. I'll try that.
Note, my last analyses of this problem was that this is inherently racy. The same information is used and updated from tranform_caps(), set_caps() and transform_frame(). Those calls are not atomic, and the problem arrive when the properties get changed randomly between the transform_caps() call (sometimes there is more then one) and the set_caps() calls. Ideally, we should have some code that queue the property changes, and find a way to keep the state coherent. Also, because each property is set seperatly (and even if you use a single g_object_set() we will treat them seperatly) it is also a bit racy, and may always make the negotiation fail. I kept this bug mostly to check if we could remove the crash, but I'm not sure we can really solve the race. We could improve it, like using dispatch_properties_changed().
There seems to be some code that tries to queue the property changes (prop_foo -> crop_foo) and only update them from within transform_frame(). Perhaps transform_caps() should use _crop_foo() instead? (But either way there'll be some raciness as is the case in all renegotiation, so question is what to do then I suppose)
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/254.