GNOME Bugzilla – Bug 733717
glmemory allocate size didn't match video_orc_pack_I420 needs if height is odd
Last modified: 2014-07-31 05:22:19 UTC
(tested with ORC disabled ) gst-launch-1.0 videotestsrc ! video/x-raw, format=\(string\)I420,width=512,height=11 ! glimagesink just crash gst-launch-1.0 videotestsrc ! video/x-raw, format=\(string\)I420,width=512,height=11 ! videoconvert ! ximagesink works OK I found that with ximagesink gst_video_test_src_fill will be feed with a GstBuffer n_memery=1 size=9216 but with glimagesink GstBuffer n_memory=3 size=8704 (512*11 + 256*6 + 256*6 allocated in _gl_mem_init) if height is even ,everything is OK clang asan report as follows: ==7456==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x9480a080 at pc 0xb67fecfa bp 0x96b787b8 sp 0x96b787b0 WRITE of size 1 at 0x9480a080 thread T1 (videotestsrc0:s) #0 0xb67fecf9 in video_orc_pack_I420 /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst-libs/gst/video/tmp-orc.c:1137 #1 0xb65d9a8b in pack_planar_420 /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst-libs/gst/video/../../../../gst-libs/gst/video/video-format.c:102 #2 0x9b0a2b0b in convert_hline_generic /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/videotestsrc.c:1202 #3 0x9b079525 in videotestsrc_convert_tmpline /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/videotestsrc.c:275 #4 0x9b073793 in gst_video_test_src_smpte /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/videotestsrc.c:423 #5 0x9b069ee5 in gst_video_test_src_fill /home/wangxinyu/project/gstreamer/gst-plugins-base/asanbuild/gst/videotestsrc/../../../gst/videotestsrc/gstvideotestsrc.c:929 #6 0x9e5bf298 in gst_push_src_fill /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:167 #7 0x9e4a99b2 in gst_base_src_default_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:1471 #8 0x9e5be00f in gst_push_src_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:133 #9 0x9e47ade6 in gst_base_src_get_range /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2445 #10 0x9e471ccc in gst_base_src_loop /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2721 #11 0xb6eb34a3 in gst_task_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttask.c:317 #12 0xb6ebba83 in default_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttaskpool.c:68 #13 0xb64a9424 in g_thread_pool_new ??:? #14 0xb64a89c9 in g_test_get_filename ??:? #15 0x80bd606 in _ZN6__asan10AsanThread11ThreadStartEm ??:? #16 0x809c45d in _ZL17asan_thread_startPv asan_interceptors.o:? #17 0xb616af6f in start_thread /build/buildd/eglibc-2.19/nptl/pthread_create.c:312 (discriminator 1) #18 0x9e70a70d in clone /build/buildd/eglibc-2.19/misc/../sysdeps/unix/sysv/linux/i386/clone.S:129 0x9480a080 is located 0 bytes to the right of 1536-byte region [0x94809a80,0x9480a080) allocated by thread T1 (videotestsrc0:s) here: #0 0x80b3a69 in __interceptor_malloc ??:? #1 0xb6487be2 in g_malloc ??:? #2 0xb724ba85 in gst_gl_memory_setup_buffer /home/wangxinyu/project/gstreamer/gst-plugins-bad/gst-libs/gst/gl/gstglmemory.c:1135 #3 0xb7262ca6 in gst_gl_buffer_pool_alloc /home/wangxinyu/project/gstreamer/gst-plugins-bad/gst-libs/gst/gl/gstglbufferpool.c:211 #4 0xb69b9f9b in do_alloc_buffer /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gstbufferpool.c:267 #5 0xb69b617f in default_acquire_buffer /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gstbufferpool.c:1098 #6 0xb726396f in gst_gl_buffer_pool_acquire_buffer /home/wangxinyu/project/gstreamer/gst-plugins-bad/gst-libs/gst/gl/gstglbufferpool.c:250 #7 0xb69aecef in gst_buffer_pool_acquire_buffer /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gstbufferpool.c:1206 #8 0x9e4aab83 in gst_base_src_default_alloc /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:1422 #9 0x9e5bea7f in gst_push_src_alloc /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:151 #10 0x9e4a950a in gst_base_src_default_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:1462 #11 0x9e5be00f in gst_push_src_create /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstpushsrc.c:133 #12 0x9e47ade6 in gst_base_src_get_range /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2445 #13 0x9e471ccc in gst_base_src_loop /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/libs/gst/base/../../../../libs/gst/base/gstbasesrc.c:2721 #14 0xb6eb34a3 in gst_task_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttask.c:317 #15 0xb6ebba83 in default_func /home/wangxinyu/project/gstreamer/gstreamer-git/asanbuild/gst/../../gst/gsttaskpool.c:68 #16 0xb64a9424 in g_thread_pool_new ??:? Thread T1 (videotestsrc0:s) created by T0 here: #0 0x809c2fe in pthread_create ??:? #1 0xb64c77df in g_private_replace ??:? SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ?? Shadow bytes around the buggy address: 0x329013c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x329013d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x329013e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x329013f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x32901400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x32901410:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x32901420: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x32901430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x32901440: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x32901450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x32901460: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Contiguous container OOB:fc ASan internal: fe ==7456==ABORTING
Ok, so videotestsrc seems to write a 12th Y line of zero's that are never used and GLMemory does not allocate space for.
That's wrong, and also: gst_video_info_set_format (&info, GST_VIDEO_FORMAT_I420, 512, 11); g_print ("%d %d %d\n", GST_VIDEO_INFO_COMP_HEIGHT (&info, 0), GST_VIDEO_INFO_COMP_HEIGHT (&info, 1), GST_VIDEO_INFO_COMP_HEIGHT (&info, 2)); g_print ("%d\n", info.size); outputs: 11 6 6 9216 So if videotestsrc writes a 12th line that would be wrong. But the size it uses is correct (and the one used by gl is wrong). Note the calculations in video-info.c:fill_planes() info->stride[0] = GST_ROUND_UP_4 (width); info->stride[1] = GST_ROUND_UP_4 (GST_ROUND_UP_2 (width) / 2); info->stride[2] = info->stride[1]; info->offset[0] = 0; info->offset[1] = info->stride[0] * GST_ROUND_UP_2 (height); info->offset[2] = info->offset[1] + info->stride[1] * (GST_ROUND_UP_2 (height) / 2); info->size = info->offset[2] + info->stride[2] * (GST_ROUND_UP_2 (height) / 2); We are probably missing the rounding up for the height in gl.
Any rational not to use GstVideoInfo on the GL side ? I suppose it's not a big problem if the plane is bigger then what GL is going to write to.
(In reply to comment #3) > Any rational not to use GstVideoInfo on the GL side ? I suppose it's not a big > problem if the plane is bigger then what GL is going to write to. We already are using GstVideoInfo. Each plane has a GLMemory. The difference is that GST_VIDEO_INFO_COMP_HEIGHT(&info, 0) returns the 11 for the actual height and so GLMemory allocates a sysmem data plane with 11 Y lines whereas the [un]pack video code expects 12 addressable lines. One simple hack is to always allocate an extra line or two to avoid these issues. Another would involve adding an extra data_height parameter to all the GLMemory creation routines (I have half a patch for this already). A third viable option is to add this data size (or height) information to GstVideoInfo/GstVideoFormat itself although I assume that the API is attempting to avoid exposing that for multiview/3D purposes.
Created attachment 281897 [details] [review] glmemory: use GstVideoInfo everywhere Fourth option, make GLMemory use GstVideoInfo everywhere and patch the GST_VIDEO_INFO_COMP_HEIGHT values for the sysmem data height.
The problem is not the component heights, but that plane offsets. With 11 lines there's an extra line of padding between the Y and U plane (into which videotestsrc should also not write).
Indeed, to get the aligned default size of a plane, you need to substract the offsets. Careful, because offset zero may not be zero in certain HW context. /* For planar format, 3 planes */ plane1_size = offset[1] - offset[0]; plane2_size = offset[2] - offset[1]; plane3_size = (offset[0] + size) - offset[2];
commit acb38a2a79588e223b7227638c7bd36590d0bfb1 Author: Matthew Waters <ystreet00@gmail.com> Date: Thu Jul 31 15:18:04 2014 +1000 glmemory: use the plane offsets to compute the size of the data pointer Certain elements expect that there be a certain number of lines that they can write into. e.g. for odd heights, I420, YV12, NV12, NV21 (and others) Y lines are expected to have exactly twice the number of U/UV lines. https://bugzilla.gnome.org/show_bug.cgi?id=733717 commit 89636392fa3fd2182dd13aa2193aca17edaa102d Author: Matthew Waters <ystreet00@gmail.com> Date: Thu Jul 31 14:07:29 2014 +1000 glmemory: use GstVideoInfo everywhere Simplifies a lot of the calling code https://bugzilla.gnome.org/show_bug.cgi?id=733717