The backtrace is not the one of crash, plus you forgot you install debug symbols prior to producing it.

I get an internal flow error, and valgrind does not report anything untoward. Linux x86_64.

Try running with ORC_DEBUG=backup and see if this fixes the crash.

For me it just works.

Oh it does indeed. I'd left out the last line of the command when copy/pasting and did not notice.
No valgrind reports either with the full command.

Nicolas, I know that backtrace is surprisingly useless; I'm not sure why it fails where it does, but I do have debug symbols installed.

I've had a bit more of a play with this, and I've noticed that the problem is reproducible only when I use an xvimagesink (which is what autovideosink uses in my case); ximagesink and glimagesink work fine.

A more concise way of reproducing the problem is 

gst-launch-1.0 videotestsrc ! alpha ! videomixer ! videoconvert ! xvimagesink

Using this method, I get a useful backtrace (attached). It would appear this problem is related to a specific colour space conversion.

Created attachment 329918 [details]
Backtrace

If I build gst-plugins-base with orc disabled, I get a segfault in the same function. The rest of the backtrace is the same.

+ Trace 236362

Thanks, Which version of ORC are you running ?

I'm using 26e9721, which is basically 0.4.25. I'm not familiar with ORC - is it still used to generate code when I configure with --disable-orc?

Does it only happen in combination with videomixer? It probably shouldn't?

Also can you get a valgrind log of the problem?


Your pipeline above is working just fine for me here with GIT master and 1.8.2, and the latest orc release.

SIGILL hints you might be running this on an arch that doesn't have the capabilities the binary was built for.
Or maybe just running non code.

I'd expect --disable-orc to really disable orc. It's possible you're not running the newly built elements (ie, not installed, or using another copy found first) ? In any case, running with ORC_CODE=backup will run a C version instead (and not ORC_DEBUG=backup as I mistakenly said before).

When disabling ORC, we then use the orc generated software backup same as ORC_CODE=backup iirc. There should be no SIMD in there though.

Created attachment 330039 [details]
valgrind log for 1.8.2, ORC enabled

Valgrind log obtained by running the following, using 1.8.2

valgrind gst-launch-1.0 videotestsrc ! alpha  ! videoconvert ! xvimagesink

Created attachment 330040 [details]
valgrind log for 1.8.2, ORC disbled

Valgrind log obtained by running the following, using 1.8.2

ORC_CODE=backup valgrind gst-launch-1.0 videotestsrc ! alpha  ! videoconvert ! xvimagesink

Sebastian, you're correct regarding the videomixer - the problem occurs when I remove it.

I've just gotten a new machine; it's running Ubuntu 16.04 and so has gstreamer-1.8.1 installed. I've reproduced the problem using the vanilla installation. I've also built 1.8.2 from source, using ORC master, and verified that the problem occurs with that code-base as well.

I'm mystified that no-one else can reproduce this one...

Does it only happen with xvimagesink? Try fakesink or glimagesink for example

Yep - it's specific to xvimagesink. I've tested with ximagesink and glimagesink, and the problem doesn't occur in those cases.

Might be a problem with your xv driver allocating wrongly aligned memory, or with wrong strides. Can you get a debug log with all the xv related output?

Created attachment 330176 [details]
xvimagesink + gdb log

Sebastian,

I've attached a log captured with GST_DEBUG=xvimagesink:7,xvimagepool:7,videoconvert:7,xvimageallocator:7,xcontext:7. The log also contains the output of a short gdb debug session, which hopefully might contain some more useful information to help understand this.

I haven't dug further, as I only came across this problem by accident, and don't have the time to spend understanding it properly. I am happy to assist anyone who wants to debug however.

0:00:00.054214202 [332m 9733[00m       0x7cf2d0 [33;01mLOG    [00m [00m    xvimageallocator xvimageallocator.c:400:gst_xvimage_allocator_alloc:<xvimageallocator0>[00m XShm image size is 4

This is the problem. It allocates a 4 byte image for a 320x240 BGRx frame. Your XV driver is broken

There is some sanity checking for the allocation size in gst_xvimage_allocator_alloc(), but only for a couple of YUV formats. This should probably be extended so that it errors out properly instead of crashing.

Duncan, could you work on that?

Possibly the same bug as https://bugzilla.gnome.org/show_bug.cgi?id=761505

(In reply to Sebastian Dröge (slomo) from comment #21)
> There is some sanity checking for the allocation size in
> gst_xvimage_allocator_alloc(), but only for a couple of YUV formats. This
> should probably be extended so that it errors out properly instead of
> crashing.
> 
> Duncan, could you work on that?

Yep, I can do that.

(In reply to Vincent Penquerc'h from comment #22)
> Possibly the same bug as https://bugzilla.gnome.org/show_bug.cgi?id=761505

It certainly looks like it. xvinfo in my case gives the same output as James reports in that issue.

Created attachment 330629 [details] [review]
Error out on sanity check failure, and add checking for RGB pixel formats

This patch causes gst_xvimage_allocator_alloc() to call on g_set_error() on buffer size sanity check failure, rather than just logging a warning. I've also added sanity checking for RGB pixel formats (note that these can't be derived from im_format using the FOURCC notation).

I pass a *GstVideoFormatInfo into gst_xvimage_allocator_alloc() to facilitate the extra sanity check. If the function is called from gst_xvimage_memory_copy(), then that pointer will be NULL, and the function won't do sanity checking for RGB pixel formats. I think this is likely ok, as the bug as it is occurs on initial allocation, and this saves us from having to store extra info in the GstXvImageMemory struct solely for sanity checking.

I've testing this with the YUV pixel formats supported by xv on my machine, but there are no working RGB formats on any machine I have access to. So, please take a look at my expected_size calculations for RGB and let me know if you see any issues.

*** Bug 761505 has been marked as a duplicate of this bug. ***

Review of attachment 330629 [details] [review]:

I think it would be a good idea to just store the GstVideoInfo (not just the GstVideoFormatInfo) in the xv allocator, it will be useful to have sooner or later and makes this NULL go away :)

Should probably also get some more sanity checks for other supported YUV formats (these are not all, right?), but that can be done when needed or later.

::: sys/xvimage/xvimageallocator.c
@@ +405,3 @@
+      switch (im_format) {
+        case GST_MAKE_FOURCC ('I', '4', '2', '0'):
+        case GST_MAKE_FOURCC ('Y', 'V', '1', '2'):

We could also consistently go via the GstVideoFormat stored in it, instead of having two different code paths

@@ +438,3 @@
+    } else if (GST_VIDEO_FORMAT_INFO_IS_RGB (info)) {
+      expected_size = padded_height * padded_width *
+          GST_VIDEO_FORMAT_INFO_PSTRIDE (info, 0);

My guess is that this should be "padded_height * GST_ROUND_UP_4(padded_width * pstride)"

Thanks Sebastian,

I'll update the patch with your suggestions this week.

(In reply to Sebastian Dröge (slomo) from comment #27)
> 
> @@ +438,3 @@
> +    } else if (GST_VIDEO_FORMAT_INFO_IS_RGB (info)) {
> +      expected_size = padded_height * padded_width *
> +          GST_VIDEO_FORMAT_INFO_PSTRIDE (info, 0);
> 
> My guess is that this should be "padded_height * GST_ROUND_UP_4(padded_width
> * pstride)"

I'm not sure if your suggestion is correct e.g. if we're using GST_VIDEO_FORMAT_RGB, where pstride[0] == 3. In this case, I would have though that the stride should be width * 3. Have I got this wrong?

Dunk

It's usually GST_ROUND_UP_4(width*3) then, IIRC XV has all strides rounded up to a multiple of 4 bytes.

Created attachment 330994 [details] [review]
Error out on sanity check failure, add checking for RGB pixel formats

I've implemented all the suggestions from Sebastians review here. After a bit of reading, I now understand why you're saying the stride is rounded up to 4 pixels.

Review of attachment 330994 [details] [review]:

::: sys/xvimage/xvimageallocator.c
@@ +344,3 @@
 gst_xvimage_allocator_alloc (GstXvImageAllocator * allocator, gint im_format,
+    const GstVideoInfo * info, gint padded_width, gint padded_height,
+    GstVideoRectangle * crop, GError ** error)

Might want to make the crop rectangle also const ;)

@@ +440,2 @@
       }
+    } else if (GST_VIDEO_FORMAT_INFO_IS_RGB (info->finfo)) {

This will break with the GBR (planar RGB) formats, maybe check for a single plane in addition to this.

Created attachment 331685 [details] [review]
Error out on sanity check failure, add checking for RGB pixel formats

Thanks Sebastian. I didn't realise there were planar RGB formats. Updated patch attached.

Created attachment 331686 [details] [review]
Const correctness for gst_xvimage_allocator_alloc()

Make the crop rectangle parameter const.

Comment on attachment 331685 [details] [review]
Error out on sanity check failure, add checking for RGB pixel formats

Thanks!

>+    if (expected_size != 0 && mem->xvimage->data_size != expected_size)
>+      goto unexpected_size;

Just wondering if this shouldn't be

  if (data_size < expected_size)

instead?

The original check was for inequality, however I'm happy to change it if you'd prefer? It would potentially make this code a little bit more robust.

I think since we error out now it's probably better to only do so if the memory is smaller than we (and all other elements) expect, so changed that on top of your patches.

commit 3273714e0a9d41d86d0013e6d6d4655162ba6da0
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Mon Jul 18 14:20:11 2016 +0100

    xvimagesink: only error out if the allocated memory is too small
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767712

commit f00bbd2ea543e55c2b38ed80e8493e3119c9eb58
Author: Duncan Palmer <dpalmer@digisoft.tv>
Date:   Mon Jul 18 19:59:23 2016 +1000

    xvimageallocator: const correctness in gst_xvimage_allocator_alloc().
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767712

commit 4e83e894dfd2bf0e52e826dd42a93f0bed61ab4f
Author: Duncan Palmer <dpalmer@digisoft.tv>
Date:   Thu Jul 7 22:27:15 2016 +1000

    xvimagesink: error out on buffer size sanity check failure.
    
    If sanity checks on the buffer size allocated by XvShmCreateImage() fail,
    call on g_set_error(), rather than just logging a warning, as this
    failure is fatal.
    
    Add a sanity check on buffer size when the video format is RGB. This adds to
    existing checks on various YUV pixel formats.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=767712

Thanks!

Thanks Tim!

Hello I confirm I have this bug also. I can avoid it with other sinks like mentioned earlier. Maybe we should take this to the X developers.

That'd be great Paulo :) Probably needs someone to run into this with a fairly recent version of things though I think.