GNOME Bugzilla – Bug 729896
glupload: Ignores stride when uploading raw data
Last modified: 2014-05-13 15:47:30 UTC
The stride is not handled properly in gst_gl_memory_wrapped_texture(), which end up with an out texture size smaller then required, and a silent failure follows. I've already added a fix to update the video_info base on the info optain by doing gst_video_frame_map (take video info into account), but the rest of the code does not seem to follow very well and need more attention.
commit afea9c6fe17f4508670cc2221147f2d2a5cca6f5 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri May 9 18:59:46 2014 -0400 gl: Fix some of the error handling It does not fail silently anymore, it actually display something but stride is clearly wrong.
Every time the format can change, one should call gst_gl_upload_set_format which should deinit the GL resources and recreate (at the correct size) on the next _draw call (only if required).
gst_gl_memory_wrapped_texture is currently only ever used for the output texture of the converted video frame which doesn't need any special stride information associated with it. That doesn't mean that we don't need to add a stride argument to the function definition like all the other glmemory functions. It would be useful for the download case where we need to pack the data according to the stride.
Ok, this mean the texture is created too soon. Because you only get the video meta on first frame. Also, you have no guaranty that it won't change over time. So what we probably need is to call gst_gl_upload_set_format() everytime and add the intelligence to not do anything if nothing important changed (I guess size is what we need to watch for right ?). This matches with the problems I'm getting. We can revert the patch to update the video info, this seemed more right, but is wrong as you mentioned.
Simply call gst_gl_upload_set_format whenever the size/format may change. It should perform all of that :).
Created attachment 276306 [details] [review] gl/upload: update the videoinfo on mapping the video frame
Review of attachment 276306 [details] [review]: Indeed, just noticed now the call to gst_video_info_is_equal (&upload->in_info, in_info) (which I didn't know existed ;-P). Thanks again for your time.
Review of attachment 276306 [details] [review]: Indeed, just noticed now the call to gst_video_info_is_equal (&upload->in_info, in_info) (which I didn't know existed ;-P). Thanks again for your time
Review of attachment 276306 [details] [review]: Sorry for the noise, it's wall all unresponsive and then got that. Though I found a problem in the patch. ::: gst-libs/gst/gl/gstglupload.c @@ +335,2 @@ /* update the video info from the one updated by frame_map using video meta */ + gst_gl_upload_set_format (upload, &v_info); Actually, that's going to set the old config again.
just doing gst_gl_upload_set_format (upload, &frame->info) should do.
The missing detail is that in gst_video_frame_map(), the info reference is not modified. Instead a copy is made and update, and found in frame->info.
Created attachment 276309 [details] [review] gl/upload: update the videoinfo on mapping the video frame Ah! I did not know that.
Review of attachment 276309 [details] [review]: Excellent !
commit f14e5ea02702f1cbfce013f7161a45615d609dcf Author: Matthew Waters <ystreet00@gmail.com> Date: Sun May 11 12:48:52 2014 +1000 gl/upload: update the video info on mapping a video frame The buffer should contain the most specific data on how the data is formatted. We should use this information. https://bugzilla.gnome.org/show_bug.cgi?id=729896
Reopening, as the stride handling is still broken. Looks like the information is set correctly, but something fails using it a bit deeper. The bug isn't fixed for gl_upload_meta where the we do endup doing a real upload (even though I doubt we'll have special strides). For the handling of video meta, when we do upload, I'm wondering if we need of not to correctly copy the stride and offset over.
Still not fixed, but found this important issue: commit e0cfd6e26b53b9530b0a3494b883c490714b8da7 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon May 12 13:18:50 2014 -0400 video-info: Also check the stride and offset are equal gst_video_info_is_equal() was not checking if stride and offset had changed. https://bugzilla.gnome.org/show_bug.cgi?id=729896 Which cause the old config to remain. After this change we would failed, cause texture was gone. commit 2d0a791348c0a37cc90c99741cd64527d90d7826 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon May 12 13:50:47 2014 -0400 glupload: Ensure we still have a texture after upload_set_format() gst_gl_upload_set_format() resets the upload, hence the texture. So we need to ensure we have a texture after this call when uploading. And I added patches to address the issue I mentioned. commit 864e1511c7cf38c1c24e1c41816bb75f71f52916 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon May 12 12:59:59 2014 -0400 glupload: Correctly update the video info from video meta Using gst_video_info_set_format() isn't complete when updating a video info from video meta. commit 4f84a6124a719c2a9c927b5e91b36f5d5b20d617 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon May 12 12:57:18 2014 -0400 glupload: Correctly update video info in upload fallback When the upload accelerated method fails, we fallback to an upload, but the video info was not correctly updated. But sadly, I still get stride issues.
Should have done that before, but reading the doc: Specifies the alignment requirements for the start of each pixel row in memory. The allowable values are 1 (byte-alignment), 2 (rows aligned to even-numbered bytes), 4 (word-alignment), and 8 (rows start on double-word boundaries). And I need 16 pixels alignment (BGRA), which would mean an invalid value or 64 bytes. I guess for this case we need a fallback, e.g. use the coordinate scaling in place.
That's what gst_gl_color_convert_set_texture_scaling() is for :). But it seems to not be used for GLES2 :/
Hey, that's what I found, but the method we use is wrong. The indexer will not skip lines correctly. I've did an experiment when I create a larger texture, and it works, but scaling seems ignored.
Created attachment 276417 [details] [review] glmemory: Fix handling of stride with alignement larger then 8
Looks like this patch is nearly correct. What's wrong now is the scaling implementation in the converter. I'd op for removing the set_scaling() and update the scaling uniforms in _preform() from the memory values (something we don't do at the moment).
Review of attachment 276417 [details] [review]: Looks good
Created attachment 276420 [details] [review] gl/colorconvert: use the texture scaling from the glmemory
Created attachment 276430 [details] [review] gl/colorconvert: use the texture scaling from the glmemory Should also remove the function definition in the header.
Comment on attachment 276417 [details] [review] glmemory: Fix handling of stride with alignement larger then 8 commit d4bcef3204709159713ff1630978ecac0bd91dc9 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Mon May 12 19:29:45 2014 -0400 glmemory: Fix handling of stride with alignement larger then 8 Setting a scaled factor for X coordinate is not enough as the indexer will still think stride is shorter and will not fully skip it. Instead, update width, so the lines are as expected. Combined with the scale, it will hide the cropped portion. https://bugzilla.gnome.org/show_bug.cgi?id=729896
Comment on attachment 276430 [details] [review] gl/colorconvert: use the texture scaling from the glmemory commit 1e90a68aec966d6a56e839db1cf759716b5583d8 Author: Matthew Waters <ystreet00@gmail.com> Date: Tue May 13 10:53:19 2014 +1000 gl/colorconvert: use the texture scaling from the gl memory The colorconvert values were not being used at all. https://bugzilla.gnome.org/show_bug.cgi?id=729896
Works now, and it's all clean, no green border, no visible rounding issue in general. Thanks for the good help.