revid/config: Common camera interface

[Alan Noble]

With our newfound multiplicity of camera types (which is likely to increase over time) a Camera interface capturing common camera methods, such as Start, Stop and Set. Revid would have a cam(era) member which is set to the requested camera type (that implements Camera), e.g.,

‌

switch cameraType:
case "Pi":
  r.cam = new(PiCamera)   // Implements Camera.
case "RTSP":
  r.cam = new(RTSPCamera) // Ditto.

Then calls like

r.startRTSPCamera()

become

r.cam.Start()

‌

[Saxon Milton]

Did you have something in mind for configuration ? I guess one of the only nice things about having methods like:

‌

func (r *Revid) startRTSPCamera() {
  ...
  gvctrl.Set(Codec(r.config.Codec),...)
  ...
}

is that we had easy access to the revid config and could easily set stuff.

I think now we will need each type that implements the Camera interface to provide a constructor where options can be passed.

For example,

‌

switch r.config.Input {
case PiCamera:
  // picam package provides an implementation of the Camera interface for the pi camera.
  r.Camera = picam.NewCamera(
    picam.Codec(r.config.Codec),
    picam.Height(r.config.Height),
    picam.Bitrate(r.config.Bitrate),
    ...,
  )
case GeoVision:
  // gvcam package provides an implementation of the Camera interface for the GeoVision camera.
  r.Camera = gvcam.NewCamera(
    gvcam.Codec(r.config.Codec),
    gvcam.Height(r.config.Height),
    gvcam.Bitrate(r.config.Bitrate),
    ...,
  )
}

Btw, revid isn’t only dealing with Camera input devices, so should our interface be Input, InputDevice or Device instead ? That way we can also have hydrophones for example also implement this interface ?

[Alan Noble]

Actually, I was thinking that the configuration would be decoupled from the instantiation/construction using the Set method that I mentioned would be part of the interface. Perhaps Set could take a map of configuration options, or perhaps it could use variadic options.

‌

cam = new(<CameraType>)
...
cam.Set({Codec:r.config.Codec, Height:r.config.Height})

‌

As a corollary, I think config params, whether command line flags or NetReceiver vars, should be passed through to the Camera value via its Set method, leaving it up to the former to perform validation. Set would naturally return an error, indeed, a multi error.

[Saxon Milton]

oh I see! Is it preferable to have it decoupled from the instantiation? Revid sets up the pipeline everytime Revid.Start is called; in other words the camera would be instantiated every time start is called, so it would actually save some lines if we just did configuration then. What do you think ?

Could validation of the passed options and relating parameters similarly be done by the constructors of a particular Camera interface implementation?

[Alan Noble]

And yes, Camera can be generalized to include audio. I think Device is too general though. Even InputDevice is rather general. Since we’re talking about AV input devices with codecs, etc., how about AudioVisualDevice or AVDevice?

[Saxon Milton]

AVDevice sounds cool

[Alan Noble]

Unless I’m missing something by factoring Set so it is separate from construction the code should be simpler. Only construction is subject to the switch statement. Everything else (Set, Start, Stop) should be outside of the switch and therefore common code. This is a pretty common design pattern.

[Alan Noble]

As for the revid pipeline, it may be time to bite the bullet and implement more flexible control than having to stop and restart a device every single time a config param is changed. This was done for the Pi camera only because we’re using raspivid as an external program. It should not be necessary for the GV camera. Indeed, if we ever implemented a low-level Pi Camera interface (bypassing the external program), we could likewise dispense with the stopping and starting.

This could be accomplished with an Update method on AVDevice. The AVDevice implementation would determine whether stopping and starting is required, not revid.

On a related point, perhaps we should call the AVDevice that implements the Pi Camera Raspivid rather than PiCamera, reserving the latter for a future low-level Pi Camera interface.

[Saxon Milton]

If Set is to be common code, where we will pass the revid config and not take into consideration the implementation, then each implementation of the AVDevice will have to filter out what it wants to use (which isn’t good right?), because as Dan put it once, each device has its own knobs. So the alternative is that we still have a common Set call in one place, but we have to add logic to give the Set function the right fields from the config, which would probably be a similar switch to that of initialisation. So it would end up where we double up on the switch statement. So we would actually have less code if we just did initialisation and configuration in the same place right ?

[Alan Noble]

Putting the implementation in charge of what it accepts or does not accept is a GOOD THING. Put another way, it is precisely because the different AVDevice implementations have different capabilities, that we want to delegate configuration requests to them, not have callers worry too about it. Callers need only worry about handling the errors if they request a capability that is not implemented.

Besides, it not so much that the knobs are all that different, but what one can do with them that is different. To use a radio analogy, all radios have a knob to control the frequency, but some radios can tune into more frequencies than others.

[Alan Noble]

By analogy, think if the io.Reader or io.Writer interfaces required callers to have “insider knowledge” about the implementation and handle different readers/writers differently. It would be considerably more complex than the current scheme.

[Saxon Milton]

Yes I agree with that completely. My concern was that if we had dedicated packages for each AVDevice, and they each implemented a function like:

func Set(c *revid.Config) error

that if revid.Config changed in any way (fields renamed, fields removed) then each and every one of those packages would also have to change to accommodate. Also if we were putting each one in it’s own package in the first place, then I’m guessing we’re interested in creating some nice APIs, and in that case I would personally stay away from configuration structs.

However, if our implementations of the AVDevice interface are just defined in the revid package then this seems fine to me.

[Saxon Milton]

I guess even if they were in dedicated packages it wouldn’t be the end of the world. In either case, you’re right, the configuration could happen with a single call to Set and would be simpler.

[Alan Noble]

I wasn’t suggesting that the Set method take a configuration struct though, let alone one that introduces a circular dependency. I was suggesting something more generic, such as some form of name/value pairs (options), e.g., either a map or a variadic list of options.

Revid will naturally depend on AVDevice, not the other way round.

We can also write unit tests for AVDevice without revid.

[Saxon Milton]

I’m not sure I can see how it could be made more generic, might you be able to to give an example of what you had in mind ?

[Alan Noble]

Ultimately, cameras and other AV devices take simply numbers and strings for their various parameter values. Therefore if we define a generic Option struct we can use a collection of those rather than a mega struct. For example, instead of passing in a config struct like:

struct myConfig {
   codec: codecH264
   resolution: res1080p
   ...
}
Set(myConfig)

For now let’s just define our Option as:

type Option struct {
    Name, Value string
}

Then the call to Set would be simply something like:

Set(Option{“codec”,”H264”}, Option{“resolution”,"1080p"})

where the Set implementation looks like the following:

func (av *Raspivid)Set(opts ...Option)

All option parsing is left up to the AVDevice, but of course there should be common helper functions.

Or said Option could alternatively be Rob Pike’s self-referential options:

https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html

Either way it is more flexible and less brittle than a big config struct.

[Alan Noble]

The options could likely be passed through as []Option, which have been extracted as a group from the command-line flags or NetReceiver vars. The latter are already a map[string]string which could be readily converted into []Option. Or dispense with Option all together and just pass map[string]string to Set (which was one of my earlier suggestions).

[Saxon Milton]

Resolved in PR #270 and PR #280