| Message ID | 20200702213654.2129054-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for your work. On 2020-07-02 22:36:49 +0100, Kieran Bingham wrote: > Move the code which constructs a FrameBuffer from the Android buffer handle > to it's own function to simplify the code flow and readability. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/android/camera_device.cpp | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 03dcdd520794..78ecfd7c2ee2 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1011,6 +1011,24 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > return 0; > } > > +static FrameBuffer *newFrameBuffer(const buffer_handle_t camera3buffer) > +{ > + std::vector<FrameBuffer::Plane> planes; > + for (unsigned int i = 0; i < 3; i++) { > + FrameBuffer::Plane plane; > + plane.fd = FileDescriptor(camera3buffer->data[i]); > + /* > + * Setting length to zero here is OK as the length is only used > + * to map the memory of the plane. Libcamera do not need to poke > + * at the memory content queued by the HAL. > + */ > + plane.length = 0; > + planes.push_back(std::move(plane)); > + } > + > + return new FrameBuffer(std::move(planes)); > +} > + > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) > { > StreamConfiguration *streamConfiguration = &config_->at(0); > @@ -1064,22 +1082,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * Create a libcamera buffer using the dmabuf descriptors of the first > * and (currently) only supported request buffer. > */ > - const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer; > - > - std::vector<FrameBuffer::Plane> planes; > - for (int i = 0; i < 3; i++) { > - FrameBuffer::Plane plane; > - plane.fd = FileDescriptor(camera3Handle->data[i]); > - /* > - * Setting length to zero here is OK as the length is only used > - * to map the memory of the plane. Libcamera do not need to poke > - * at the memory content queued by the HAL. > - */ > - plane.length = 0; > - planes.push_back(std::move(plane)); > - } > - > - FrameBuffer *buffer = new FrameBuffer(std::move(planes)); > + FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer); Would it make sens to change the argument to a pointer instead of dereferencing it here? Also I think I would callt the function constructFrameBuffer() or createFrameBuffer(). > if (!buffer) { > LOG(HAL, Error) << "Failed to create buffer"; > delete descriptor; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Fri, Jul 03, 2020 at 01:25:59AM +0200, Niklas Söderlund wrote: > On 2020-07-02 22:36:49 +0100, Kieran Bingham wrote: > > Move the code which constructs a FrameBuffer from the Android buffer handle > > to it's own function to simplify the code flow and readability. s/it's/its/ > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/android/camera_device.cpp | 35 +++++++++++++++++++---------------- > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 03dcdd520794..78ecfd7c2ee2 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1011,6 +1011,24 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > return 0; > > } > > > > +static FrameBuffer *newFrameBuffer(const buffer_handle_t camera3buffer) > > +{ > > + std::vector<FrameBuffer::Plane> planes; > > + for (unsigned int i = 0; i < 3; i++) { > > + FrameBuffer::Plane plane; > > + plane.fd = FileDescriptor(camera3buffer->data[i]); > > + /* > > + * Setting length to zero here is OK as the length is only used > > + * to map the memory of the plane. Libcamera do not need to poke > > + * at the memory content queued by the HAL. > > + */ > > + plane.length = 0; > > + planes.push_back(std::move(plane)); > > + } > > + > > + return new FrameBuffer(std::move(planes)); > > +} > > + > > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) > > { > > StreamConfiguration *streamConfiguration = &config_->at(0); > > @@ -1064,22 +1082,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * Create a libcamera buffer using the dmabuf descriptors of the first > > * and (currently) only supported request buffer. > > */ > > - const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer; > > - > > - std::vector<FrameBuffer::Plane> planes; > > - for (int i = 0; i < 3; i++) { > > - FrameBuffer::Plane plane; > > - plane.fd = FileDescriptor(camera3Handle->data[i]); > > - /* > > - * Setting length to zero here is OK as the length is only used > > - * to map the memory of the plane. Libcamera do not need to poke > > - * at the memory content queued by the HAL. > > - */ > > - plane.length = 0; > > - planes.push_back(std::move(plane)); > > - } > > - > > - FrameBuffer *buffer = new FrameBuffer(std::move(planes)); > > + FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer); > > Would it make sens to change the argument to a pointer instead of > dereferencing it here? buffer_handle_t is already a pointer, the function would then need to do plane.fd = FileDescriptor((*camera3buffer)->data[i]); which isn't nice. I think it's best to dereference it once only here, in the case function needs to access more than one field. > Also I think I would callt the function > constructFrameBuffer() or createFrameBuffer(). I like createFrameBuffer() better, and I would make it a private member of CameraDevice. With these changes, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > if (!buffer) { > > LOG(HAL, Error) << "Failed to create buffer"; > > delete descriptor;
Hi Kieran, On Fri, Jul 03, 2020 at 03:29:03AM +0300, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Jul 03, 2020 at 01:25:59AM +0200, Niklas Söderlund wrote: > > On 2020-07-02 22:36:49 +0100, Kieran Bingham wrote: > > > Move the code which constructs a FrameBuffer from the Android buffer handle > > > to it's own function to simplify the code flow and readability. > > s/it's/its/ > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/android/camera_device.cpp | 35 +++++++++++++++++++---------------- > > > 1 file changed, 19 insertions(+), 16 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 03dcdd520794..78ecfd7c2ee2 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1011,6 +1011,24 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) > > > return 0; > > > } > > > > > > +static FrameBuffer *newFrameBuffer(const buffer_handle_t camera3buffer) > > > +{ > > > + std::vector<FrameBuffer::Plane> planes; > > > + for (unsigned int i = 0; i < 3; i++) { > > > + FrameBuffer::Plane plane; > > > + plane.fd = FileDescriptor(camera3buffer->data[i]); > > > + /* > > > + * Setting length to zero here is OK as the length is only used > > > + * to map the memory of the plane. Libcamera do not need to poke > > > + * at the memory content queued by the HAL. > > > + */ > > > + plane.length = 0; > > > + planes.push_back(std::move(plane)); > > > + } > > > + > > > + return new FrameBuffer(std::move(planes)); > > > +} > > > + > > > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) > > > { > > > StreamConfiguration *streamConfiguration = &config_->at(0); > > > @@ -1064,22 +1082,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > * Create a libcamera buffer using the dmabuf descriptors of the first > > > * and (currently) only supported request buffer. > > > */ > > > - const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer; > > > - > > > - std::vector<FrameBuffer::Plane> planes; > > > - for (int i = 0; i < 3; i++) { > > > - FrameBuffer::Plane plane; > > > - plane.fd = FileDescriptor(camera3Handle->data[i]); > > > - /* > > > - * Setting length to zero here is OK as the length is only used > > > - * to map the memory of the plane. Libcamera do not need to poke > > > - * at the memory content queued by the HAL. > > > - */ > > > - plane.length = 0; > > > - planes.push_back(std::move(plane)); > > > - } > > > - > > > - FrameBuffer *buffer = new FrameBuffer(std::move(planes)); > > > + FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer); > > > > Would it make sens to change the argument to a pointer instead of > > dereferencing it here? > > buffer_handle_t is already a pointer, the function would then need to do > > plane.fd = FileDescriptor((*camera3buffer)->data[i]); > > which isn't nice. I think it's best to dereference it once only here, in > the case function needs to access more than one field. > > > Also I think I would callt the function > > constructFrameBuffer() or createFrameBuffer(). > > I like createFrameBuffer() better, and I would make it a private member > of CameraDevice. With these changes, Me too, and I like this better as a private helper function > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > > if (!buffer) { > > > LOG(HAL, Error) << "Failed to create buffer"; > > > delete descriptor; > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi All, On 03/07/2020 10:13, Jacopo Mondi wrote: > Hi Kieran, > > On Fri, Jul 03, 2020 at 03:29:03AM +0300, Laurent Pinchart wrote: >> Hi Kieran, >> >> Thank you for the patch. >> >> On Fri, Jul 03, 2020 at 01:25:59AM +0200, Niklas Söderlund wrote: >>> On 2020-07-02 22:36:49 +0100, Kieran Bingham wrote: >>>> Move the code which constructs a FrameBuffer from the Android buffer handle >>>> to it's own function to simplify the code flow and readability. >> >> s/it's/its/ >> >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> src/android/camera_device.cpp | 35 +++++++++++++++++++---------------- >>>> 1 file changed, 19 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index 03dcdd520794..78ecfd7c2ee2 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -1011,6 +1011,24 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) >>>> return 0; >>>> } >>>> >>>> +static FrameBuffer *newFrameBuffer(const buffer_handle_t camera3buffer) >>>> +{ >>>> + std::vector<FrameBuffer::Plane> planes; >>>> + for (unsigned int i = 0; i < 3; i++) { >>>> + FrameBuffer::Plane plane; >>>> + plane.fd = FileDescriptor(camera3buffer->data[i]); >>>> + /* >>>> + * Setting length to zero here is OK as the length is only used >>>> + * to map the memory of the plane. Libcamera do not need to poke >>>> + * at the memory content queued by the HAL. >>>> + */ >>>> + plane.length = 0; >>>> + planes.push_back(std::move(plane)); >>>> + } >>>> + >>>> + return new FrameBuffer(std::move(planes)); >>>> +} >>>> + >>>> int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) >>>> { >>>> StreamConfiguration *streamConfiguration = &config_->at(0); >>>> @@ -1064,22 +1082,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>>> * Create a libcamera buffer using the dmabuf descriptors of the first >>>> * and (currently) only supported request buffer. >>>> */ >>>> - const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer; >>>> - >>>> - std::vector<FrameBuffer::Plane> planes; >>>> - for (int i = 0; i < 3; i++) { >>>> - FrameBuffer::Plane plane; >>>> - plane.fd = FileDescriptor(camera3Handle->data[i]); >>>> - /* >>>> - * Setting length to zero here is OK as the length is only used >>>> - * to map the memory of the plane. Libcamera do not need to poke >>>> - * at the memory content queued by the HAL. >>>> - */ >>>> - plane.length = 0; >>>> - planes.push_back(std::move(plane)); >>>> - } >>>> - >>>> - FrameBuffer *buffer = new FrameBuffer(std::move(planes)); >>>> + FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer); >>> >>> Would it make sens to change the argument to a pointer instead of >>> dereferencing it here? >> >> buffer_handle_t is already a pointer, the function would then need to do >> >> plane.fd = FileDescriptor((*camera3buffer)->data[i]); >> >> which isn't nice. I think it's best to dereference it once only here, in >> the case function needs to access more than one field. Yup, the ** was a pain, so I chose to dereference it here. >> >>> Also I think I would callt the function >>> constructFrameBuffer() or createFrameBuffer(). >> >> I like createFrameBuffer() better, and I would make it a private member I'll rename to createFrameBuffer() >> of CameraDevice. With these changes, > > Me too, and I like this better as a private helper function Agreed, me too - this was just a remnant of hacking it in to get the reworks done. >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks. > > Thanks > j >> >>>> if (!buffer) { >>>> LOG(HAL, Error) << "Failed to create buffer"; >>>> delete descriptor; >> >> -- >> Regards, >> >> Laurent Pinchart >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 03dcdd520794..78ecfd7c2ee2 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1011,6 +1011,24 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list) return 0; } +static FrameBuffer *newFrameBuffer(const buffer_handle_t camera3buffer) +{ + std::vector<FrameBuffer::Plane> planes; + for (unsigned int i = 0; i < 3; i++) { + FrameBuffer::Plane plane; + plane.fd = FileDescriptor(camera3buffer->data[i]); + /* + * Setting length to zero here is OK as the length is only used + * to map the memory of the plane. Libcamera do not need to poke + * at the memory content queued by the HAL. + */ + plane.length = 0; + planes.push_back(std::move(plane)); + } + + return new FrameBuffer(std::move(planes)); +} + int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request) { StreamConfiguration *streamConfiguration = &config_->at(0); @@ -1064,22 +1082,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * Create a libcamera buffer using the dmabuf descriptors of the first * and (currently) only supported request buffer. */ - const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer; - - std::vector<FrameBuffer::Plane> planes; - for (int i = 0; i < 3; i++) { - FrameBuffer::Plane plane; - plane.fd = FileDescriptor(camera3Handle->data[i]); - /* - * Setting length to zero here is OK as the length is only used - * to map the memory of the plane. Libcamera do not need to poke - * at the memory content queued by the HAL. - */ - plane.length = 0; - planes.push_back(std::move(plane)); - } - - FrameBuffer *buffer = new FrameBuffer(std::move(planes)); + FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer); if (!buffer) { LOG(HAL, Error) << "Failed to create buffer"; delete descriptor;
Move the code which constructs a FrameBuffer from the Android buffer handle to it's own function to simplify the code flow and readability. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/android/camera_device.cpp | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-)