Replace CanvasTransformer concept with PixelMapper.

The CanvasTransformer was meant to be more universal, but this complexity
was adding unnecesary complexity.

Introduce the new concept of a PixelMapper that with just the implementation
of a simple mapping function allows the same thing.
In this first step, the multiplex transformers are converted to the new scheme
and simplify them in the process. Only one place to add a multiplex transformer,
then the rest (command line support) works automatically.

Next steps:
  o replace the old UArrangementTransformer and RotateTransformer with
    the new scheme and only use these in all the demos and utilities.
  o Since PixelMappers now have a name, it is possible to implement a scheme
    by which they are registered and mentioned on the command line (for simple
    use in other language bindings).

include/canvas.h

@@ -48,26 +48,15 @@ class Canvas {
   virtual void Fill(uint8_t red, uint8_t green, uint8_t blue) = 0;
 };
 
-// A canvas transformer is an object that, given a Canvas, returns a
-// canvas that applies transformations before writing to the original
-// Canvas.
+#ifndef REMOVE_DEPRECATED_TRANSFORMERS
+// Canvas Transformer used to be a way to map pixels in the LED matrix.
+// It was an overdesigned concept and required some confusing boilerplate
+// to set-up, but was too compilcated for what it was used for: mapping
+// pixels from some logical mapping to some physical mapping.
 //
-// To simplify assumptions for implementations and users of implementations,
-// the following conditions and constraints apply:
+// These days, this is done by rgb_matrix::PixelMapper, see pixel-mapper.h
 //
-//   1) The CanvasTransformer _never_ takes ownership of the delegatee.
-//   2) The ownership of the returned Canvas is _not_ passed to the caller.
-//   3) The returned Canvas can only be assumed to be valid for the lifetime
-//      of the CanvasTranformer and the lifetime of the original canvas.
-//   4) The returned canvas is only valid up to the next call of Transform().
-//
-//   * Point 2)-4) imply that the CanvasTransformer can hand out the same
-//     canvas object every time, just configured to write to the new
-//     delegatee.
-//   * Point 4) implies that one instance of CanvasTransformer cannot be used in
-//     parallel in multiple threads calling Transform().
-//   * The constraints also imply that it is valid for a CanvasTransformer to
-//     return the passed in canvas itself.
+// Don't use CanvasTransformer in new code.
 class CanvasTransformer {
 public:
   virtual ~CanvasTransformer() {}
@@ -76,6 +65,7 @@ class CanvasTransformer {
   // the output canvas.
   virtual Canvas *Transform(Canvas *output) = 0;
 };
+#endif
 
 }  // namespace rgb_matrix
 #endif  // RPI_CANVAS_H

include/led-matrix.h

@@ -34,7 +34,7 @@ class RGBMatrix;
 class FrameCanvas;   // Canvas for Double- and Multibuffering
 namespace internal {
 class Framebuffer;
-class PixelMapper;
+class PixelDesignatorMap;
 }
 
 // The RGB matrix provides the framebuffer and the facilities to constantly
@@ -241,36 +241,10 @@ class RGBMatrix : public Canvas {
   // 28Hz animation, nicely locked to the frame-rate).
   FrameCanvas *SwapOnVSync(FrameCanvas *other, unsigned framerate_fraction = 1);
 
-  // Set image transformer that maps the logical canvas coordinates to the
-  // physical canvas coordinates.
-  // This preprocesses the transformation for static pixel mapping once.
-  //
-  // (In the rate case that you have transformers that dynamically change
-  //  their behavior at runtime or do transformations on the color, you have to
-  //  manually use them to wrap canvases.)
-  void ApplyStaticTransformer(const CanvasTransformer &transformer);
-
-  // Don't use this function anymore, use ApplyStaticTransformer() instead.
-  // See demo-main.cc how.
-  //
-  // This used to somewhat work with dynamic tranformations, but it
-  // was confusing as that didn't apply to FrameCanvases as well.
-  // If you have static transformations that can be done at program start
-  // (such as rotation or creating your particular pysical display mapping),
-  // use ApplyStaticTransformer().
-  // If you use the Transformer concept to modify writes to canvases on-the-fly,
-  // use them directly as such.
-  //
-  // DO NOT USE. WILL BE REMOVED.
-  void SetTransformer(CanvasTransformer *t) __attribute__((deprecated)) {
-    transformer_ = t;
-    if (t) ApplyStaticTransformer(*t);
-  }
-
-  // DO NOT USE. WILL BE REMOVED.
-  CanvasTransformer *transformer() __attribute__((deprecated)) {
-    return transformer_;
-  }
+  // Apply a pixel mapper. This is used to re-map pixels according to some
+  // scheme implemented by the PixelMapper.
+  // Returns a boolean indicating if this was successful.
+  bool ApplyPixelMapper(const PixelMapper &mapper);
 
   // -- Canvas interface. These write to the active FrameCanvas
   // (see documentation in canvas.h)
@@ -281,10 +255,30 @@ class RGBMatrix : public Canvas {
   virtual void Clear();
   virtual void Fill(uint8_t red, uint8_t green, uint8_t blue);
 
+
+#ifndef REMOVE_DEPRECATED_TRANSFORMERS
+  //--- deprecated section: transformers. Use PixelMapper instead.
+  void ApplyStaticTransformer(const CanvasTransformer &transformer) __attribute__((deprecated)) {
+    ApplyStaticTransformerDeprecated(transformer);
+  }
+  void SetTransformer(CanvasTransformer *t) __attribute__((deprecated)) {
+    transformer_ = t;
+    if (t) ApplyStaticTransformerDeprecated(*t);
+  }
+  CanvasTransformer *transformer() __attribute__((deprecated)) {
+    return transformer_;
+  }
+  // --- end deprecated section.
+#endif  // INCLUDE_DEPRECATED_TRANSFORMERS
+
 private:
   class UpdateThread;
   friend class UpdateThread;
 
+#ifndef REMOVE_DEPRECATED_TRANSFORMERS
+  void ApplyStaticTransformerDeprecated(const CanvasTransformer &transformer);
+#endif  // REMOVE_DEPRECATED_TRANSFORMERS
+
   Options params_;
   bool do_luminance_correct_;
 
@@ -295,7 +289,7 @@ class RGBMatrix : public Canvas {
   CanvasTransformer *transformer_;  // deprecated. To be removed.
   UpdateThread *updater_;
   std::vector<FrameCanvas*> created_frames_;
-  internal::PixelMapper *shared_pixel_mapper_;
+  internal::PixelDesignatorMap *shared_pixel_mapper_;
 };
 
 class FrameCanvas : public Canvas {

include/pixel-mapper.h

@@ -0,0 +1,84 @@
+// -*- mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; -*-
+// Copyright (C) 2018 Henner Zeller <[email protected]>
+//
+// This program is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation version 2.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://gnu.org/licenses/gpl-2.0.txt>
+#ifndef RGBMATRIX_PIXEL_MAPPER
+#define RGBMATRIX_PIXEL_MAPPER
+
+#include <string>
+
+namespace rgb_matrix {
+
+// A pixel mapper is a way for you to map pixels of LED matrixes to a different
+// layout. If you have an implementation of a PixelMapper, you can give it
+// to the RGBMatrix::ApplyPixelMapper(), which then presents you a canvas
+// that has the new "visible_width", "visible_height".
+class PixelMapper {
+public:
+  virtual ~PixelMapper() {}
+
+  // Get the name of this PixelMapper. Each PixelMapper needs to have a name
+  // so that it can be referred to with command line flags.
+  virtual const char *GetName() const = 0;
+
+  // Pixel mappers might receive optional parameters, e.g. from command
+  // line flags.
+  // This is a single string containing the parameters.
+  // You can be used from simple scalar parameters, such as the angle for
+  // the rotate transformer, or more complex parameters that describe a mapping
+  // of panels for instance.
+  // Keep it concise (as people will give parameters on the command line) and
+  // don't use semicolons in your string (as they are
+  // used to separate pixel mappers on the command line).
+  //
+  // For instance, the rotate transformer is invoked like this
+  //  --led-pixel-mapper=rotate:90
+  // And the parameter that is passed to SetParameter() is "90".
+  //
+  // Returns 'true' if parameter was parsed successfully.
+  virtual bool SetParameter(const std::string &parameter_string) {
+    return parameter_string.empty();  // Default: expecting no parameter.
+  }
+
+  // Given a underlying matrix (width, height), returns the
+  // visible (width, height) after the mapping.
+  // E.g. a 90 degree rotation might map matrix=(64, 32) -> visible=(32, 64)
+  // Some multiplexing matrices will double the height and half the width.
+  //
+  // While not technically necessary, one would expect that the number of
+  // pixels stay the same, so
+  // matrix_width * matrix_height == (*visible_width) * (*visible_height);
+  //
+  // Returns boolean "true" if the mapping can be successfully done with this
+  // mapper.
+  virtual bool GetSizeMapping(int matrix_width, int matrix_height,
+                              int *visible_width, int *visible_height)
+    const = 0;
+
+  // Map where a visible pixel (x,y) is mapped to the underlying matrix (x,y).
+  //
+  // To be convienently stateless, the first parameters are the full
+  // matrix width and height.
+  //
+  // So for many multiplexing methods this means to map a panel to a double
+  // length and half height panel (32x16 -> 64x8).
+  // The logic_x, logic_y are output parameters and guaranteed not to be
+  // nullptr.
+  virtual void MapVisibleToMatrix(int matrix_width, int matrix_height,
+                                  int visible_x, int visible_y,
+                                  int *matrix_x, int *matrix_y) const = 0;
+};
+
+}  // namespace rgb_matrix
+
+#endif  // RGBMATRIX_PIXEL_MAPPER

include/transformer.h

@@ -14,13 +14,24 @@
 // You should have received a copy of the GNU General Public License
 // along with this program.  If not, see <http://gnu.org/licenses/gpl-2.0.txt>
 
+/*
+ * NOTE:
+ *
+ * Transformers are deprecated. For the kind of mappings they were be
+ * used by they turned out to be too complicated.
+ *
+ * They have been superseeded by the simpler PixelMapper, see pixel-mapper.h
+ */
 #ifndef RPI_TRANSFORMER_H
 #define RPI_TRANSFORMER_H
 
+#ifndef REMOVE_DEPRECATED_TRANSFORMERS
+
 #include <vector>
 #include <cstddef>
 
 #include "canvas.h"
+#include "pixel-mapper.h"
 
 namespace rgb_matrix {
 
@@ -116,4 +127,5 @@ class LargeSquare64x64Transformer : public CanvasTransformer {
 
 } // namespace rgb_matrix
 
-#endif // RPI_TRANSFORMER_H
+#endif  // REMOVE_DEPRECATED_TRANSFORMERS
+#endif  // RPI_TRANSFORMER_H

lib/Makefile

@@ -5,7 +5,8 @@
 ##
 OBJECTS=gpio.o led-matrix.o options-initialize.o framebuffer.o \
         thread.o bdf-font.o graphics.o transformer.o led-matrix-c.o \
-	hardware-mapping.o content-streamer.o multiplex-transformers.o
+	hardware-mapping.o content-streamer.o multiplex-mappers.o
+
 TARGET=librgbmatrix
 
 # There are several different pinouts for various breakout boards that uses
@@ -95,6 +96,12 @@ HARDWARE_DESC?=regular
 # Flag: --led-no-hardware-pulses
 #DEFINES+=-DDISABLE_HARDWARE_PULSES
 
+# If defined, remove deprecated transformers from the API.
+# This will eventually be disabled by default and at some point removed
+# from the code-base, so don't use transformers. Use PixelMappers
+# instead (include/pixel-mapper.h)
+#DEFINES+=-DREMOVE_DEPRECATED_TRANSFORMERS
+
 # ---- Pinout options for hardware variants; usually no change needed here ----
 
 # Uncomment if you want to use the Adafruit HAT with stable PWM timings.

lib/framebuffer-internal.h

@@ -37,13 +37,13 @@ struct PixelDesignator {
   uint32_t mask;
 };
 
-class PixelMapper {
+class PixelDesignatorMap {
 public:
-  PixelMapper(int width, int height);
-  ~PixelMapper();
+  PixelDesignatorMap(int width, int height);
+  ~PixelDesignatorMap();
 
   // Get a writable version of the PixelDesignator. Outside Framebuffer used
-  // by the RGBMatrix to re-assign mappings to new PixelMappers.
+  // by the RGBMatrix to re-assign mappings to new PixelDesignatorMappers.
   PixelDesignator *get(int x, int y);
 
   inline int width() const { return width_; }
@@ -64,7 +64,7 @@ class Framebuffer {
   Framebuffer(int rows, int columns, int parallel,
               int scan_mode,
               const char* led_sequence, bool inverse_color,
-              PixelMapper **mapper);
+              PixelDesignatorMap **mapper);
   ~Framebuffer();
 
   // Initialize GPIO bits for output. Only call once.
@@ -144,7 +144,7 @@ class Framebuffer {
   gpio_bits_t *bitplane_buffer_;
   inline gpio_bits_t *ValueAt(int double_row, int column, int bit);
 
-  PixelMapper **shared_mapper_;  // Storage in RGBMatrix.
+  PixelDesignatorMap **shared_mapper_;  // Storage in RGBMatrix.
 };
 }  // namespace internal
 }  // namespace rgb_matrix

lib/framebuffer.cc

@@ -45,18 +45,18 @@ static PinPulser *sOutputEnablePulser = NULL;
 #  define SUB_PANELS_ 2
 #endif
 
-PixelDesignator *PixelMapper::get(int x, int y) {
+PixelDesignator *PixelDesignatorMap::get(int x, int y) {
   if (x < 0 || y < 0 || x >= width_ || y >= height_)
     return NULL;
   return buffer_ + (y*width_) + x;
 }
 
-PixelMapper::PixelMapper(int width, int height)
+PixelDesignatorMap::PixelDesignatorMap(int width, int height)
   : width_(width), height_(height),
     buffer_(new PixelDesignator[width * height]) {
 }
 
-PixelMapper::~PixelMapper() {
+PixelDesignatorMap::~PixelDesignatorMap() {
   delete [] buffer_;
 }
 
@@ -145,9 +145,9 @@ class ShiftRegisterRowAddressSetter : public RowAddressSetter {
   int last_row_;
 };
 
-// The DirectABCDRowAddressSetter sets the address by one of 
+// The DirectABCDRowAddressSetter sets the address by one of
 // row pin ABCD for 32х16 matrix 1:4 multiplexing. The matrix has
-// 4 addressable rows. Row is selected by a low level on the 
+// 4 addressable rows. Row is selected by a low level on the
 // corresponding row address pin. Other row address pins must be in high level.
 //
 // Row addr| 0 | 1 | 2 | 3
@@ -161,7 +161,7 @@ class DirectABCDLineRowAddressSetter : public RowAddressSetter {
   DirectABCDLineRowAddressSetter(int double_rows, const HardwareMapping &h)
     : last_row_(-1) {
 	row_mask_ = h.a | h.b | h.c | h.d;
-		
+
 	row_lines_[0] = /*h.a |*/ h.b | h.c | h.d;
 	row_lines_[1] = h.a /*| h.b*/ | h.c | h.d;
 	row_lines_[2] = h.a | h.b /*| h.c */| h.d;
@@ -172,7 +172,7 @@ class DirectABCDLineRowAddressSetter : public RowAddressSetter {
 
   virtual void SetRowAddress(GPIO *io, int row) {
     if (row == last_row_) return;
-     
+
     gpio_bits_t row_address = row_lines_[row % 4];
 
     io->WriteMaskedBits(row_address, row_mask_);
@@ -181,7 +181,7 @@ class DirectABCDLineRowAddressSetter : public RowAddressSetter {
 
 private:
   gpio_bits_t row_lines_[4];
-  gpio_bits_t row_mask_;  
+  gpio_bits_t row_mask_;
   int last_row_;
 };
 
@@ -193,7 +193,7 @@ RowAddressSetter *Framebuffer::row_setter_ = NULL;
 Framebuffer::Framebuffer(int rows, int columns, int parallel,
                          int scan_mode,
                          const char *led_sequence, bool inverse_color,
-                         PixelMapper **mapper)
+                         PixelDesignatorMap **mapper)
   : rows_(rows),
     parallel_(parallel),
     height_(rows * parallel),
@@ -227,7 +227,7 @@ Framebuffer::Framebuffer(int rows, int columns, int parallel,
   // Newly created PixelMappers then can just copy around PixelDesignators
   // from the parent PixelMapper opaquely without having to know the details.
   if (*shared_mapper_ == NULL) {
-    *shared_mapper_ = new PixelMapper(columns_, height_);
+    *shared_mapper_ = new PixelDesignatorMap(columns_, height_);
     for (int y = 0; y < height_; ++y) {
       for (int x = 0; x < columns_; ++x) {
         InitDefaultDesignator(x, y, (*shared_mapper_)->get(x, y));