GNOME Bugzilla – Bug 694164
Don't use GL_MAP_INVALIDATE_RANGE_BIT on READ_BIT buffers
Last modified: 2013-02-19 15:07:08 UTC
Currently i get this from clutter: (gnome-boxes:2849): Cogl-WARNING **: ./driver/gl/cogl-util-gl.c:89: GL error (1282): Invalid operation (gnome-boxes:2849): Cogl-CRITICAL **: _cogl_buffer_gl_map_range: assertion `data != ((void *)0)' failed (gnome-boxes:2849): GLib-CRITICAL **: g_error_free: assertion `error != NULL' failed (gnome-boxes:2849): Cogl-CRITICAL **: _cogl_buffer_bind_no_create: assertion `ctx->current_buffer[buffer->last_target] != buffer' failed This is because clutter-canvas.c does: data = cogl_buffer_map (buffer, COGL_BUFFER_ACCESS_READ_WRITE, COGL_BUFFER_MAP_HINT_DISCARD); And, if cogl hits the glMapBufferRange path it will get an error from Gl because GL_MAP_INVALIDATE_BUFFER_BIT and GL_MAP_READ_BIT are not compatible. However, it seems to me that the semantics of COGL_BUFFER_MAP_HINT_DISCARD is just that of a hint, so that if we're unable to support it we should just not set GL_MAP_INVALIDATE_BUFFER_BIT, rather than fail hard. As things stand now the semantics of READ_WRITE and HINT_DISCARD changed to make a previously successful codepath now fail. Attaching a possible fix.
Hmm, looking more at this it seems like we can't just ignore the DISCARD hint and just redo the mapbuffer, as that will fail if the buffer is mapped already. Do we have to manually unmap the range in this case?
I'm not sure when you mean about having to remap the buffer but after discussing on IRC it sounds like that was just a misunderstanding. I'm attaching a patch which makes it stop passing the discard hint when read access is requested. I think we should also stop Clutter from using CoglBitmaps for Cairo. As far as I know we haven't done any actual testing to prove that using a PBO is faster and anecdotal advice suggests that is potentially a lot slower. I think that on a lot of platforms reading from a buffer that is used by the GPU can be very slow because it will not use the cache in the same way. Therefore it's probably faster to just draw into normal memory and copy the data to the texture. PBOs will usually end up doing a copy within the driver anyway unless you happen to guess the right format for the texture.
Created attachment 236762 [details] [review] buffer: Don't set the invalidate hint when requesting read access glMapBufferRange is documented to fail with GL_INVALID_OPERATION if GL_MAP_INVALIDATE_BUFFER_BIT is set as well as GL_MAP_READ_BIT. I guess this makes sense when only read access is requested because there would be no point in reading back uninitialised data. However, Clutter requests read/write access with the discard hint when rendering to a CoglBitmap with Cairo. The data is new so the discard hint makes sense but it also needs read access so that it can read back the data it just wrote for blending. This patch works around the GL restriction by skipping the discard hints if read access is requested. If the buffer discard hint is set along with read access it will recreate the buffer store as an alternative way to discard the buffer as it does in the case where the GL_ARB_map_buffer_range extension is not supported.
This fixes the warnings for me and seems right.
Comment on attachment 236762 [details] [review] buffer: Don't set the invalidate hint when requesting read access This looks good to land to me, though maybe it would be good to also add a small comment in the code to remind us that glMapBufferRange gets upset if you pass INVALIDATE_BUFFER_BIT in conjunction with READ_BIT.
Ok, thanks. I've added a comment to the code and pushed it to master and the Cogl 1.14 branch. http://git.gnome.org/browse/cogl/tree/cogl/driver/gl/cogl-buffer-gl.c?id=986675d#n251