[libcamera-devel] [RFC PATCH] android: camera_device: Reorder configurations given by a HAL client
Hirokazu Honda
hiroh at chromium.org
Thu Nov 5 12:29:08 CET 2020
Hi Jacopo,
Thanks for replying.
On Wed, Nov 4, 2020 at 11:48 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
> sorry for the delay.
>
> Not a really detailed review but more generic questions on the design
>
> On Wed, Oct 28, 2020 at 07:01:34PM +0900, Hirokazu Honda wrote:
> > Reorder configurations given by a HAL client in configureStreams()
> > so that CameraDevice and pipeline handlers can accept them as much
> > as possible.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-
> > src/android/camera_device.h | 3 +-
> > 2 files changed, 109 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ca60f51..eee7248 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -9,6 +9,7 @@
> > #include "camera_ops.h"
> > #include "post_processor.h"
> >
> > +#include <string.h>
> > #include <sys/mman.h>
> > #include <tuple>
> > #include <vector>
> > @@ -132,6 +133,16 @@ const std::map<int, const Camera3Format> camera3FormatsMap = {
> >
> > LOG_DECLARE_CATEGORY(HAL)
> >
> > +void printStreamList(const camera3_stream_configuration_t& streamList) {
> > + LOG(HAL, Error) << "num_streams=" << streamList.num_streams;
> > + for (unsigned int i = 0; i < streamList.num_streams; i++) {
> > + const auto* stream = streamList.streams[i];
> > + LOG(HAL, Error) << i << ": width=" << stream->width
> > + << ", height=" << stream->height
> > + << ", format=" << stream->format;
> > + }
> > +}
> > +
> > MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > int flags)
> > {
> > @@ -1188,7 +1199,7 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> > return requestTemplate->get();
> > }
> >
> > -PixelFormat CameraDevice::toPixelFormat(int format)
> > +PixelFormat CameraDevice::toPixelFormat(int format) const
> > {
> > /* Translate Android format code to libcamera pixel format. */
> > auto it = formatsMap_.find(format);
> > @@ -1201,6 +1212,97 @@ PixelFormat CameraDevice::toPixelFormat(int format)
> > return it->second;
> > }
> >
> > +int CameraDevice::sortStreamConfig(camera3_stream_configuration_t* streamList) const
>
> Is the intended sorting order of the Android streams the following one?
>
> { largest NV12, JPEG, NV12 same as JPEG, other streams.. }
>
> It see you are sorting the stream requested by Android.
> I might see the reason to do so, as StreamConfiguration are added to
> the CameraConfiguration while we walk the Android streams, so
> presenting the android streams ordered has the effect of ordering the
> StreamConfiguration consequentally. Is this the intent ?
>
Yes, I don't add any new config in sortsortStreamConfig(), because I
must not change the given config and the "backup" config for jpeg is
added later as you said.
> In example:
>
> { NV12-max, JPEG-vga, NV12-vga, .. }
>
> works because the CameraDevice::configureStreams() function will associate
> JPEG-vga with NV12-vga and the resulting StreamConfiguration will be
>
> { NV12-max, NV12-vga, .. }
>
> If there's no NV12-vga to generate JPEG-vga from, if I'm not mistaken,
> the result will be
>
> { NV12-max, ... , NV12-vga }
>
> as configureStreams inspects the JPEG streams after all the other
> ones, and eventually adds an NV12 stream to generate JPEG streams
> from.
>
> Apart from this, what concerns me a bit is that this ordering is meant
> to satisfy the configureStreams() implementation, and if one changes,
> the one has to change as well, as they are interdependent.
>
> If the final goal is to be able to sort the StreamConfiguration before
> presenting them to the libcamera::Camera I wonder if it wouldn't be
> better to rework configureStream() to:
> 1) Store StreamConfiguration in an std::vector<>
> 2) Order the vector of StreamConfiguration with the same logic you
> have here
> 3) Add the sorted StreamConfiguration to the CameraConfiguration
>
> This implies the CameraStream class will have to change, as the index
> of the associated StreamConfiguration will have to be injected later,
> after the StreamConfiguration is CameraConfiguration::add() to the
> CameraConfiguration. It's quite a substantial change, but I feel like
> it would be more robust than sorting the android stream to please code
> that we're in control of.
>
> What do you think ?
>
I think this also works.
I am going to try and submit your idea.
Let's see how the code looks like in the next version patch.
Thanks so much,
-Hiro
>
> > +{
> > + std::map<uint32_t, std::vector<const camera3_stream_t*>> streamMap;
> > + for (unsigned int i = 0; i < streamList->num_streams; ++i) {
> > + const camera3_stream_t* stream = streamList->streams[i];
> > + PixelFormat format = toPixelFormat(stream->format);
> > + if (!format.isValid())
> > + return -EINVAL;
> > + streamMap[format.fourcc()].push_back(stream);
> > + }
> > + std::vector<uint32_t> formats;
> > + for (auto& s: streamMap) {
> > + formats.push_back(s.first);
> > + auto& streams = s.second;
> > + /* Sorted by resolution. Smaller is put first. */
> > + std::sort(streams.begin(), streams.end(),
> > + [](const camera3_stream_t* streamA,
> > + const camera3_stream_t* streamB) {
> > + if (streamA->width != streamB->width)
> > + return streamA->width < streamB->width;
> > + return streamA->height < streamB->height;
> > + });
> > + }
> > +
> > + if (streamMap.find(formats::MJPEG) != streamMap.end() &&
> > + streamMap[formats::MJPEG].size() > 1) {
> > + LOG(HAL, Error) << "Multiple JPEG streams are not supported";
> > + return -EINVAL;
> > + }
> > +
> > + std::vector<const camera3_stream_t*> sortedStreams;
> > + /*
> > + * NV12 is the most prioritized format. Put the configuration with NV12
> > + * and the largest resolution first.
> > + */
> > + auto nv12StreamsIt = streamMap.find(formats::NV12);
> > + if (nv12StreamsIt != streamMap.end()) {
> > + auto& nv12Streams = nv12StreamsIt->second;
> > + const camera3_stream_t* nv12Stream = nv12Streams.back();
> > + sortedStreams.push_back(nv12Stream);
> > + nv12Streams.pop_back();
> > + }
> > +
> > + /*
> > + * If JPEG is there, then put the JPEG configuration second and a
> > + * configuration whose specified stream the JPEG is encoded from third
> > + * if there is.
> > + */
> > + auto jpegStreamsIt = streamMap.find(formats::MJPEG);
> > + if (jpegStreamsIt != streamMap.end()) {
> > + auto& jpegStreams = jpegStreamsIt->second;
> > + const camera3_stream_t* jpegStream = jpegStreams[0];
> > + sortedStreams.push_back(jpegStream);
> > + streamMap.erase(jpegStreamsIt);
> > + if (nv12StreamsIt != streamMap.end()) {
> > + auto& nv12Streams = nv12StreamsIt->second;
> > + auto nv12StreamForJpegIt =
> > + std::find_if(nv12Streams.begin(),
> > + nv12Streams.end(),
> > + [&jpegStream](const auto* nv12Stream) {
> > + return nv12Stream->width == jpegStream->width && nv12Stream->height == jpegStream->height;
> > + });
> > + if (nv12StreamForJpegIt != nv12Streams.end()) {
> > + sortedStreams.push_back(*nv12StreamForJpegIt);
> > + nv12Streams.erase(nv12StreamForJpegIt);
> > + }
> > + }
> > + }
> > +
> > + /*
> > + * Put configurations with different formats and larger resolutions
> > + * earlier.
> > + */
> > + while (!streamMap.empty()) {
> > + for (auto it = streamMap.begin(); it != streamMap.end();) {
> > + auto& streams = it->second;
> > + if (streams.empty()) {
> > + it = streamMap.erase(it);
> > + continue;
> > + }
> > + sortedStreams.push_back(streams.back());
> > + streams.pop_back();
> > + }
> > + }
> > +
> > + assert(sortedStreams.size() == streamList->num_streams);
> > + memcpy(streamList->streams, sortedStreams.data(),
> > + sizeof(camera3_stream_t*) * streamList->num_streams);
> > + return 0;
> > +}
> > +
> > /*
> > * Inspect the stream_list to produce a list of StreamConfiguration to
> > * be use to configure the Camera.
> > @@ -1225,6 +1327,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > streams_.clear();
> > streams_.reserve(stream_list->num_streams);
> >
> > + printStreamList(*stream_list);
> > + sortStreamConfig(stream_list);
> > + printStreamList(*stream_list);
> > +
> > /* First handle all non-MJPEG streams. */
> > camera3_stream_t *jpegStream = nullptr;
> > for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index fd08738..f8dc28b 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -104,7 +104,8 @@ private:
> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> > CameraMetadata *requestTemplatePreview();
> > - libcamera::PixelFormat toPixelFormat(int format);
> > + libcamera::PixelFormat toPixelFormat(int format) const;
> > + int sortStreamConfig(camera3_stream_configuration_t* streamList) const;
> > std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> > int64_t timestamp);
> >
> > --
> > 2.29.0.rc2.309.g374f81d7ae-goog
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list