<div dir="ltr"><div dir="ltr">I am starting to review these patches today.<div>This change LGTM.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Sep 5, 2020 at 1:11 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi me<br>
<br>
On Wed, Sep 02, 2020 at 05:22:36PM +0200, Jacopo Mondi wrote:<br>
> Complete the transformation of CameraStream into a class and provide<br>
> a read-only interface that allows to access its parameters but not<br>
> modify them at run-time.<br>
><br>
> No functional changes intended but this change aims to make the code<br>
> more robust by enforcing a stricter interface in the CameraStream class.<br>
><br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ---<br>
>  src/android/camera_device.cpp |  9 +++++----<br>
>  src/android/camera_device.h   | 14 ++++++++------<br>
>  2 files changed, 13 insertions(+), 10 deletions(-)<br>
><br>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp<br>
> index 28d8e081eab0..88c85ad183d2 100644<br>
> --- a/src/android/camera_device.cpp<br>
> +++ b/src/android/camera_device.cpp<br>
> @@ -168,8 +168,9 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,<br>
>       }<br>
>  }<br>
><br>
> -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)<br>
> -     : format(f), size(s), index_(i), encoder_(encoder)<br>
> +CameraStream::CameraStream(PixelFormat format, Size size,<br>
> +                        unsigned int index, Encoder *encoder)<br>
> +     : format_(format), size_(size), index_(index), encoder_(encoder)<br>
>  {<br>
>  }<br>
><br>
> @@ -1415,7 +1416,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques<br>
>               descriptor->buffers[i].buffer = camera3Buffers[i].buffer;<br>
><br>
>               /* Software streams are handled after hardware streams complete. */<br>
> -             if (cameraStream->format == formats::MJPEG)<br>
> +             if (cameraStream->format() == formats::MJPEG)<br>
>                       continue;<br>
><br>
>               /*<br>
> @@ -1479,7 +1480,7 @@ void CameraDevice::requestComplete(Request *request)<br>
>               CameraStream *cameraStream =<br>
>                       static_cast<CameraStream *>(descriptor->buffers[i].stream->priv);<br>
><br>
> -             if (cameraStream->format != formats::MJPEG)<br>
> +             if (cameraStream->format() != formats::MJPEG)<br>
>                       continue;<br>
><br>
>               Encoder *encoder = cameraStream->encoder();<br>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h<br>
> index 3c57ffec265d..729b2556011b 100644<br>
> --- a/src/android/camera_device.h<br>
> +++ b/src/android/camera_device.h<br>
> @@ -27,19 +27,21 @@<br>
><br>
>  class CameraMetadata;<br>
><br>
> -struct CameraStream {<br>
> +class CameraStream<br>
> +{<br>
>  public:<br>
> -     CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,<br>
> -                  Encoder *encoder = nullptr);<br>
> +     CameraStream(libcamera::PixelFormat format, libcamera::Size size,<br>
> +                  unsigned int index, Encoder *encoder = nullptr);<br>
>       ~CameraStream();<br>
><br>
> +     libcamera::PixelFormat format() const { return format_; }<br>
> +     libcamera::Size size() const { return size_; }<br>
<br>
Should these be const & ?<br>
<br></blockquote><div><br></div><div>Yes, it should be. Returning const reference reduces a copy.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>       unsigned int index() const { return index_; }<br>
>       Encoder *encoder() const { return encoder_; }<br>
><br>
> -     libcamera::PixelFormat format;<br>
> -     libcamera::Size size;<br>
> -<br>
>  private:<br>
> +     libcamera::PixelFormat format_;<br>
> +     libcamera::Size size_;<br>
>       /*<br>
>        * The index of the libcamera StreamConfiguration as added during<br>
>        * configureStreams(). A single libcamera Stream may be used to deliver<br>
> --<br>
> 2.28.0<br>
><br>
</blockquote></div></div>