From 40a267cf2568de9b1bb1280822194bd368ce9133 Mon Sep 17 00:00:00 2001 From: lawnjelly Date: Mon, 12 Apr 2021 16:55:59 +0100 Subject: [PATCH] Batching - store parent items in default batches In rare cases default batches could occur which were containing commands that were not owned by the first item referenced by the joined item. This had assumed to be the case, and would read the wrong command, or crash. Instead for safety in this PR we now store a pointer to the parent item in default batches, and use this to determine the correct command list instead of assuming. --- drivers/gles2/rasterizer_canvas_gles2.cpp | 7 +++-- drivers/gles2/rasterizer_canvas_gles2.h | 2 +- drivers/gles3/rasterizer_canvas_gles3.cpp | 7 +++-- drivers/gles3/rasterizer_canvas_gles3.h | 2 +- .../gles_common/rasterizer_canvas_batcher.h | 27 +++++++++++++------ 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/drivers/gles2/rasterizer_canvas_gles2.cpp b/drivers/gles2/rasterizer_canvas_gles2.cpp index 652d466302a..1e4e436ea5b 100644 --- a/drivers/gles2/rasterizer_canvas_gles2.cpp +++ b/drivers/gles2/rasterizer_canvas_gles2.cpp @@ -295,7 +295,7 @@ void RasterizerCanvasGLES2::_batch_render_generic(const Batch &p_batch, Rasteriz glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0); } -void RasterizerCanvasGLES2::render_batches(Item::Command *const *p_commands, Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES2::Material *p_material) { +void RasterizerCanvasGLES2::render_batches(Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES2::Material *p_material) { int num_batches = bdata.batches.size(); @@ -318,9 +318,12 @@ void RasterizerCanvasGLES2::render_batches(Item::Command *const *p_commands, Ite default: { int end_command = batch.first_command + batch.num_commands; + RAST_DEV_DEBUG_ASSERT(batch.item); + RasterizerCanvas::Item::Command *const *commands = batch.item->commands.ptr(); + for (int i = batch.first_command; i < end_command; i++) { - Item::Command *command = p_commands[i]; + Item::Command *command = commands[i]; switch (command->type) { diff --git a/drivers/gles2/rasterizer_canvas_gles2.h b/drivers/gles2/rasterizer_canvas_gles2.h index 6970503590e..2038a24033c 100644 --- a/drivers/gles2/rasterizer_canvas_gles2.h +++ b/drivers/gles2/rasterizer_canvas_gles2.h @@ -55,7 +55,7 @@ private: void canvas_render_items_implementation(Item *p_item_list, int p_z, const Color &p_modulate, Light *p_light, const Transform2D &p_base_transform); void render_joined_item(const BItemJoined &p_bij, RenderItemState &r_ris); bool try_join_item(Item *p_ci, RenderItemState &r_ris, bool &r_batch_break); - void render_batches(Item::Command *const *p_commands, Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES2::Material *p_material); + void render_batches(Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES2::Material *p_material); // low level batch funcs void _batch_upload_buffers(); diff --git a/drivers/gles3/rasterizer_canvas_gles3.cpp b/drivers/gles3/rasterizer_canvas_gles3.cpp index 2e7aab2ad8b..b58d45ecd3e 100644 --- a/drivers/gles3/rasterizer_canvas_gles3.cpp +++ b/drivers/gles3/rasterizer_canvas_gles3.cpp @@ -502,7 +502,7 @@ void RasterizerCanvasGLES3::_legacy_canvas_render_item(Item *p_ci, RenderItemSta } } -void RasterizerCanvasGLES3::render_batches(Item::Command *const *p_commands, Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES3::Material *p_material) { +void RasterizerCanvasGLES3::render_batches(Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES3::Material *p_material) { // bdata.reset_flush(); // return; @@ -527,9 +527,12 @@ void RasterizerCanvasGLES3::render_batches(Item::Command *const *p_commands, Ite default: { int end_command = batch.first_command + batch.num_commands; + RAST_DEV_DEBUG_ASSERT(batch.item); + RasterizerCanvas::Item::Command *const *commands = batch.item->commands.ptr(); + for (int i = batch.first_command; i < end_command; i++) { - Item::Command *c = p_commands[i]; + Item::Command *c = commands[i]; switch (c->type) { diff --git a/drivers/gles3/rasterizer_canvas_gles3.h b/drivers/gles3/rasterizer_canvas_gles3.h index aff7cb228c4..3c019f4261c 100644 --- a/drivers/gles3/rasterizer_canvas_gles3.h +++ b/drivers/gles3/rasterizer_canvas_gles3.h @@ -58,7 +58,7 @@ private: void canvas_render_items_implementation(Item *p_item_list, int p_z, const Color &p_modulate, Light *p_light, const Transform2D &p_base_transform); void render_joined_item(const BItemJoined &p_bij, RenderItemState &r_ris); bool try_join_item(Item *p_ci, RenderItemState &r_ris, bool &r_batch_break); - void render_batches(Item::Command *const *p_commands, Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES3::Material *p_material); + void render_batches(Item *p_current_clip, bool &r_reclip, RasterizerStorageGLES3::Material *p_material); // low level batch funcs void _batch_upload_buffers(); diff --git a/drivers/gles_common/rasterizer_canvas_batcher.h b/drivers/gles_common/rasterizer_canvas_batcher.h index 1404d8a56f7..d3209880a87 100644 --- a/drivers/gles_common/rasterizer_canvas_batcher.h +++ b/drivers/gles_common/rasterizer_canvas_batcher.h @@ -184,7 +184,15 @@ public: // first vertex of this batch in the vertex lists uint32_t first_vert; - BatchColor color; + // we can keep the batch structure small because we either need to store + // the color if a handled batch, or the parent item if a default batch, so + // we can reference the correct originating command + union { + BatchColor color; + + // for default batches we will store the parent item + const RasterizerCanvas::Item *item; + }; }; struct BatchTex { @@ -655,8 +663,11 @@ public: RAST_DEBUG_ASSERT(batch); } - if (p_blank) + if (p_blank) { memset(batch, 0, sizeof(Batch)); + } else { + batch->item = nullptr; + } return batch; } @@ -857,6 +868,7 @@ PREAMBLE(void)::_prefill_default_batch(FillState &r_fill_state, int p_command_nu r_fill_state.curr_batch->type = RasterizerStorageCommon::BT_DEFAULT; r_fill_state.curr_batch->first_command = extra_command; r_fill_state.curr_batch->num_commands = 1; + r_fill_state.curr_batch->item = &p_item; // revert to the original transform mode // e.g. go back to NONE if we were in hardware transform mode @@ -882,6 +894,7 @@ PREAMBLE(void)::_prefill_default_batch(FillState &r_fill_state, int p_command_nu r_fill_state.curr_batch->type = RasterizerStorageCommon::BT_DEFAULT; r_fill_state.curr_batch->first_command = p_command_num; r_fill_state.curr_batch->num_commands = 1; + r_fill_state.curr_batch->item = &p_item; } } @@ -2445,15 +2458,14 @@ PREAMBLE(void)::flush_render_batches(RasterizerCanvas::Item *p_first_item, Raste // send buffers to opengl get_this()->_batch_upload_buffers(); - RasterizerCanvas::Item::Command *const *commands = p_first_item->commands.ptr(); - #if defined(TOOLS_ENABLED) && defined(DEBUG_ENABLED) if (bdata.diagnose_frame) { + RasterizerCanvas::Item::Command *const *commands = p_first_item->commands.ptr(); diagnose_batches(commands); } #endif - get_this()->render_batches(commands, p_current_clip, r_reclip, p_material); + get_this()->render_batches(p_current_clip, r_reclip, p_material); // if we overrode the fvf for lines, set it back to the joined item fvf bdata.fvf = backup_fvf; @@ -2552,15 +2564,14 @@ PREAMBLE(void)::_legacy_canvas_item_render_commands(RasterizerCanvas::Item *p_it int command_count = p_item->commands.size(); - RasterizerCanvas::Item::Command *const *commands = p_item->commands.ptr(); - // legacy .. just create one massive batch and render everything as before bdata.batches.reset(); Batch *batch = _batch_request_new(); batch->type = RasterizerStorageCommon::BT_DEFAULT; batch->num_commands = command_count; + batch->item = p_item; - get_this()->render_batches(commands, p_current_clip, r_reclip, p_material); + get_this()->render_batches(p_current_clip, r_reclip, p_material); bdata.reset_flush(); }