fix(render): bind identity into the epoch, titles into reserved strips, one layout per frame
Four review findings on the Task 7 renderer: - InstanceStore::epoch() is derived from gl::Buffer::generation() -- advanced by every allocation, deletion and move -- plus the slot the instances were written to, so the orphan fallback of grow_persistent() can no longer leave the vertex array's attribute pointers bound to a deleted buffer: the epoch reports the new identity and upload() re-points them before the draw. - PlotLayout reserves y_title_area/x_title_area outside axes (sized from the text metrics) and draw_titles() draws in them, so a title can no longer land on the data, whatever the envelope does. - draw() computes plot.layout() once and hands it to the helpers, instead of six times per frame, each of which rebuilt the ticks and the grid. - A refused fence is no longer dropped: mark_drawn() falls back to the orphan path and reports the failure rather than rotating through an unsynchronised slot. New smoke cases force both refusals through the GL entry points (glMapBufferRange, glFenceSync) and assert the pixels plus the live transfer mode; the title case asserts the title colour is absent from the bands inside axes and present in both strips. All three fail against the pre-fix sources.
This commit is contained in:
Martino Ferrari committed
1 parent
3555ab63d7
commit
8f45108559
11 files changed
+492
-70
No files matched your search
@@ -101,6 +101,56 @@ void dump_if_requested(const plot::Renderer& r, const char* default_name) {
|
||||
PLOT_CHECK(ok);
|
||||
}
|
||||
|
||||
/// True when `px` is (within a couple of levels) the straight-alpha colour `c`,
|
||||
/// which is what a fully covered text pixel is. The colour comes from the style,
|
||||
/// so no predicate here has to guess what the renderer draws with.
|
||||
bool pixel_is(const plot::u8 px[4], plot::Color c) {
|
||||
const auto near = [](plot::u8 got, plot::f32 want) {
|
||||
const int diff = static_cast<int>(got) - static_cast<int>(want * 255.0f + 0.5f);
|
||||
return diff >= -3 && diff <= 3;
|
||||
};
|
||||
return near(px[0], c.r) && near(px[1], c.g) && near(px[2], c.b);
|
||||
}
|
||||
|
||||
/// Holds one GL entry point at a stub for as long as it lives. The entry points
|
||||
/// are the library's own function pointers (`plot/gl/api.hpp`), so a driver
|
||||
/// refusal -- a map or a fence that fails while the caps still advertise support
|
||||
/// -- is testable without a broken driver. The pointer is saved on construction
|
||||
/// and put back in the destructor, which also runs while a failed `PLOT_CHECK`
|
||||
/// unwinds, so a case can never leak its stub into the next one.
|
||||
class RefusePersistentMap {
|
||||
public:
|
||||
RefusePersistentMap() noexcept : saved_(plot::gl::glMapBufferRange) {
|
||||
plot::gl::glMapBufferRange = &refuse;
|
||||
}
|
||||
~RefusePersistentMap() { plot::gl::glMapBufferRange = saved_; }
|
||||
RefusePersistentMap(const RefusePersistentMap&) = delete;
|
||||
RefusePersistentMap& operator=(const RefusePersistentMap&) = delete;
|
||||
|
||||
private:
|
||||
static void* refuse(plot::gl::GLenum, plot::gl::GLintptr, plot::gl::GLsizeiptr,
|
||||
plot::gl::GLbitfield) noexcept {
|
||||
return nullptr;
|
||||
}
|
||||
decltype(plot::gl::glMapBufferRange) saved_;
|
||||
};
|
||||
|
||||
/// Same, for `glFenceSync`: the sync object the ring's slot guard needs is
|
||||
/// refused while the caps and the entry point still say the mapped path is on.
|
||||
class RefuseFenceSync {
|
||||
public:
|
||||
RefuseFenceSync() noexcept : saved_(plot::gl::glFenceSync) { plot::gl::glFenceSync = &refuse; }
|
||||
~RefuseFenceSync() { plot::gl::glFenceSync = saved_; }
|
||||
RefuseFenceSync(const RefuseFenceSync&) = delete;
|
||||
RefuseFenceSync& operator=(const RefuseFenceSync&) = delete;
|
||||
|
||||
private:
|
||||
static plot::gl::GLsync refuse(plot::gl::GLenum, plot::gl::GLbitfield) noexcept {
|
||||
return nullptr;
|
||||
}
|
||||
decltype(plot::gl::glFenceSync) saved_;
|
||||
};
|
||||
|
||||
} // namespace
|
||||
|
||||
PLOT_TEST(renders_visible_trace_across_viewport) {
|
||||
@@ -343,3 +393,169 @@ PLOT_TEST(draws_cursors_measure_band_and_annotations) {
|
||||
|
||||
ctx.destroy();
|
||||
}
|
||||
|
||||
// Finding 2 of the review: `draw_titles()` placed both titles inside
|
||||
// `layout.axes`, because the layout reserved no room outside it for them, so in
|
||||
// the dense frame -- whose envelope spans every column's full height -- a title
|
||||
// sat on the trace and no pixel test could see it. The layout now insets `axes` by
|
||||
// a strip per direction and the titles are drawn inside those strips, which is
|
||||
// what makes a title over the data impossible. This draws known titles over that
|
||||
// same dense envelope and asserts both halves: no title-coloured pixel in a band
|
||||
// just inside either horizontal edge of `axes`, and the title colour inside both
|
||||
// strips. The colour is the style's own `axis_text` -- what the text pipeline is
|
||||
// tinted with -- never a guess, and no other element of the frame can match it
|
||||
// (the trace is blue-dominant, the grid grey-dark, the axis line mid-grey).
|
||||
PLOT_TEST(titles_stay_out_of_the_axes_band) {
|
||||
plottest::HeadlessContext ctx(kW, kH);
|
||||
plot::Status st;
|
||||
PLOT_CHECK(ctx.create(&st));
|
||||
PLOT_CHECK(plot::gl::load(nullptr));
|
||||
plot::Renderer r;
|
||||
PLOT_CHECK(r.init(&st));
|
||||
|
||||
plot::data::ArraySeries s(sine_samples(), 0.0, 1.0);
|
||||
plot::Plot p;
|
||||
p.set_viewport({0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
const plot::u32 ax = p.add_x_axis({"seconds", s.x_bounds(), true, plot::AxisPlace::Bottom, false});
|
||||
const plot::u32 ay = p.add_y_axis({"volts", {-1.0, 1.0}, true, plot::AxisPlace::Left, false});
|
||||
p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay});
|
||||
|
||||
r.begin_frame(kW, kH, {0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
r.draw(p);
|
||||
r.end_frame();
|
||||
PLOT_CHECK(!r.last_frame_had_gl_error());
|
||||
dump_if_requested(r, "titles.ppm");
|
||||
|
||||
const plot::PlotLayout& layout = p.layout();
|
||||
const plot::Rect& a = layout.axes;
|
||||
const plot::Color title = r.style().axis_text;
|
||||
|
||||
// Reads a Rect -- whose rows run top-down, like every Rect here -- and reports
|
||||
// whether any pixel in it is the title colour.
|
||||
auto area_has_title = [&](const plot::Rect& area) {
|
||||
for (plot::u32 row = 0; row < static_cast<plot::u32>(area.h); ++row) {
|
||||
for (plot::u32 column = 0; column < static_cast<plot::u32>(area.w); ++column) {
|
||||
plot::u8 px[4] = {};
|
||||
PLOT_CHECK(ctx.read_pixel(static_cast<plot::u32>(area.x) + column,
|
||||
ctx.height - 1u - (static_cast<plot::u32>(area.y) + row), px));
|
||||
if (pixel_is(px, title)) return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
};
|
||||
|
||||
// One text line plus the two pixels the old placement inset itself by: tall
|
||||
// enough for the whole title line that used to be drawn there.
|
||||
const plot::f32 band = r.axis_font_px() + 2.0f;
|
||||
PLOT_CHECK(!area_has_title({a.x, a.y, a.w, band}));
|
||||
PLOT_CHECK(!area_has_title({a.x, a.bottom() - band, a.w, band}));
|
||||
|
||||
// ... while the strips the layout reserved hold both titles.
|
||||
PLOT_CHECK(layout.x_title_area.h > 0.0f);
|
||||
PLOT_CHECK(layout.y_title_area.w > 0.0f);
|
||||
PLOT_CHECK(layout.axes.bottom() <= layout.x_title_area.y); // outside the data
|
||||
PLOT_CHECK(area_has_title(layout.x_title_area));
|
||||
PLOT_CHECK(area_has_title(layout.y_title_area));
|
||||
ctx.destroy();
|
||||
}
|
||||
|
||||
// Finding 1 of the review: when the mapping failed, the store re-created its
|
||||
// buffer as the orphan path's -- and nothing told the vertex array, so it stayed
|
||||
// bound to a buffer whose attribute bindings `glDeleteBuffers` had already
|
||||
// cleared, and every later frame drew nothing at all. The epoch now carries the
|
||||
// buffer's identity, so the array re-points itself before the draw. The refusal is
|
||||
// forced through the entry point (`glMapBufferRange` returns null while the caps
|
||||
// and the pointer still advertise the mapped path, which is exactly the state the
|
||||
// fallback exists for), and the frame after it must still rasterise the trace.
|
||||
PLOT_TEST(mapped_ring_fallback_still_draws_after_refused_map) {
|
||||
plottest::HeadlessContext ctx(kW, kH);
|
||||
plot::Status st;
|
||||
PLOT_CHECK(ctx.create(&st));
|
||||
PLOT_CHECK(plot::gl::load(nullptr));
|
||||
|
||||
plot::Renderer r;
|
||||
{
|
||||
RefusePersistentMap guard; // before init(): the mapped path is still selected
|
||||
PLOT_CHECK(r.init(&st));
|
||||
const bool forced = r.using_persistent_upload();
|
||||
|
||||
plot::data::ArraySeries s(sine_samples(), 0.0, 1.0);
|
||||
plot::Plot p;
|
||||
p.set_viewport({0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
const plot::u32 ax = p.add_x_axis({"t", s.x_bounds(), true, plot::AxisPlace::Bottom, false});
|
||||
const plot::u32 ay = p.add_y_axis({"v", {-1.0, 1.0}, true, plot::AxisPlace::Left, false});
|
||||
p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay});
|
||||
p.set_x_range(ax, {0.0, 640.0}); // one sample per pixel: an unmistakable polyline
|
||||
|
||||
// The first upload is the one that grows the ring, so it is the one whose map
|
||||
// is refused and which ends up on the orphan buffer.
|
||||
r.begin_frame(kW, kH, {0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
r.draw(p);
|
||||
r.end_frame();
|
||||
// Geometry first: this is the assertion the deleted buffer's attribute
|
||||
// bindings broke, leaving the draw with no attributes to read at all.
|
||||
PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15);
|
||||
if (forced) {
|
||||
PLOT_CHECK(!r.using_persistent_upload()); // the fallback really ran
|
||||
}
|
||||
PLOT_CHECK(!r.last_frame_had_gl_error());
|
||||
|
||||
// ... and the frame after it, which is where the deleted buffer's attribute
|
||||
// bindings used to leave the pipeline with nothing bound at all.
|
||||
r.begin_frame(kW, kH, {0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
r.draw(p);
|
||||
r.end_frame();
|
||||
PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15);
|
||||
PLOT_CHECK(!r.last_frame_had_gl_error());
|
||||
}
|
||||
ctx.destroy();
|
||||
}
|
||||
|
||||
// Finding 4 of the review: `mark_drawn()` dropped a failed fence's status, so the
|
||||
// slot stayed unsynchronised and the next upload memcpy'd into storage the GPU may
|
||||
// still have been reading -- exactly the tearing the ring exists to prevent. A
|
||||
// refused fence now drops the ring for the orphan path and reports it, instead of
|
||||
// rotating through a slot nothing guards. `glFenceSync` is stubbed to hand back no
|
||||
// sync object while the caps still advertise the mapped path, so the fence is the
|
||||
// only thing that fails.
|
||||
PLOT_TEST(refused_fence_drops_the_mapped_ring) {
|
||||
plottest::HeadlessContext ctx(kW, kH);
|
||||
plot::Status st;
|
||||
PLOT_CHECK(ctx.create(&st));
|
||||
PLOT_CHECK(plot::gl::load(nullptr));
|
||||
|
||||
plot::Renderer r;
|
||||
{
|
||||
RefuseFenceSync guard;
|
||||
PLOT_CHECK(r.init(&st));
|
||||
const bool forced = r.using_persistent_upload();
|
||||
|
||||
plot::data::ArraySeries s(sine_samples(), 0.0, 1.0);
|
||||
plot::Plot p;
|
||||
p.set_viewport({0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
const plot::u32 ax = p.add_x_axis({"t", s.x_bounds(), true, plot::AxisPlace::Bottom, false});
|
||||
const plot::u32 ay = p.add_y_axis({"v", {-1.0, 1.0}, true, plot::AxisPlace::Left, false});
|
||||
p.add_trace({&s, plot::Color::hex(0x40C0FF), 1.5f, ax, ay});
|
||||
p.set_x_range(ax, {0.0, 640.0});
|
||||
|
||||
r.begin_frame(kW, kH, {0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
r.draw(p);
|
||||
r.end_frame();
|
||||
if (forced) {
|
||||
// The frame whose slot could not be fenced has stopped using the ring: the
|
||||
// next upload must not write storage the draw may still be reading, and the
|
||||
// frame reports the failure rather than swallowing it.
|
||||
PLOT_CHECK(!r.using_persistent_upload());
|
||||
PLOT_CHECK(r.last_frame_had_gl_error());
|
||||
}
|
||||
PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15); // it still drew
|
||||
|
||||
// The next frames draw through the orphan path, which needs no fence.
|
||||
r.begin_frame(kW, kH, {0, 0, static_cast<plot::f32>(kW), static_cast<plot::f32>(kH)});
|
||||
r.draw(p);
|
||||
r.end_frame();
|
||||
PLOT_CHECK(covered_columns(ctx, p.layout().axes, 16) >= 15);
|
||||
PLOT_CHECK(!r.last_frame_had_gl_error());
|
||||
}
|
||||
ctx.destroy();
|
||||
}
|
||||
Reference in new issue
Block a user