| Message ID | 20200720224232.153717-9-kieran.bingham@ideasonboard.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote: > Utilise the MappedBuffer interface to map each of the planes provided > in the Camera3 buffer to facilitate use in software streams. > > The buffers will be automatically unmapped when the object goes out of > scope or is deleted. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > This patch shows an addition of a new MappedBuffer type constructed from > a camera3buffer. The base class deals with the move-constructors and > destructor, and gives us a common interface to pass a set of mapped > dmabufs around. > > I had hoped to use this to pass in the camera3buffer for writing jpeg > buffers to, giving the same interface for both the source and > destination buffer - but for JPEG, I do also need to return the number > of bytes actually consumed, so this ended up potentially not adding the > benefits I hoped for. > > Still, it might still provide some benefits, so I've included it here as > something to talk about. This, along with patch 4/8, is the only part of the series I'm not sure to like :-S It feels quite like a ad-hoc solution, which is probably normal, as that's what it is :-) It may not be that bad, but doesn't qualify for the public API in my opinion. We could keep this as-is for now as an internal helper, until we figure something better (if there's ever a need to do so). A random idea, would it make sense to convert buffer_handle_t to FrameBuffer as early as possible in the HAL and operate on FrameBuffer ? There's at least one drawback in that it would dup() the fds, which may call for a rework of the FileDescriptor class to make borrowing fds possible. Or maybe it's just a bad idea. I'd like to see how this ends up being used for the JPEG encoding to get a better feeling of the API. I thus can't guarantee at this point I'll ack the idea as-is, but I think it's worth continuing the experiment, I feel there's something good here. > src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 6212ccdd61ec..f78486117e9f 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > > LOG_DECLARE_CATEGORY(HAL); > > +class MappedCamera3Buffer : public MappedBuffer { > +public: > + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) > + { > + maps_.reserve(camera3buffer->numFds); > + error_ = 0; > + > + for (int i = 0; i < camera3buffer->numFds; i++) { > + if (camera3buffer->data[i] == -1) > + continue; > + > + off_t length = lseek(camera3buffer->data[i], 0, SEEK_END); > + if (length < 0) { > + error_ = errno; > + LOG(HAL, Error) << "Failed to query plane length"; > + break; > + } > + > + void *address = mmap(nullptr, length, flags, MAP_SHARED, > + camera3buffer->data[i], 0); > + if (address == MAP_FAILED) { > + error_ = errno; > + LOG(HAL, Error) << "Failed to mmap plane"; > + break; > + } > + > + maps_.push_back({address, static_cast<size_t>(length)}); > + } > + > + valid_ = error_ == 0; > + } > +}; > + > /* > * \struct Camera3RequestDescriptor > *
Hi Laurent, On 24/07/2020 18:24, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote: >> Utilise the MappedBuffer interface to map each of the planes provided >> in the Camera3 buffer to facilitate use in software streams. >> >> The buffers will be automatically unmapped when the object goes out of >> scope or is deleted. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> >> This patch shows an addition of a new MappedBuffer type constructed from >> a camera3buffer. The base class deals with the move-constructors and >> destructor, and gives us a common interface to pass a set of mapped >> dmabufs around. >> >> I had hoped to use this to pass in the camera3buffer for writing jpeg >> buffers to, giving the same interface for both the source and >> destination buffer - but for JPEG, I do also need to return the number >> of bytes actually consumed, so this ended up potentially not adding the >> benefits I hoped for. >> >> Still, it might still provide some benefits, so I've included it here as >> something to talk about. > > This, along with patch 4/8, is the only part of the series I'm not sure > to like :-S It feels quite like a ad-hoc solution, which is probably > normal, as that's what it is :-) It may not be that bad, but doesn't > qualify for the public API in my opinion. We could keep this as-is for > now as an internal helper, until we figure something better (if there's > ever a need to do so). This *isn't* public api - this is under src/android/camera_device. Perhaps you mean the MappedBuffer API shouldn't be public (Though even if the base is still internal, the Android layer can still use this). > A random idea, would it make sense to convert buffer_handle_t to > FrameBuffer as early as possible in the HAL and operate on FrameBuffer ? > There's at least one drawback in that it would dup() the fds, which may > call for a rework of the FileDescriptor class to make borrowing fds > possible. Or maybe it's just a bad idea. I thnik it was the unnecessary dup's required to construct a FrameBuffer that stopped me, but perhaps it might make some sense still. > I'd like to see how this ends up being used for the JPEG encoding to get > a better feeling of the API. I thus can't guarantee at this point I'll > ack the idea as-is, but I think it's worth continuing the experiment, I > feel there's something good here. The idea was to make sure that the Mapped buffer object was the same regardless of whether it came from a FrameBuffer, or from a buffer_handle_t. Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is a solution to that, it just wastes/duplicates more fds. See: [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer (Ugh ,sorry same patch name, this was split from there) for how this got used to remove the aribtrary mapAndroidBlobBuffer() and unmapAndroidBlobBuffer() fucntions that I had created earlier on. (Kept separate for exactly these discussions). I think for now, I'm going to end up mapping a FrameBuffer from every buffer_handle_t() for consistency instead. -- Kieran >> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 6212ccdd61ec..f78486117e9f 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { >> >> LOG_DECLARE_CATEGORY(HAL); >> >> +class MappedCamera3Buffer : public MappedBuffer { >> +public: >> + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) >> + { >> + maps_.reserve(camera3buffer->numFds); >> + error_ = 0; >> + >> + for (int i = 0; i < camera3buffer->numFds; i++) { >> + if (camera3buffer->data[i] == -1) >> + continue; >> + >> + off_t length = lseek(camera3buffer->data[i], 0, SEEK_END); >> + if (length < 0) { >> + error_ = errno; >> + LOG(HAL, Error) << "Failed to query plane length"; >> + break; >> + } >> + >> + void *address = mmap(nullptr, length, flags, MAP_SHARED, >> + camera3buffer->data[i], 0); >> + if (address == MAP_FAILED) { >> + error_ = errno; >> + LOG(HAL, Error) << "Failed to mmap plane"; >> + break; >> + } >> + >> + maps_.push_back({address, static_cast<size_t>(length)}); >> + } >> + >> + valid_ = error_ == 0; >> + } >> +}; >> + >> /* >> * \struct Camera3RequestDescriptor >> * >
Hi Kieran, On Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote: > On 24/07/2020 18:24, Laurent Pinchart wrote: > > On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote: > >> Utilise the MappedBuffer interface to map each of the planes provided > >> in the Camera3 buffer to facilitate use in software streams. > >> > >> The buffers will be automatically unmapped when the object goes out of > >> scope or is deleted. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> > >> This patch shows an addition of a new MappedBuffer type constructed from > >> a camera3buffer. The base class deals with the move-constructors and > >> destructor, and gives us a common interface to pass a set of mapped > >> dmabufs around. > >> > >> I had hoped to use this to pass in the camera3buffer for writing jpeg > >> buffers to, giving the same interface for both the source and > >> destination buffer - but for JPEG, I do also need to return the number > >> of bytes actually consumed, so this ended up potentially not adding the > >> benefits I hoped for. > >> > >> Still, it might still provide some benefits, so I've included it here as > >> something to talk about. > > > > This, along with patch 4/8, is the only part of the series I'm not sure > > to like :-S It feels quite like a ad-hoc solution, which is probably > > normal, as that's what it is :-) It may not be that bad, but doesn't > > qualify for the public API in my opinion. We could keep this as-is for > > now as an internal helper, until we figure something better (if there's > > ever a need to do so). > > This *isn't* public api - this is under src/android/camera_device. > > Perhaps you mean the MappedBuffer API shouldn't be public (Though even > if the base is still internal, the Android layer can still use this). I may also have been confused. I think I was referring to the idea of creating a base class that can be inherited from, that part is in the public API. Making it private should be fine, but exposing that design externally would I think require a bit more work first. > > A random idea, would it make sense to convert buffer_handle_t to > > FrameBuffer as early as possible in the HAL and operate on FrameBuffer ? > > There's at least one drawback in that it would dup() the fds, which may > > call for a rework of the FileDescriptor class to make borrowing fds > > possible. Or maybe it's just a bad idea. > > I thnik it was the unnecessary dup's required to construct a FrameBuffer > that stopped me, but perhaps it might make some sense still. We really want to avoid unnecessary dup() calls as much as possible, as that's expensive. Do you think it would make sense to extend FileDescriptor to allow borrowing fds, or is that too complicated, or too hackish ? > > I'd like to see how this ends up being used for the JPEG encoding to get > > a better feeling of the API. I thus can't guarantee at this point I'll > > ack the idea as-is, but I think it's worth continuing the experiment, I > > feel there's something good here. > > The idea was to make sure that the Mapped buffer object was the same > regardless of whether it came from a FrameBuffer, or from a buffer_handle_t. > > Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is > a solution to that, it just wastes/duplicates more fds. > > See: > > [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer > > (Ugh ,sorry same patch name, this was split from there) > > for how this got used to remove the aribtrary mapAndroidBlobBuffer() and > unmapAndroidBlobBuffer() fucntions that I had created earlier on. > > (Kept separate for exactly these discussions). > > I think for now, I'm going to end up mapping a FrameBuffer from every > buffer_handle_t() for consistency instead. > > >> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++ > >> 1 file changed, 33 insertions(+) > >> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >> index 6212ccdd61ec..f78486117e9f 100644 > >> --- a/src/android/camera_device.cpp > >> +++ b/src/android/camera_device.cpp > >> @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > >> > >> LOG_DECLARE_CATEGORY(HAL); > >> > >> +class MappedCamera3Buffer : public MappedBuffer { > >> +public: > >> + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) > >> + { > >> + maps_.reserve(camera3buffer->numFds); > >> + error_ = 0; > >> + > >> + for (int i = 0; i < camera3buffer->numFds; i++) { > >> + if (camera3buffer->data[i] == -1) > >> + continue; > >> + > >> + off_t length = lseek(camera3buffer->data[i], 0, SEEK_END); > >> + if (length < 0) { > >> + error_ = errno; > >> + LOG(HAL, Error) << "Failed to query plane length"; > >> + break; > >> + } > >> + > >> + void *address = mmap(nullptr, length, flags, MAP_SHARED, > >> + camera3buffer->data[i], 0); > >> + if (address == MAP_FAILED) { > >> + error_ = errno; > >> + LOG(HAL, Error) << "Failed to mmap plane"; > >> + break; > >> + } > >> + > >> + maps_.push_back({address, static_cast<size_t>(length)}); > >> + } > >> + > >> + valid_ = error_ == 0; > >> + } > >> +}; > >> + > >> /* > >> * \struct Camera3RequestDescriptor > >> *
Hi Laurent, On 29/07/2020 15:27, Laurent Pinchart wrote: > Hi Kieran, > > On Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote: >> On 24/07/2020 18:24, Laurent Pinchart wrote: >>> On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote: >>>> Utilise the MappedBuffer interface to map each of the planes provided >>>> in the Camera3 buffer to facilitate use in software streams. >>>> >>>> The buffers will be automatically unmapped when the object goes out of >>>> scope or is deleted. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> >>>> This patch shows an addition of a new MappedBuffer type constructed from >>>> a camera3buffer. The base class deals with the move-constructors and >>>> destructor, and gives us a common interface to pass a set of mapped >>>> dmabufs around. >>>> >>>> I had hoped to use this to pass in the camera3buffer for writing jpeg >>>> buffers to, giving the same interface for both the source and >>>> destination buffer - but for JPEG, I do also need to return the number >>>> of bytes actually consumed, so this ended up potentially not adding the >>>> benefits I hoped for. >>>> >>>> Still, it might still provide some benefits, so I've included it here as >>>> something to talk about. >>> >>> This, along with patch 4/8, is the only part of the series I'm not sure >>> to like :-S It feels quite like a ad-hoc solution, which is probably >>> normal, as that's what it is :-) It may not be that bad, but doesn't >>> qualify for the public API in my opinion. We could keep this as-is for >>> now as an internal helper, until we figure something better (if there's >>> ever a need to do so). >> >> This *isn't* public api - this is under src/android/camera_device. >> >> Perhaps you mean the MappedBuffer API shouldn't be public (Though even >> if the base is still internal, the Android layer can still use this). > > I may also have been confused. I think I was referring to the idea of > creating a base class that can be inherited from, that part is in the > public API. Making it private should be fine, but exposing that design > externally would I think require a bit more work first. > >>> A random idea, would it make sense to convert buffer_handle_t to >>> FrameBuffer as early as possible in the HAL and operate on FrameBuffer ? >>> There's at least one drawback in that it would dup() the fds, which may >>> call for a rework of the FileDescriptor class to make borrowing fds >>> possible. Or maybe it's just a bad idea. >> >> I thnik it was the unnecessary dup's required to construct a FrameBuffer >> that stopped me, but perhaps it might make some sense still. > > We really want to avoid unnecessary dup() calls as much as possible, as > that's expensive. Do you think it would make sense to extend > FileDescriptor to allow borrowing fds, or is that too complicated, or > too hackish ? I don't think it's too hackish necessarily, but I don't know how that could be expressed. And indeed it could make things complicated for lifetimes as suddenly what happens if the other owner closes them etc.. (ok so you could document the requirements for borrowing etc)... I presume we'd have to have some sort of a flag to determine the fd was borrowed and prevent the associated closes on destructor ... it feels a bit awkward ... :S But I have two paths for 'now': Make a MappedBuffer generic base that can map from a buffer_t or Map all buffer_t's to a FrameBuffer, then make a MappedFrameBuffer from that ... Do you have a particular preference on either? If I construct a FrameBuffer for each buffer_t, we can always look to improve performance later... -- Kieran >>> I'd like to see how this ends up being used for the JPEG encoding to get >>> a better feeling of the API. I thus can't guarantee at this point I'll >>> ack the idea as-is, but I think it's worth continuing the experiment, I >>> feel there's something good here. >> >> The idea was to make sure that the Mapped buffer object was the same >> regardless of whether it came from a FrameBuffer, or from a buffer_handle_t. >> >> Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is >> a solution to that, it just wastes/duplicates more fds. >> >> See: >> >> [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer >> >> (Ugh ,sorry same patch name, this was split from there) >> >> for how this got used to remove the aribtrary mapAndroidBlobBuffer() and >> unmapAndroidBlobBuffer() fucntions that I had created earlier on. >> >> (Kept separate for exactly these discussions). >> >> I think for now, I'm going to end up mapping a FrameBuffer from every >> buffer_handle_t() for consistency instead. >> >>>> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++ >>>> 1 file changed, 33 insertions(+) >>>> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index 6212ccdd61ec..f78486117e9f 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { >>>> >>>> LOG_DECLARE_CATEGORY(HAL); >>>> >>>> +class MappedCamera3Buffer : public MappedBuffer { >>>> +public: >>>> + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) >>>> + { >>>> + maps_.reserve(camera3buffer->numFds); >>>> + error_ = 0; >>>> + >>>> + for (int i = 0; i < camera3buffer->numFds; i++) { >>>> + if (camera3buffer->data[i] == -1) >>>> + continue; >>>> + >>>> + off_t length = lseek(camera3buffer->data[i], 0, SEEK_END); >>>> + if (length < 0) { >>>> + error_ = errno; >>>> + LOG(HAL, Error) << "Failed to query plane length"; >>>> + break; >>>> + } >>>> + >>>> + void *address = mmap(nullptr, length, flags, MAP_SHARED, >>>> + camera3buffer->data[i], 0); >>>> + if (address == MAP_FAILED) { >>>> + error_ = errno; >>>> + LOG(HAL, Error) << "Failed to mmap plane"; >>>> + break; >>>> + } >>>> + >>>> + maps_.push_back({address, static_cast<size_t>(length)}); >>>> + } >>>> + >>>> + valid_ = error_ == 0; >>>> + } >>>> +}; >>>> + >>>> /* >>>> * \struct Camera3RequestDescriptor >>>> * >
Hi Kieran, On Wed, Jul 29, 2020 at 03:37:16PM +0100, Kieran Bingham wrote: > On 29/07/2020 15:27, Laurent Pinchart wrote: > > On Wed, Jul 29, 2020 at 03:19:24PM +0100, Kieran Bingham wrote: > >> On 24/07/2020 18:24, Laurent Pinchart wrote: > >>> On Mon, Jul 20, 2020 at 11:42:32PM +0100, Kieran Bingham wrote: > >>>> Utilise the MappedBuffer interface to map each of the planes provided > >>>> in the Camera3 buffer to facilitate use in software streams. > >>>> > >>>> The buffers will be automatically unmapped when the object goes out of > >>>> scope or is deleted. > >>>> > >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>> --- > >>>> > >>>> This patch shows an addition of a new MappedBuffer type constructed from > >>>> a camera3buffer. The base class deals with the move-constructors and > >>>> destructor, and gives us a common interface to pass a set of mapped > >>>> dmabufs around. > >>>> > >>>> I had hoped to use this to pass in the camera3buffer for writing jpeg > >>>> buffers to, giving the same interface for both the source and > >>>> destination buffer - but for JPEG, I do also need to return the number > >>>> of bytes actually consumed, so this ended up potentially not adding the > >>>> benefits I hoped for. > >>>> > >>>> Still, it might still provide some benefits, so I've included it here as > >>>> something to talk about. > >>> > >>> This, along with patch 4/8, is the only part of the series I'm not sure > >>> to like :-S It feels quite like a ad-hoc solution, which is probably > >>> normal, as that's what it is :-) It may not be that bad, but doesn't > >>> qualify for the public API in my opinion. We could keep this as-is for > >>> now as an internal helper, until we figure something better (if there's > >>> ever a need to do so). > >> > >> This *isn't* public api - this is under src/android/camera_device. > >> > >> Perhaps you mean the MappedBuffer API shouldn't be public (Though even > >> if the base is still internal, the Android layer can still use this). > > > > I may also have been confused. I think I was referring to the idea of > > creating a base class that can be inherited from, that part is in the > > public API. Making it private should be fine, but exposing that design > > externally would I think require a bit more work first. > > > >>> A random idea, would it make sense to convert buffer_handle_t to > >>> FrameBuffer as early as possible in the HAL and operate on FrameBuffer ? > >>> There's at least one drawback in that it would dup() the fds, which may > >>> call for a rework of the FileDescriptor class to make borrowing fds > >>> possible. Or maybe it's just a bad idea. > >> > >> I thnik it was the unnecessary dup's required to construct a FrameBuffer > >> that stopped me, but perhaps it might make some sense still. > > > > We really want to avoid unnecessary dup() calls as much as possible, as > > that's expensive. Do you think it would make sense to extend > > FileDescriptor to allow borrowing fds, or is that too complicated, or > > too hackish ? > > I don't think it's too hackish necessarily, but I don't know how that > could be expressed. > > And indeed it could make things complicated for lifetimes as suddenly > what happens if the other owner closes them etc.. (ok so you could > document the requirements for borrowing etc)... > > I presume we'd have to have some sort of a flag to determine the fd was > borrowed and prevent the associated closes on destructor ... it feels a > bit awkward ... :S Yes, that's what would be needed, and it would be a bit awkward indeed. > But I have two paths for 'now': > > Make a MappedBuffer generic base that can map from a buffer_t > > or > > Map all buffer_t's to a FrameBuffer, then make a MappedFrameBuffer from > that ... > > Do you have a particular preference on either? > > If I construct a FrameBuffer for each buffer_t, we can always look to > improve performance later... As we agreed it will be an internal API for now, I would avoid dup()s already, at the expense of a future refactoring of the API. > >>> I'd like to see how this ends up being used for the JPEG encoding to get > >>> a better feeling of the API. I thus can't guarantee at this point I'll > >>> ack the idea as-is, but I think it's worth continuing the experiment, I > >>> feel there's something good here. > >> > >> The idea was to make sure that the Mapped buffer object was the same > >> regardless of whether it came from a FrameBuffer, or from a buffer_handle_t. > >> > >> Of course mapping the MappedFrameBuffer(FrameBuffer(buffer_handle_t)) is > >> a solution to that, it just wastes/duplicates more fds. > >> > >> See: > >> > >> [RFC PATCH 6/6] android: camera_device: Provide a MappedCamera3Buffer > >> > >> (Ugh ,sorry same patch name, this was split from there) > >> > >> for how this got used to remove the aribtrary mapAndroidBlobBuffer() and > >> unmapAndroidBlobBuffer() fucntions that I had created earlier on. > >> > >> (Kept separate for exactly these discussions). > >> > >> I think for now, I'm going to end up mapping a FrameBuffer from every > >> buffer_handle_t() for consistency instead. > >> > >>>> src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 33 insertions(+) > >>>> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > >>>> index 6212ccdd61ec..f78486117e9f 100644 > >>>> --- a/src/android/camera_device.cpp > >>>> +++ b/src/android/camera_device.cpp > >>>> @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { > >>>> > >>>> LOG_DECLARE_CATEGORY(HAL); > >>>> > >>>> +class MappedCamera3Buffer : public MappedBuffer { > >>>> +public: > >>>> + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) > >>>> + { > >>>> + maps_.reserve(camera3buffer->numFds); > >>>> + error_ = 0; > >>>> + > >>>> + for (int i = 0; i < camera3buffer->numFds; i++) { > >>>> + if (camera3buffer->data[i] == -1) > >>>> + continue; > >>>> + > >>>> + off_t length = lseek(camera3buffer->data[i], 0, SEEK_END); > >>>> + if (length < 0) { > >>>> + error_ = errno; > >>>> + LOG(HAL, Error) << "Failed to query plane length"; > >>>> + break; > >>>> + } > >>>> + > >>>> + void *address = mmap(nullptr, length, flags, MAP_SHARED, > >>>> + camera3buffer->data[i], 0); > >>>> + if (address == MAP_FAILED) { > >>>> + error_ = errno; > >>>> + LOG(HAL, Error) << "Failed to mmap plane"; > >>>> + break; > >>>> + } > >>>> + > >>>> + maps_.push_back({address, static_cast<size_t>(length)}); > >>>> + } > >>>> + > >>>> + valid_ = error_ == 0; > >>>> + } > >>>> +}; > >>>> + > >>>> /* > >>>> * \struct Camera3RequestDescriptor > >>>> *
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 6212ccdd61ec..f78486117e9f 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -86,6 +86,39 @@ const std::map<int, const Camera3Format> camera3FormatsMap = { LOG_DECLARE_CATEGORY(HAL); +class MappedCamera3Buffer : public MappedBuffer { +public: + MappedCamera3Buffer(const buffer_handle_t camera3buffer, int flags) + { + maps_.reserve(camera3buffer->numFds); + error_ = 0; + + for (int i = 0; i < camera3buffer->numFds; i++) { + if (camera3buffer->data[i] == -1) + continue; + + off_t length = lseek(camera3buffer->data[i], 0, SEEK_END); + if (length < 0) { + error_ = errno; + LOG(HAL, Error) << "Failed to query plane length"; + break; + } + + void *address = mmap(nullptr, length, flags, MAP_SHARED, + camera3buffer->data[i], 0); + if (address == MAP_FAILED) { + error_ = errno; + LOG(HAL, Error) << "Failed to mmap plane"; + break; + } + + maps_.push_back({address, static_cast<size_t>(length)}); + } + + valid_ = error_ == 0; + } +}; + /* * \struct Camera3RequestDescriptor *
Utilise the MappedBuffer interface to map each of the planes provided in the Camera3 buffer to facilitate use in software streams. The buffers will be automatically unmapped when the object goes out of scope or is deleted. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- This patch shows an addition of a new MappedBuffer type constructed from a camera3buffer. The base class deals with the move-constructors and destructor, and gives us a common interface to pass a set of mapped dmabufs around. I had hoped to use this to pass in the camera3buffer for writing jpeg buffers to, giving the same interface for both the source and destination buffer - but for JPEG, I do also need to return the number of bytes actually consumed, so this ended up potentially not adding the benefits I hoped for. Still, it might still provide some benefits, so I've included it here as something to talk about. -- Kieran src/android/camera_device.cpp | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)