GNOME Bugzilla – Bug 728355
journal debug breaks everything on GLES
Last modified: 2015-09-23 12:58:46 UTC
In _cogl_journal_flush_vbo_offsets_and_entries(): if (G_UNLIKELY (COGL_DEBUG_ENABLED (COGL_DEBUG_JOURNAL))) { uint8_t *verts; /* Mapping a buffer for read is probably a really bad thing to do but this will only happen during debugging so it probably doesn't matter */ verts = ((uint8_t *)_cogl_buffer_map (COGL_BUFFER (state->attribute_buffer), COGL_BUFFER_ACCESS_READ, 0, NULL) + state->array_offset); The comment here is somewhat of an understatement, since you can never map a buffer for read access in GLES, regardless of which extensions you have. So enabling COGL_DEBUG=journal turns a previously working app into one which breaks hard. Making the call conditional would merely lose information; it would be nice if this were available/cached some other way, so we could still dump everything even though glMapBuffer won't let you do read mappings.
Still present in 1.22.0.
I'd gladly review a patch.
I don't know enough about the journal to suggest a sensible patch, so the best I could offer would be to add ... && (ctx->driver == COGL_DRIVER_GL || ctx->driver == COGL_DRIVER_GL3) into the conditional. Not sure if that would be seen as OK or not.
(In reply to Daniel Stone from comment #3) > I don't know enough about the journal to suggest a sensible patch, so the > best I could offer would be to add ... && (ctx->driver == COGL_DRIVER_GL || > ctx->driver == COGL_DRIVER_GL3) into the conditional. Not sure if that would > be seen as OK or not. Considering that it'd at least be better than just making a mess of the carpet as Cogl is doing now, I'd say it's okay to allow debugging of the journal on desktop GL. If somebody ever adds a client-side debug buffer for attributes then we could still do a driver detection and only use it on GLES anyway. I'd also add a warning if somebody uses COGL_DEBUG=journal on GLES, to manage expectations.
Created attachment 311939 [details] [review] Journal: Skip journal dumping when unsupported GLES2 does not support passing READ to glMapBuffer; attempting to call cogl_buffer_map for read on ES2 will bring it down with an assert. Make sure COGL_DEBUG=journal doesn't do this when it's not possible. Signed-off-by: Daniel Stone <daniels@collabora.com>
Created attachment 311940 [details] [review] GLES: Support glMapBufferRange from ES3 ES3 provides glMapBufferRange as core, with the added bonus that it also supports read mappings. Use this where possible. Signed-off-by: Daniel Stone <daniels@collabora.com>
Review of attachment 311940 [details] [review]: ACK.
Review of attachment 311939 [details] [review]: ACK, with a minimal coding style issue; you can fix it while pushing it. ::: cogl/cogl-journal.c @@ +652,3 @@ + if (G_UNLIKELY (COGL_DEBUG_ENABLED (COGL_DEBUG_JOURNAL)) && + cogl_has_feature(ctx, COGL_FEATURE_ID_MAP_BUFFER_FOR_READ)) Coding style: missing space between function name and open parenthesis.
Thanks, but I don't have commit access to GNOME. :)
(In reply to Daniel Stone from comment #9) > Thanks, but I don't have commit access to GNOME. :) We need to get you one of those. :-P
Attachment 311939 [details] pushed as d6f415d - Journal: Skip journal dumping when unsupported Attachment 311940 [details] pushed as 5253264 - GLES: Support glMapBufferRange from ES3