| Message ID | 20200907164450.13082-1-david.plowman@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi again everyone I was wondering if anyone else had any thoughts on this. In spite of the long email which is perhaps a bit off-putting I don't think there's actually very much complicated stuff going on. Anyway, I'm keen to try and stop this discussion from getting marooned on the backlog for too long if we can! Thanks again David On Mon, 7 Sep 2020 at 17:44, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi everyone > > I wanted to return to the question of digital zoom. Rather than simply > writing a long email I've included an implementation that shows where > I am. It's changed in the light of exchanges with Jacopo and others, > and I would say it's "for discussion" at this point. Principally: > > 1. I'd like users to have access to CameraSensorInfo::outputSize, and > I'm using Camera::properties() to get it. Unfortunately it changes > with every camera mode, so will we have to do this differently? > > 2. The terminology is a bit more Android-like. Because, why not? > > 3. I'm no longer enforcing any particular method of cropping. But to > make life easier for applications I've added some geometry helpers. To > understand the rationale behind these I've included some example code > further down that I think an application might use. > > This work is currently made up of 4 commits: > > 1. The SensorOutputSize property. > > 2. The SensorCrop control. > > 3. The geometry helper functions. > > 4. Implementation in the Raspberry Pi pipeline handler. > > Example Application Code > > Assume we have: > > camera - pointer to the Camera object > streamCfg - reference to the StreamConfiguration of the output image > request - pointer to a Request object > zoomFactor - floating point zoom factor, >= 1 > dx - amount in pixels to pan left (-ve) or right (+ve) > dy - amount in pixels to pan up (-ve) or down (+ve) > > You'll want to understand the helper methods: > > Size Size::aspectRatioDown(const Size &ar) const > Returns a letter/pillarboxed version of this size. > > Size operator/(const Size &size, float f) > Downscale a Size object by f. > > Rectangle Size::centre(const Rectangle ®ion, int offsetX = 0, int offsetY = 0) const > Returns a Rectangle of this size centred at the same point as region, > optionally with offsets. > > Rectangle Rectangle::clamp(const Rectangle &boundary) const > Translates the rectangle so that no part of it lies outside boundary. > > 1. Basic zooming into the centre > > Size area = camera->properties().get(properties::SensorOutputSize); > Size size = area.aspectRatioDown(streamCfg.size) / zoomFactor; > Rectangle crop = size.centre(Rectangle(area)); > request->controls.set(controls::SensorCrop, crop); > > (I'm hoping this first one is easy enough to understand!) > > 2. Zooming anywhere in the sensor output (even into parts of the image that > were not intially visible) > > Size area = camera->properties().get(properties::SensorOutputSize); > Size size = area.aspectRatioDown(streamCfg.size) / zoomFactor; > Rectangle crop = size.centre(Rectangle(area), dx, dy).clamp(Rectangle(area)); > request->controls.set(controls::SensorCrop, crop); > > (As before, but we have offsets dx and dy to enable panning, so we > must also clamp the result to be sure it's within the sensor area.) > > 3. Zooming as before, but not allowing you to pan into anything that > wasn't initially visible > > Size area = camera->properties().get(properties::SensorOutputSize); > Rectangle maxCrop = area.aspectRatioDown(streamCfg.size).centre(area); > Size size = maxCrop.size() / zoomFactor; > Rectangle crop = size.centre(maxCrop, dx, dy).clamp(maxCrop); > request->controls.set(controls::SensorCrop, crop); > > (The subtle difference here is that we clamp the result against maxCrop, > not the full sensor area.) > > 4. Android > > This time we're given the "crop" (a Rectangle) that Android has requested. I > think Android only requires us to sort out the aspect ratio. > > Size size = crop.size().aspectRatioDown(streamCfg.size); > request->controls.set(controls::SensorCrop, size.centre(crop)); > > > That's as far as I've got. Thoughts and comments welcome! > > Best regards > David > > David Plowman (4): > libcamera: Add SensorOutputSize property > libcamera: Add SensorCrop control > libcamera: Add geometry helper functions > libcamera: pipeline: raspberrypi: Implementation of digital zoom > > include/libcamera/geometry.h | 18 +++ > include/libcamera/ipa/raspberrypi.h | 1 + > src/ipa/raspberrypi/raspberrypi.cpp | 7 + > src/libcamera/camera_sensor.cpp | 3 + > src/libcamera/control_ids.yaml | 9 ++ > src/libcamera/geometry.cpp | 129 ++++++++++++++++++ > .../pipeline/raspberrypi/raspberrypi.cpp | 41 ++++++ > src/libcamera/property_ids.yaml | 6 + > 8 files changed, 214 insertions(+) > > -- > 2.20.1 >
Hi David, On Mon, Sep 21, 2020 at 07:24:45AM +0100, David Plowman wrote: > Hi again everyone > > I was wondering if anyone else had any thoughts on this. In spite of > the long email which is perhaps a bit off-putting I don't think > there's actually very much complicated stuff going on. Anyway, I'm > keen to try and stop this discussion from getting marooned on the > backlog for too long if we can! Sorry sorry for the late reply. We also left quite some discussion pending on the previous version, let's try to catch up here. > > Thanks again > David > > On Mon, 7 Sep 2020 at 17:44, David Plowman > <david.plowman@raspberrypi.com> wrote: > > > > Hi everyone > > > > I wanted to return to the question of digital zoom. Rather than simply > > writing a long email I've included an implementation that shows where > > I am. It's changed in the light of exchanges with Jacopo and others, > > and I would say it's "for discussion" at this point. Principally: > > > > 1. I'd like users to have access to CameraSensorInfo::outputSize, and > > I'm using Camera::properties() to get it. Unfortunately it changes > > with every camera mode, so will we have to do this differently? > > I'll reply to this in 1/4. I think it's not optimal, but we should not block this feature any longer. Although, I'm afraid that application might be forced to change the way they retrieve the SensorCrop (from Camera to CameraConfiguration). How fast do yuo expect this will be adopted? Do you plan to build anything on top ? > > 2. The terminology is a bit more Android-like. Because, why not? > > :) > > 3. I'm no longer enforcing any particular method of cropping. But to > > make life easier for applications I've added some geometry helpers. To > > understand the rationale behind these I've included some example code > > further down that I think an application might use. Nice! > > > > This work is currently made up of 4 commits: > > > > 1. The SensorOutputSize property. > > > > 2. The SensorCrop control. > > > > 3. The geometry helper functions. > > > > 4. Implementation in the Raspberry Pi pipeline handler. > > Repluies in individual patches > > Example Application Code > > > > Assume we have: > > > > camera - pointer to the Camera object > > streamCfg - reference to the StreamConfiguration of the output image > > request - pointer to a Request object > > zoomFactor - floating point zoom factor, >= 1 > > dx - amount in pixels to pan left (-ve) or right (+ve) > > dy - amount in pixels to pan up (-ve) or down (+ve) > > > > You'll want to understand the helper methods: > > > > Size Size::aspectRatioDown(const Size &ar) const > > Returns a letter/pillarboxed version of this size. > > > > Size operator/(const Size &size, float f) > > Downscale a Size object by f. > > > > Rectangle Size::centre(const Rectangle ®ion, int offsetX = 0, int offsetY = 0) const > > Returns a Rectangle of this size centred at the same point as region, > > optionally with offsets. > > > > Rectangle Rectangle::clamp(const Rectangle &boundary) const > > Translates the rectangle so that no part of it lies outside boundary. > > > > 1. Basic zooming into the centre > > > > Size area = camera->properties().get(properties::SensorOutputSize); > > Size size = area.aspectRatioDown(streamCfg.size) / zoomFactor; > > Rectangle crop = size.centre(Rectangle(area)); > > request->controls.set(controls::SensorCrop, crop); > > > > (I'm hoping this first one is easy enough to understand!) > > > > 2. Zooming anywhere in the sensor output (even into parts of the image that > > were not intially visible) > > > > Size area = camera->properties().get(properties::SensorOutputSize); > > Size size = area.aspectRatioDown(streamCfg.size) / zoomFactor; > > Rectangle crop = size.centre(Rectangle(area), dx, dy).clamp(Rectangle(area)); > > request->controls.set(controls::SensorCrop, crop); > > > > (As before, but we have offsets dx and dy to enable panning, so we > > must also clamp the result to be sure it's within the sensor area.) > > > > 3. Zooming as before, but not allowing you to pan into anything that > > wasn't initially visible > > > > Size area = camera->properties().get(properties::SensorOutputSize); > > Rectangle maxCrop = area.aspectRatioDown(streamCfg.size).centre(area); > > Size size = maxCrop.size() / zoomFactor; > > Rectangle crop = size.centre(maxCrop, dx, dy).clamp(maxCrop); > > request->controls.set(controls::SensorCrop, crop); > > > > (The subtle difference here is that we clamp the result against maxCrop, > > not the full sensor area.) Thanks! very informative! > > > > 4. Android > > > > This time we're given the "crop" (a Rectangle) that Android has requested. I > > think Android only requires us to sort out the aspect ratio. > > > > Size size = crop.size().aspectRatioDown(streamCfg.size); > > request->controls.set(controls::SensorCrop, size.centre(crop)); We'll here have to deal with the fact Android specifies its sensor crop with the full size sensor pixel array as reference, not the actual sensor output frame. This shall be handled in the HAL though. > > > > > > That's as far as I've got. Thoughts and comments welcome! > > > > Best regards > > David > > > > David Plowman (4): > > libcamera: Add SensorOutputSize property > > libcamera: Add SensorCrop control > > libcamera: Add geometry helper functions > > libcamera: pipeline: raspberrypi: Implementation of digital zoom > > > > include/libcamera/geometry.h | 18 +++ > > include/libcamera/ipa/raspberrypi.h | 1 + > > src/ipa/raspberrypi/raspberrypi.cpp | 7 + > > src/libcamera/camera_sensor.cpp | 3 + > > src/libcamera/control_ids.yaml | 9 ++ > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++ > > .../pipeline/raspberrypi/raspberrypi.cpp | 41 ++++++ > > src/libcamera/property_ids.yaml | 6 + > > 8 files changed, 214 insertions(+) > > > > -- > > 2.20.1 > > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, just noticed that.... On Mon, Sep 21, 2020 at 12:41:38PM +0200, Jacopo Mondi wrote: > Hi David, > > On Mon, Sep 21, 2020 at 07:24:45AM +0100, David Plowman wrote: > > Hi again everyone > > > > I was wondering if anyone else had any thoughts on this. In spite of > > the long email which is perhaps a bit off-putting I don't think > > there's actually very much complicated stuff going on. Anyway, I'm > > keen to try and stop this discussion from getting marooned on the > > backlog for too long if we can! > > Sorry sorry for the late reply. We also left quite some discussion > pending on the previous version, let's try to catch up here. > > > > > Thanks again > > David > > > > On Mon, 7 Sep 2020 at 17:44, David Plowman > > <david.plowman@raspberrypi.com> wrote: > > > > > > Hi everyone > > > > > > I wanted to return to the question of digital zoom. Rather than simply > > > writing a long email I've included an implementation that shows where > > > I am. It's changed in the light of exchanges with Jacopo and others, > > > and I would say it's "for discussion" at this point. Principally: All patches miss your Signed-off-by, but reading this, it might be very well intentional. Thanks j
Hi Jacopo Thanks for the comments! On Mon, 21 Sep 2020 at 11:37, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Mon, Sep 21, 2020 at 07:24:45AM +0100, David Plowman wrote: > > Hi again everyone > > > > I was wondering if anyone else had any thoughts on this. In spite of > > the long email which is perhaps a bit off-putting I don't think > > there's actually very much complicated stuff going on. Anyway, I'm > > keen to try and stop this discussion from getting marooned on the > > backlog for too long if we can! > > Sorry sorry for the late reply. We also left quite some discussion > pending on the previous version, let's try to catch up here. > > > > > Thanks again > > David > > > > On Mon, 7 Sep 2020 at 17:44, David Plowman > > <david.plowman@raspberrypi.com> wrote: > > > > > > Hi everyone > > > > > > I wanted to return to the question of digital zoom. Rather than simply > > > writing a long email I've included an implementation that shows where > > > I am. It's changed in the light of exchanges with Jacopo and others, > > > and I would say it's "for discussion" at this point. Principally: > > > > > > 1. I'd like users to have access to CameraSensorInfo::outputSize, and > > > I'm using Camera::properties() to get it. Unfortunately it changes > > > with every camera mode, so will we have to do this differently? > > > > > I'll reply to this in 1/4. I think it's not optimal, but we should not > block this feature any longer. Although, I'm afraid that application > might be forced to change the way they retrieve the SensorCrop (from > Camera to CameraConfiguration). How fast do yuo expect this will be > adopted? Do you plan to build anything on top ? As you know, we're building libcamera versions of our existing raspicam apps, and digital zoom is a feature those applications have. But beyond that, I don't see it spreading very far just yet. In the meantime, anyway, I'm not keen to make those new apps public while they're still missing some key features, though I'd be happy to share them privately if anyone from the libcamera team was curious. I was also wondering about adding digital zoom to qcam as a stream option at startup, otherwise it's a bit hard to try without our as-yet unavailable libcamera apps. Would that be helpful? Thanks again for all the feedback. I'll get on and produce a proper set of patches in the next day or two (properly signed too!) Best regards David > > > > 2. The terminology is a bit more Android-like. Because, why not? > > > > > :) > > > > 3. I'm no longer enforcing any particular method of cropping. But to > > > make life easier for applications I've added some geometry helpers. To > > > understand the rationale behind these I've included some example code > > > further down that I think an application might use. > > Nice! > > > > > > > This work is currently made up of 4 commits: > > > > > > 1. The SensorOutputSize property. > > > > > > 2. The SensorCrop control. > > > > > > 3. The geometry helper functions. > > > > > > 4. Implementation in the Raspberry Pi pipeline handler. > > > > > Repluies in individual patches > > > > Example Application Code > > > > > > Assume we have: > > > > > > camera - pointer to the Camera object > > > streamCfg - reference to the StreamConfiguration of the output image > > > request - pointer to a Request object > > > zoomFactor - floating point zoom factor, >= 1 > > > dx - amount in pixels to pan left (-ve) or right (+ve) > > > dy - amount in pixels to pan up (-ve) or down (+ve) > > > > > > You'll want to understand the helper methods: > > > > > > Size Size::aspectRatioDown(const Size &ar) const > > > Returns a letter/pillarboxed version of this size. > > > > > > Size operator/(const Size &size, float f) > > > Downscale a Size object by f. > > > > > > Rectangle Size::centre(const Rectangle ®ion, int offsetX = 0, int offsetY = 0) const > > > Returns a Rectangle of this size centred at the same point as region, > > > optionally with offsets. > > > > > > Rectangle Rectangle::clamp(const Rectangle &boundary) const > > > Translates the rectangle so that no part of it lies outside boundary. > > > > > > 1. Basic zooming into the centre > > > > > > Size area = camera->properties().get(properties::SensorOutputSize); > > > Size size = area.aspectRatioDown(streamCfg.size) / zoomFactor; > > > Rectangle crop = size.centre(Rectangle(area)); > > > request->controls.set(controls::SensorCrop, crop); > > > > > > (I'm hoping this first one is easy enough to understand!) > > > > > > 2. Zooming anywhere in the sensor output (even into parts of the image that > > > were not intially visible) > > > > > > Size area = camera->properties().get(properties::SensorOutputSize); > > > Size size = area.aspectRatioDown(streamCfg.size) / zoomFactor; > > > Rectangle crop = size.centre(Rectangle(area), dx, dy).clamp(Rectangle(area)); > > > request->controls.set(controls::SensorCrop, crop); > > > > > > (As before, but we have offsets dx and dy to enable panning, so we > > > must also clamp the result to be sure it's within the sensor area.) > > > > > > 3. Zooming as before, but not allowing you to pan into anything that > > > wasn't initially visible > > > > > > Size area = camera->properties().get(properties::SensorOutputSize); > > > Rectangle maxCrop = area.aspectRatioDown(streamCfg.size).centre(area); > > > Size size = maxCrop.size() / zoomFactor; > > > Rectangle crop = size.centre(maxCrop, dx, dy).clamp(maxCrop); > > > request->controls.set(controls::SensorCrop, crop); > > > > > > (The subtle difference here is that we clamp the result against maxCrop, > > > not the full sensor area.) > > Thanks! very informative! > > > > > > > 4. Android > > > > > > This time we're given the "crop" (a Rectangle) that Android has requested. I > > > think Android only requires us to sort out the aspect ratio. > > > > > > Size size = crop.size().aspectRatioDown(streamCfg.size); > > > request->controls.set(controls::SensorCrop, size.centre(crop)); > > We'll here have to deal with the fact Android specifies its sensor > crop with the full size sensor pixel array as reference, not the actual > sensor output frame. This shall be handled in the HAL though. > > > > > > > > > > That's as far as I've got. Thoughts and comments welcome! > > > > > > Best regards > > > David > > > > > > David Plowman (4): > > > libcamera: Add SensorOutputSize property > > > libcamera: Add SensorCrop control > > > libcamera: Add geometry helper functions > > > libcamera: pipeline: raspberrypi: Implementation of digital zoom > > > > > > include/libcamera/geometry.h | 18 +++ > > > include/libcamera/ipa/raspberrypi.h | 1 + > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 + > > > src/libcamera/camera_sensor.cpp | 3 + > > > src/libcamera/control_ids.yaml | 9 ++ > > > src/libcamera/geometry.cpp | 129 ++++++++++++++++++ > > > .../pipeline/raspberrypi/raspberrypi.cpp | 41 ++++++ > > > src/libcamera/property_ids.yaml | 6 + > > > 8 files changed, 214 insertions(+) > > > > > > -- > > > 2.20.1 > > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel