GNOME Bugzilla – Bug 744246
gl: Broken VideoAlignment handling?
Last modified: 2015-04-28 10:49:05 UTC
When using latest GIT master on Android with glimagesink and avviddec_h264, the following crash usually happens. Putting a tee before glimagesink (which makes the ALLOCATION query fail and fall back to not use VideoAlignment or anything) makes it work again. Crash is a SIGBUS btw, so we probably need to align memory differently somehow to make libav happy. I/DEBUG ( 2468): #00 pc 00653d1c /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so (ff_pred16x16_dc_neon+111) I/DEBUG ( 2468): #01 pc 0044d3ad /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #02 pc 00458645 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #03 pc 0045b3eb /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so (ff_h264_execute_decode_slices+634) I/DEBUG ( 2468): #04 pc 00435307 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #05 pc 005a2f6f /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so (avcodec_decode_video2+146) I/DEBUG ( 2468): #06 pc 00359cd1 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #07 pc 0035b1cd /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #08 pc 00f30949 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #09 pc 00f32b67 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #10 pc 00f33b3b /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #11 pc 00fd1ea7 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #12 pc 00fd335d /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #13 pc 00f9b5a1 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #14 pc 00fd1ea7 /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #15 pc 00fd335d /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so I/DEBUG ( 2468): #16 pc 00f83dbd /data/app-lib/com.centricular.android_launch-2/libgstreamer_android.so (gst_base_parse_push_frame+2788)
Could you provide a proper backtrace ? If I remember well, all you have to do is run ndk-build NDK_DEBUG=1 and then re-install. Then start the app, it will wait for you to run ndk-gdb.
This is also the cause of flickering in glimagesink from the use of old frames. placing a tee in between libav and glimagesink or dropping the exposure of GstVideoAlignment from the GL bufferpool config fixes this.
(In reply to Matthew Waters from comment #2) > This is also the cause of flickering in glimagesink from the use of old > frames. placing a tee in between libav and glimagesink or dropping the > exposure of GstVideoAlignment from the GL bufferpool config fixes this. I can't reproduce the flickering here, is this also only on Android ? Do you have a pipeline and a media to produce ? A flickering would most likely point to another bug. If that bug is on gst-libav side, the same issue shall be visible on xvimagesink, so it can be useful to test that too (if not Android here). What we noted today, is that we ignore the allocation parameter. Instead we blindly do a g_try_malloc(). Libav also set requirement on the allocated pointer alignment. This is a very plausible cause of crash in neon optimized code.
Either on OS X by default or with playbin video-sink=navseek ! glimagesink on linux reproduces for me.
Ok, I see the flickering now, this is a recent regression since it worked with playbin before. Clearly not the same bug.
Created attachment 297935 [details] [review] [PATCH] glmemory: Add GstAllocationParams and alignment support This implements support for GstAllocationParams and memory alignments. The parameters where simply ignored which could lead to crash on certain platform when used with libav and no luck. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- ext/gl/gstgloverlay.c | 6 ++- gst-libs/gst/gl/gstglbufferpool.c | 3 +- gst-libs/gst/gl/gstglcolorconvert.c | 5 ++- gst-libs/gst/gl/gstglmemory.c | 81 ++++++++++++++++++++++++------------- gst-libs/gst/gl/gstglmemory.h | 9 +++-- gst-libs/gst/gl/gstglupload.c | 4 +- 6 files changed, 69 insertions(+), 39 deletions(-)
Note this patch have had very little testing so far and haven't been tested on Android yet.
Review of attachment 297935 [details] [review]: Looks good to me
There was one concern raised yesterday on IRC, the massive amount of parameters in our GL memory allocation function. So I'd like to share a proposed solution. I would create a new public structure, GstGLAllocationParams. This structure could contain GstAllocationParams, GstVideoInfo and GstVideoAlignment. Would remain to decide if pass these as pointers or not. But it would make it easier to extend if needed later. Do you think it's a good idea ?
Review of attachment 297935 [details] [review]: Seems to be missing the GL side of things. ::: gst-libs/gst/gl/gstglmemory.c @@ -727,3 @@ mem->notify = notify; mem->user_data = user_data; - mem->data_wrapped = FALSE; Curious as to why you remove this line? @@ +826,3 @@ } + data = (guint8 *) gl_mem->data + gl_mem->mem.offset; You seem to be missing this computation for the GL case. i.e. the data parameter to glTexSubImage2D inside _upload_memory and glReadPixels for _download_memory.
I like the idea of a GstGLAllocationParameters to simplify the number of parameters to the GL memory allocation functions. Extensibility is also a plus.
(In reply to Matthew Waters from comment #10) > Review of attachment 297935 [details] [review] [review]: > > Seems to be missing the GL side of things. > > ::: gst-libs/gst/gl/gstglmemory.c > @@ -727,3 @@ > mem->notify = notify; > mem->user_data = user_data; > - mem->data_wrapped = FALSE; > > Curious as to why you remove this line? We now have an allocated data pointer, and a data pointer. Remembering if the data pointer was allocated of not is no longer needed. > > @@ +826,3 @@ > } > > + data = (guint8 *) gl_mem->data + gl_mem->mem.offset; > > You seem to be missing this computation for the GL case. i.e. the data > parameter to glTexSubImage2D inside _upload_memory and glReadPixels for > _download_memory. Ok, I'll check.
Review of attachment 297935 [details] [review]: Changing to need work, as you don't seem to reject the change ;-P ::: gst-libs/gst/gl/gstglmemory.h @@ +96,3 @@ gpointer data; gboolean data_wrapped; I forgot to remove that line actually.
Created attachment 298170 [details] [review] [PATCH] glmemory: Add GstAllocationParams and alignment support This implements support for GstAllocationParams and memory alignments. The parameters where simply ignored which could lead to crash on certain platform when used with libav and no luck. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- ext/gl/gstgloverlay.c | 6 +- gst-libs/gst/gl/gstglbufferpool.c | 3 +- gst-libs/gst/gl/gstglcolorconvert.c | 5 +- gst-libs/gst/gl/gstglmemory.c | 135 ++++++++++++++++++++++-------------- gst-libs/gst/gl/gstglmemory.h | 10 +-- gst-libs/gst/gl/gstglupload.c | 4 +- 6 files changed, 99 insertions(+), 64 deletions(-)
Created attachment 298171 [details] [review] [PATCH] glmemory: Use fallback for partial copy When the memory is partial copy, the texture size and videoinfo no longer make sense. As we cannot guess what the application wants, we safely copy into a sysmem memory. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 20 +++++++++++++++++--- gst-libs/gst/gl/gstglmemory.h | 1 + 2 files changed, 18 insertions(+), 3 deletions(-)
Created attachment 298175 [details] [review] [PATCH] glmemory: Provide correct size on download Provide the right size to GL when downloading. This fixes downloading from GLMemory that where created for libav. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Created attachment 298176 [details] [review] [PATCH] glmemory: Provide correct size on upload Provide the right size to GL when uploading. Using maxsize is wrong since we offset the data point with the memory offset and video alignement offset. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Review of attachment 298170 [details] [review]: ::: gst-libs/gst/gl/gstglmemory.c @@ +852,3 @@ } + data = (guint8 *) gl_mem->data + gl_mem->mem.offset; Oops, this change is wrong. gst_memory_map() take care of applying the offset.
Created attachment 298177 [details] [review] [PATCH] glmemory: Use fallback for partial copy When the memory is partial copy, the texture size and videoinfo no longer make sense. As we cannot guess what the application wants, we safely copy into a sysmem memory. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 20 +++++++++++++++++--- gst-libs/gst/gl/gstglmemory.h | 1 + 2 files changed, 18 insertions(+), 3 deletions(-)
Created attachment 298178 [details] [review] [PATCH] glmemory: Provide correct size on download Provide the right size to GL when downloading. This fixes downloading from GLMemory that where created for libav. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Created attachment 298179 [details] [review] [PATCH] glmemory: Provide correct size on upload Provide the right size to GL when uploading. Using maxsize is wrong since we offset the data point with the memory offset and video alignement offset. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Created attachment 298180 [details] [review] [PATCH] glmemory: Add GstAllocationParams and alignment support This implements support for GstAllocationParams and memory alignments. The parameters where simply ignored which could lead to crash on certain platform when used with libav and no luck. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- ext/gl/gstgloverlay.c | 6 +- gst-libs/gst/gl/gstglbufferpool.c | 3 +- gst-libs/gst/gl/gstglcolorconvert.c | 5 +- gst-libs/gst/gl/gstglmemory.c | 137 ++++++++++++++++++++++-------------- gst-libs/gst/gl/gstglmemory.h | 10 +-- gst-libs/gst/gl/gstglupload.c | 4 +- 6 files changed, 102 insertions(+), 63 deletions(-)
Created attachment 298181 [details] [review] [PATCH] glmemory: Use fallback for partial copy When the memory is partial copy, the texture size and videoinfo no longer make sense. As we cannot guess what the application wants, we safely copy into a sysmem memory. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 20 +++++++++++++++++--- gst-libs/gst/gl/gstglmemory.h | 1 + 2 files changed, 18 insertions(+), 3 deletions(-)
Created attachment 298182 [details] [review] [PATCH] glmemory: Provide correct size on download Provide the right size to GL when downloading. This fixes downloading from GLMemory that where created for libav. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Created attachment 298183 [details] [review] [PATCH] glmemory: Provide correct size on upload Provide the right size to GL when uploading. Using maxsize is wrong since we offset the data point with the memory offset and video alignement offset. https://bugzilla.gnome.org/show_bug.cgi?id=744246 --- gst-libs/gst/gl/gstglmemory.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Sorry, I'll cleanup, I didn't understand how send-bugzilla worked it seems.
Review of attachment 298180 [details] [review]: ::: gst-libs/gst/gl/gstglmemory.c @@ +707,3 @@ + gsize align = gst_memory_alignment, offset = 16; + + //align = 4096 - 1; Fixed in my local copy, sorry.
Works now, it also fixes a lot of the issues I was having on nouveau here. The dirver is probably less robust to us giving wrong larger size. commit 8c0fcef81868f0a89adfc766a2a0a89cb1923245 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Feb 28 13:01:16 2015 -0500 glmemory: Provide correct size on upload Provide the right size to GL when uploading. Using maxsize is wrong since we offset the data point with the memory offset and video alignement offset. https://bugzilla.gnome.org/show_bug.cgi?id=744246 commit e9bec9ec0e79cd007b7a070adec200fb207ebc65 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Feb 28 12:48:03 2015 -0500 glmemory: Provide correct size on download Provide the right size to GL when downloading. This fixes downloading from GLMemory that where created for libav. https://bugzilla.gnome.org/show_bug.cgi?id=744246 commit 4db0ef5789880fe588a6372dc457efe37345f464 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Feb 28 11:55:26 2015 -0500 glmemory: Use fallback for partial copy When the memory is partial copy, the texture size and videoinfo no longer make sense. As we cannot guess what the application wants, we safely copy into a sysmem memory. https://bugzilla.gnome.org/show_bug.cgi?id=744246 commit b8f168cd65fbdc85199bfb4eca2afd1445215880 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Wed Feb 25 18:07:03 2015 -0500 glmemory: Add GstAllocationParams and alignment support This implements support for GstAllocationParams and memory alignments. The parameters where simply ignored which could lead to crash on certain platform when used with libav and no luck. https://bugzilla.gnome.org/show_bug.cgi?id=74424
Review of attachment 298180 [details] [review]: With cleanup
I've tested with tutorial-5 and various video size and it works now.
I can confirm
Nicolas, in _gl_mem_copy: if (dest == NULL) { GST_WARNING ("Could not copy GL Memory"); gst_memory_unref ((GstMemory *) dest); goto done; ^^^^ } This doesn't look right :)
commit 17084a4608e21349a502de006210f2df7e9b97a3 Author: Matthew Waters <matthew@centricular.com> Date: Tue Apr 28 20:46:52 2015 +1000 glmemory: remove uneeded unref The call to _gl_mem_alloc_data will unref and NULLify 'dest' for us. We just need to return. https://bugzilla.gnome.org/show_bug.cgi?id=744246