Add ImageWriter

[SeanCurtis-TRI]

Simple publishing system whose whole point is to save Image instances to the disk (format depends on image type) from an RgbdCamera.

Provides a general image writer that has a configurable set of inputs where each input can output a different type of image, at an independent rate, to an independent location, etc.

This is the follow up to the discussion that arose in #9772. Admittedly, it's based on what was in my brain. So, if there was some feature that was called out there that is missing or misrepresented in this PR, please call it out.

One open design question: the declaration of an input port currently requires explicit passing of the PixelType from the caller's perspective. This is certainly the most general solution. However, it may not necessarily be as convenient for users. I could certainly add three sugar functions: DeclareColorImageInputPort(), DeclareDepthImageInputPort(), DeclareLabelImageInputPort() which masks the PixelType. This PR is already quite long, so I thought I'd defer that call until I got some feedback.

To all the people who ended up involved in the previous PR. If one of you wants to assign yourself as feature reviewer, I'd be grateful.
/cc @kunimatsu-tri @thduynguyen @EricCousineau-TRI @jwnimmer-tri

---

This change is 

[kunimatsu-tri]

+@kunimatsu-tri for feature review.

Reviewable status: all discussions resolved, platform LGTM missing

[SeanCurtis-TRI]

Thanks @kunimatsu-tri

Reviewable status: all discussions resolved, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

[kunimatsu-tri]

Checkpoint. Will review test later.

Reviewed 8 of 9 files at r1.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

---

systems/sensors/image_writer.h, line 85 at r1 (raw file):

  ImageWriter();

  /** Declares and configures a new image input port. A port is configured by

Very nice documentation. Thanks, @SeanCurtis-TRI!

@thduynguyen Can you also look at this and say something based-on your use cases?

---

systems/sensors/image_writer.h, line 102 at r1 (raw file):

   Given a _positive_ publish period `p`, images will be written at times
   contained in the list of times: `t = [0, p, 1⋅p, 2⋅p, ...]`. The start time

Isn't this t = [0, 1⋅p, 2⋅p, ...] instead of t = [0, p, 1⋅p, 2⋅p, ...]?

---

systems/sensors/image_writer.h, line 105 at r1 (raw file):

   parameter determines what the _first_ output time will be. Given a "start
   time" value `tₛ`, the frames will be written at:
   `t = tₛ + [0, p, 1⋅p, 2⋅p, ...]`.

Ditto for the sequence inside [].

---

systems/sensors/image_writer.h, line 132 at r1 (raw file):

   File names can then be specified as with the following examples (assuming
   the port was declared as a color image port, with a name of "my_port", a
   period of 0.02 (50 Hz), and a start time of 5 s.

0.02 -> 0.02 s.

---

systems/sensors/image_writer.cc, line 103 at r1 (raw file):

  // Test to confirm valid pixel type.
  static_assert(kPixelType == PixelType::kRgba8U ||
                    kPixelType == PixelType::kDepth32F ||

nit: wrong indent.

[SeanCurtis-TRI]

Initial comments addressed.

Reviewable status: 5 unresolved discussions, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

---

systems/sensors/image_writer.h, line 85 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

Very nice documentation. Thanks, @SeanCurtis-TRI!

@thduynguyen Can you also look at this and say something based-on your use cases?

Thanks

---

systems/sensors/image_writer.h, line 102 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

Isn't this t = [0, 1⋅p, 2⋅p, ...] instead of t = [0, p, 1⋅p, 2⋅p, ...]?

D'oh! Yes it is.

---

systems/sensors/image_writer.h, line 105 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

Ditto for the sequence inside [].

Ditto

---

systems/sensors/image_writer.h, line 132 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

0.02 -> 0.02 s.

Done

---

systems/sensors/image_writer.cc, line 103 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

nit: wrong indent.

I thought the same, but it's what clang format produces.

[kunimatsu-tri]

Reviewed 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

---

systems/sensors/image_writer.cc, line 103 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I thought the same, but it's what clang format produces.

OK, never mind then.

[kunimatsu-tri]

Reviewed all the file. PTAL

Reviewed 1 of 9 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

---

systems/sensors/test/image_writer_test.cc, line 396 at r1 (raw file):

  // there.
  test_file_name(writer, "/hard/coded/file.txt", PixelType::kRgba8U, 0, "port",
                 0, "/hard/coded/file.txt");

Shouldn't the expected string be "/hard/coded/file.txt.png"?

---

systems/sensors/test/image_writer_test.cc, line 439 at r1 (raw file):

      test_file_name(writer, "/hard/{port}/file.png", PixelType::kRgba8U, 0,
                     "port", 0, "/hard/{port}/file.png"),
      std::runtime_error);

IMO, comparing directly with fmt::format_erroris better than doing so with its superclass, so that we can make sure that the exception is definitely coming from where we expected.

---

systems/sensors/test/image_writer_test.cc, line 492 at r1 (raw file):

  spruce::path path2(temp_dir());
  path.append("alt_file_{count:3}");

Shouldn't this be path2.append instead of path.append, although it seems that this won't affect the test result below?

[thduynguyen]

Reviewed 7 of 9 files at r1, 1 of 1 files at r2.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

---

systems/sensors/image_writer.h, line 85 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Thanks

Very detailed indeed! Thanks, @SeanCurtis-TRI!

---

systems/sensors/image_writer.cc, line 23 at r2 (raw file):

namespace sensors {

using std::move;

BTW This might be redundant.

---

systems/sensors/image_writer.cc, line 99 at r2 (raw file):

template <PixelType kPixelType>
const InputPort<double>& ImageWriter::DeclareImageInputPort(
    std::string port_name, std::string file_name_format, double publish_period,

BTW May I ask why you don't use const ref for port_name here?

[SeanCurtis-TRI]

Comments addressed. PTAL.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee kunimatsu-tri, platform LGTM missing

---

systems/sensors/image_writer.h, line 85 at r1 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

Very detailed indeed! Thanks, @SeanCurtis-TRI!

Done.

---

systems/sensors/image_writer.cc, line 23 at r2 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

BTW This might be redundant.

Done

---

systems/sensors/image_writer.cc, line 99 at r2 (raw file):

Previously, thduynguyen (Duy-Nguyen Ta) wrote…

BTW May I ask why you don't use const ref for port_name here?

OK This is part of super secret modern C++. If you're planning on taking ownership of the string, rather than the traditional const ref, by passing a value, when this function gets called with an r-value string, no copying will happen. (If passed with an l-value, then it is copied once.) So, worst case it's no worse than the traditional. Best case it's faster.

---

systems/sensors/test/image_writer_test.cc, line 396 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

Shouldn't the expected string be "/hard/coded/file.txt.png"?

Nope. That's what the preceding comment was supposed to communicate. As I looked at it, the comment and test are just confusing and redundant. I've removed them.

---

systems/sensors/test/image_writer_test.cc, line 439 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

IMO, comparing directly with fmt::format_erroris better than doing so with its superclass, so that we can make sure that the exception is definitely coming from where we expected.

Agreed

---

systems/sensors/test/image_writer_test.cc, line 492 at r1 (raw file):

Previously, kunimatsu-tri (Kunimatsu Hashimoto) wrote…

Shouldn't this be path2.append instead of path.append, although it seems that this won't affect the test result below?

True and true. But better to be consistent.

[kunimatsu-tri]

Thanks!

Reviewed 2 of 2 files at r3.
Reviewable status: all discussions resolved, platform LGTM missing

[SeanCurtis-TRI]

+@sherm1 in the hope that we can get this merged before end of day on Monday (it'd be nice to get this merged before I disappear for a week).

Reviewable status: all discussions resolved, platform LGTM missing

[thduynguyen]

Reviewable status: all discussions resolved, LGTM missing from assignee sherm1, platform LGTM missing

---

systems/sensors/image_writer.cc, line 99 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

OK This is part of super secret modern C++. If you're planning on taking ownership of the string, rather than the traditional const ref, by passing a value, when this function gets called with an r-value string, no copying will happen. (If passed with an l-value, then it is copied once.) So, worst case it's no worse than the traditional. Best case it's faster.

Cool! Thanks for the explanation!

[thduynguyen]

Reviewable status: all discussions resolved, LGTM missing from assignee sherm1, platform LGTM missing

[sherm1]

Platform pending a few minor issues and some comments.

Reviewed 6 of 9 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Reviewable status: 8 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

/** Writes the color (8-bit, RGBA) image data to disk.  */
void SaveToPng(const std::string& file_path, const ImageRgba8U& image);

BTW would be more in the GSG flavor for the "output" parameter (the file represented by its name) to be the final argument. Please consider reversing the arguments for these 3 functions.

---

systems/sensors/image_writer.h, line 43 at r3 (raw file):

/** Writes the label (16-bit) image data to disk.  */
void SaveToPng(const std::string& file_path, const ImageLabel16I& image);

minor: please document the handling of file extensions for these methods (always adds .png? never adds anything? adds .png only if it isn't already there? adds .png only if there is no extension at all?)

---

systems/sensors/image_writer.h, line 55 at r3 (raw file):

   - input image format (which, in turn, implies a file-system format)
   - port name (which needs to be unique across all input ports in this system)
   - context time at which output starts

BTW consider commas and a final period for this list (I'm not sure which way is better). The list below has periods after each item, and the one in the doxygen for DeclareImageInputPort() has commas and a final period.

---

systems/sensors/image_writer.h, line 76 at r3 (raw file):

 that function's documentation for elaboration on how to configure image output.
 It is important to note, that every declared image input port _must_ be
 connected, otherwise, attempting to write an image from that port, will cause

nit "connected," -> "connected;" (or split into two sentences)

---

systems/sensors/image_writer.h, line 95 at r3 (raw file):

   Each port is evaluated independently, so that two ports on the same
   %ImageWriter can write images to different locations at different frequency,

nit frequency -> frequencies

---

systems/sensors/image_writer.h, line 120 at r3 (raw file):

                       invocation of Publish(), represented as a double.
     - `time_usec`   - The time (in microseconds) stored in the context at the
                       invocation of Publish(), represented as an integer.

minor: a 32 bit integer counting microseconds goes negative in 40 minutes which wouldn't be enough for some plausible home applications (e.g.). Please consider storing this in a 64 bit integer if you aren't already so we don't have a Y2K problem to deal with later.

(Later) Oh, I see you did use an int64_t. In that case please document so user won't have to worry the way I did.

---

systems/sensors/image_writer.h, line 130 at r3 (raw file):

   File names can then be specified as shown in the following examples (assuming
   the port was declared as a color image port, with a name of "my_port", a
   period of 0.02 s (50 Hz), and a start time of 5 s.

BTW typically no space before units 0.02s, etc.

---

systems/sensors/image_writer.h, line 182 at r3 (raw file):

                                  documentation above for definition),
                               2. `publish_period` is not positive,
                               3. `port_name` is used by a previous input port.

BTW possible formatting trouble per #9912.

---

systems/sensors/image_writer.h, line 196 at r3 (raw file):

#endif

  // Does the work of writing image i to the disk.

minor: i -> index

---

systems/sensors/image_writer.h, line 223 at r3 (raw file):

  // The per-input port data.
  struct ImagePortData {

BTW this name confused me at first because "image port data" sounds like what comes into the image port, i.e. a frame. This seems more like a specification. Consider InputPortSpec or InputPortInfo instead?

---

systems/sensors/image_writer.cc, line 103 at r3 (raw file):

                    kPixelType == PixelType::kDepth32F ||
                    kPixelType == PixelType::kLabel16I,
                "ImageWriter::DeclareImageInputPort() the only supported "

nit: add a ":" after "()"

---

systems/sensors/image_writer.cc, line 115 at r3 (raw file):

  if (!IsDirectoryValid(test_dir)) {
    throw std::logic_error(
        fmt::format("ImageWriter: The format string `{}` implied the invalid "

BTW did you want the function name in the message? (Here and above)

---

systems/sensors/image_writer.cc, line 215 at r3 (raw file):

    }
  } else {
    drake::log()->error("ImageWriter: the provided folder doesn't exist: " +

minor: Why are you using log() here rather than throw as for the other error conditions? I would rather see an std::runtime_error unless there is some reason that won't work?

---

systems/sensors/test/image_writer_test.cc, line 23 at r3 (raw file):

#include "drake/systems/framework/event_collection.h"

// NOTE: This is a limited test of ImageWriter functionality.

minor: please say what about this is "limited"

---

systems/sensors/test/image_writer_test.cc, line 111 at r3 (raw file):

template <>
Image<PixelType::kLabel16I> test_image() {
  // Creates a simple 4x1 image consisting of: 0, 1, 2, 3.

BTW should this be 0 100 200 300 ?

[SeanCurtis-TRI]

Comments addressed (bar one). PTAL

Reviewable status: 2 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW would be more in the GSG flavor for the "output" parameter (the file represented by its name) to be the final argument. Please consider reversing the arguments for these 3 functions.

You've lost me on this one. There are no output parameters. Can you elaborate?

---

systems/sensors/image_writer.h, line 43 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: please document the handling of file extensions for these methods (always adds .png? never adds anything? adds .png only if it isn't already there? adds .png only if there is no extension at all?)

Clarified.

---

systems/sensors/image_writer.h, line 55 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider commas and a final period for this list (I'm not sure which way is better). The list below has periods after each item, and the one in the doxygen for DeclareImageInputPort() has commas and a final period.

Done

---

systems/sensors/image_writer.h, line 76 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit "connected," -> "connected;" (or split into two sentences)

Done

---

systems/sensors/image_writer.h, line 95 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit frequency -> frequencies

Done

---

systems/sensors/image_writer.h, line 120 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: a 32 bit integer counting microseconds goes negative in 40 minutes which wouldn't be enough for some plausible home applications (e.g.). Please consider storing this in a 64 bit integer if you aren't already so we don't have a Y2K problem to deal with later.

(Later) Oh, I see you did use an int64_t. In that case please document so user won't have to worry the way I did.

Done

---

systems/sensors/image_writer.h, line 130 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW typically no space before units 0.02s, etc.

I really want a half space....but I feel a full space is less a sin than no-space.

---

systems/sensors/image_writer.h, line 182 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW possible formatting trouble per #9912.

Done

---

systems/sensors/image_writer.h, line 196 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: i -> index

Done

---

systems/sensors/image_writer.h, line 223 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW this name confused me at first because "image port data" sounds like what comes into the image port, i.e. a frame. This seems more like a specification. Consider InputPortSpec or InputPortInfo instead?

Done

---

systems/sensors/image_writer.cc, line 103 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: add a ":" after "()"

Done

---

systems/sensors/image_writer.cc, line 115 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW did you want the function name in the message? (Here and above)

OK The majority of the exceptions are thrown in private methods -- so, for unity, I eliminated method names in the exceptions.

---

systems/sensors/image_writer.cc, line 215 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: Why are you using log() here rather than throw as for the other error conditions? I would rather see an std::runtime_error unless there is some reason that won't work?

For one very minor reason -- throwing here doesn't have access to the actual user-provided string, only the test string. I wanted the error message to include both (see the error message thrown above).

That said, the logging is a bit of a relic from a previous incarnation. I'll change it to return an enumeration so I can provide reason and data in the thrown error.

---

systems/sensors/test/image_writer_test.cc, line 23 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: please say what about this is "limited"

Done (another relic of older code).

---

systems/sensors/test/image_writer_test.cc, line 111 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW should this be 0 100 200 300 ?

Done

[jwnimmer-tri]

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You've lost me on this one. There are no output parameters. Can you elaborate?

Sherm is saying that the files written to by this function are an "output" of this function, so the file_path argument should be last.

I strongly disagree that GSG is at all relevant for this question (it is only about output arguments ala @param[out]), but I do think its still fair for you to consider which order is clearest.

(I don't have an opinion on which is clearest.)

[SeanCurtis-TRI]

Reviewable status: 3 unresolved discussions, platform LGTM from [sherm1]

a discussion (no related file):
@drake-jenkins-bot linux-xenial-clang-bazel-experimental-debug please

---

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Sherm is saying that the files written to by this function are an "output" of this function, so the file_path argument should be last.

I strongly disagree that GSG is at all relevant for this question (it is only about output arguments ala @param[out]), but I do think its still fair for you to consider which order is clearest.

(I don't have an opinion on which is clearest.)

@jwnimmer-tri that was my guess, I wanted to see if he had something more GSG-specific than that in case there was something I'd missed.

[SeanCurtis-TRI]

@drake-jenkins-bot linux-xenial-clang-bazel-experimental-debug please

[sherm1]

Reviewable status: 3 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

@jwnimmer-tri that was my guess, I wanted to see if he had something more GSG-specific than that in case there was something I'd missed.

Nope -- not asserting a violation of anything, just that the file name is serving an "output-like" role so if you want to maintain an input->output flow in the arguments putting it last has more of that flavor.

[SeanCurtis-TRI]

Reviewable status: 3 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Nope -- not asserting a violation of anything, just that the file name is serving an "output-like" role so if you want to maintain an input->output flow in the arguments putting it last has more of that flavor.

I had to rebase onto master. So, as long as I had to re-run CI and since I don't care, I'll defer to the opinion of someone who does care.

[jamiesnape]

Reviewable status: 4 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 14 at r5 (raw file):

#include <vector>

#include <spruce.hh>

Is this intended to be a public header? If so, I think this may be the first time that spruce is used from a public header, therefore we would need to install it.

[jwnimmer-tri]

---

systems/sensors/image_writer.h, line 14 at r5 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Is this intended to be a public header? If so, I think this may be the first time that spruce is used from a public header, therefore we would need to install it.

Good catch. It's only used privately. It's a defect to use spruce in headers. We should fix the code so that include is in the cc file (by only using std::string in this header).

[jamiesnape]

Reviewable status: 4 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 14 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Good catch. It's only used privately. It's a defect to use spruce in headers. We should fix the code so that include is in the cc file (by only using std::string in this header).

Yes, the best solution. I need to add something to CI to catch this sort of thing.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 14 at r5 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Yes, the best solution. I need to add something to CI to catch this sort of thing.

Relates #7451.

[SeanCurtis-TRI]

Reviewable status: 4 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 14 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Relates #7451.

I'm glad you caught it.

[jwnimmer-tri]

Dismissed @jamiesnape from a discussion.
Reviewable status: 3 unresolved discussions, platform LGTM from [sherm1]

[SeanCurtis-TRI]

@drake-jenkins-bot linux-xenial-clang-bazel-experimental-debug please

[sherm1]

Reviewable status: 3 unresolved discussions, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 35 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I had to rebase onto master. So, as long as I had to re-run CI and since I don't care, I'll defer to the opinion of someone who does care.

Thanks!

[sherm1]

Reviewed 1 of 4 files at r5, 3 of 3 files at r6.
Reviewable status: 1 unresolved discussion, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 196 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Done

Still says i but the parameter name is index.

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, platform LGTM from [sherm1]

---

systems/sensors/image_writer.h, line 196 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Still says i but the parameter name is index.

Done