[sensors] Encapsulate VTK image readers and writers

[jwnimmer-tri]

Towards #19945 and #16502. In particular, the new unit test here will serve as a backstop against regressions in the reader/writer code.

---

This change is 

[jwnimmer-tri]

\CC @SeanCurtis-TRI @sammy-tri @zachfang FYI. I still want to double-check the testing for missing cases, but overall this is the direction I'm aiming.

The call site code is somewhat improved by this, but the real win here is the additional tests. The new tests are part of what will help convince me that our pending upgrade to VTK will be sound.

I also have a WIP branch that further encapsulates image reading and writing, when the source / destination is specifically a sensors::Image instead of just a raw pixel buffer, specifically encapsulating the alpha (un)padding, bgr swizzling, etc. That one isn't ready for prime time yet.

[jwnimmer-tri]

Reviewed 6 of 10 files at r1, 5 of 5 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

[jwnimmer-tri]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

[jwnimmer-tri]

Okay, I think this is ready for feature review.

+@SeanCurtis-TRI you have right of first refusal, I think.

[SeanCurtis-TRI]

+@xuchenhan-tri for platform review, please.

Reviewed 3 of 10 files at r1, 3 of 5 files at r2, 4 of 5 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform)

---

systems/sensors/lcm_image_array_to_images.cc line 67 at r6 (raw file):

      image->width() * image->height() * image->kPixelSize) {
    drake::log()->error(
        fmt::format("Malformed output decoding incoming LCM {} image", format));

BTW Why the injection of fmt::format? log()->error() is already equipped to handle the format string.

---

systems/sensors/vtk_image_reader_writer.h line 19 at r6 (raw file):

namespace internal {

// (Internal use only) This file provides helper functions that encapsulate

BTW This kind of comment seems redundant when you have namespace internal { just two lines higher.

---

systems/sensors/vtk_image_reader_writer.cc line 75 at r6 (raw file):

}

/* Trampolies VTK progress events into a Drake-specific callback. */

nit typo

Suggestion:

Trampolines

---

systems/sensors/vtk_image_reader_writer.cc line 136 at r6 (raw file):

  // Add an observer to the writer that copies the encoded image into the
  // `output` buffer.
  auto observer = vtkSmartPointer<VtkProgressObserver>::New();

BTW An equivalent way to do this:

vtkNew<VtkProgressObserver> observer;

[jwnimmer-tri]

Reviewed 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform)

---

systems/sensors/lcm_image_array_to_images.cc line 67 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Why the injection of fmt::format? log()->error() is already equipped to handle the format string.

Oops. Shrapnel from the future version of this that uses DiagnosticPolicy.

---

systems/sensors/vtk_image_reader_writer.cc line 136 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW An equivalent way to do this:

vtkNew<VtkProgressObserver> observer;

I need a shared ptr though, to attach it to the writer and return it. I think your line only creates a unique ptr?

[xuchenhan-tri]

nice!

Reviewed 2 of 10 files at r1, 3 of 5 files at r2, 2 of 5 files at r3, 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: 1 unresolved discussion

[SeanCurtis-TRI]

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

---

systems/sensors/vtk_image_reader_writer.cc line 136 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I need a shared ptr though, to attach it to the writer and return it. I think your line only creates a unique ptr?

In today's episode of "Strange but True", we discuss vtk's "smart pointers"...

While it's natural to want to think of vtkNew and vtkSmartPointer as pre-C++11 versions of unique_ptr and shared_ptr (the names invite this), they are not.

vtkNew is not a unique pointer. Not at all. And while vtkSmartPointer is kind of like a shared pointer, it doesn't maintain the reference counter, that lives inside the object itself. vtkNew<Foo> merely takes care of calling Foo:New for you. You can implicitly create a vtkSmartPointer from a vtkNew and both remain valid pointers to the underlying object (both accounting for a tick in the reference counter). In fact, it was only in 2019 (after I suggested it), that they even created move semantics to create a vtkSmartPointer from vtkNew&&.

So, as counter-intuitive as it may seem, vtkNew does exactly what you need it to do. Particularly as all you're doing is adding it as an observer to writer. That said, vtkSmartPointer isn't wrong, just overly verbose.

[jwnimmer-tri]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)