Upgrade RenderEngineGl's image readback

[SeanCurtis-TRI]

Using glReadPixels is much faster than glTextTextureImage (the latter incurs a number of pieces of overhead that are unnecessary when all we want to do is read back from an uncompressed frame buffer).

---

This change is 

[SeanCurtis-TRI]

You can see the impact of this change by running the following command on master and then on this branch.

bazel run geometry/benchmarking:render_benchmark -- --benchmark_filter=.*/Gl*/*

@calderpg-tri this doesn't solve your problem, per se. But it's a step in the right direction. It certainly justifies enriching the feature set. Are you willing to do a feature review? (Essentially, run the benchmark before and after and confirm that numbers are smaller with the PR than without).

@SeanCurtis-TRI made 1 comment.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes.

[jwnimmer-tri]

@jwnimmer-tri made 2 comments.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on SeanCurtis-TRI).

---

geometry/render_gl/internal_render_engine_gl.h line 279 at r1 (raw file):
BTW

This is slower than it has to be because it does per-pixel processing on the CPU. TODO(SeanCurtis-TRI): Figure out how to do all of this directly on the GPU.

Does this comment need revisiting now? I guess it's still true, since we still have the for-loop inside here.

---

geometry/render_gl/internal_render_engine_gl.cc line 1186 at r1 (raw file):

  // wouldn't be visible.
  SetWindowVisibility(camera.core(), camera.show_window(), render_target);
  // TODO(SeanCurtis-TRI): Apparently, we *should* be able to create a frame

BTW Does this comment need revisiting now? I guess it still seems accurate, but shouldn't it at least be placed next to the for-loops inside of the helper function, instead of at the call site of the helper here? Perhaps the "frame buffer texture" suggested solution is moot now with the change to the GL readback call?

[SeanCurtis-TRI]

@SeanCurtis-TRI made 2 comments and resolved 1 discussion.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on jwnimmer-tri).

---

geometry/render_gl/internal_render_engine_gl.h line 279 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW

This is slower than it has to be because it does per-pixel processing on the CPU. TODO(SeanCurtis-TRI): Figure out how to do all of this directly on the GPU.

Does this comment need revisiting now? I guess it's still true, since we still have the for-loop inside here.

Correct.

---

geometry/render_gl/internal_render_engine_gl.cc line 1186 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Does this comment need revisiting now? I guess it still seems accurate, but shouldn't it at least be placed next to the for-loops inside of the helper function, instead of at the call site of the helper here? Perhaps the "frame buffer texture" suggested solution is moot now with the change to the GL readback call?

I have one fix that moves this inside the GetLabelImage() method.

I also am working up a resolution to the TODO. GIven that it is thematically consistent with the idea of "get the images back faster", it could fit in this. However, it is more complex with more flailing parts. As such, it might make sense to little here and allow those gymnastics to play out in their own space.

[sherm1]

Is this ready to have a feature reviewer assigned?

[calderpg-tri]

I can do a feature review pass

@calderpg-tri made 1 comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on jwnimmer-tri).