[libcamera-devel,RFC] android: camera_device: Reorder configurations given by a HAL client
diff mbox series

Message ID 20201028100134.3007829-1-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,RFC] android: camera_device: Reorder configurations given by a HAL client
Related show

Commit Message

Hirokazu Honda Oct. 28, 2020, 10:01 a.m. UTC 
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@chromium.org>
---
 src/android/camera_device.cpp | 108 +++++++++++++++++++++++++++++++++-
 src/android/camera_device.h   |   3 +-
 2 files changed, 109 insertions(+), 2 deletions(-)

--
2.29.0.rc2.309.g374f81d7ae-goog

Comments

Hirokazu Honda Oct. 28, 2020, 10:04 a.m. UTC | #1 
I would like to call for comments about adding this ad-hoc reordering
before CameraDevice configures pipeline handles.
The purpose is that a pipeline handler can produce streams from which
given configurations can be achieved.

Best Regards,
-Hiro

On Wed, Oct 28, 2020 at 7:01 PM Hirokazu Honda <hiroh@chromium.org> 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@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
> +{
> +       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
Hirokazu Honda Oct. 28, 2020, 10:15 a.m. UTC | #2 
Definitely please ignore printStreamList(). I forgot removing that in
uploading the patch.

On Wed, Oct 28, 2020 at 7:04 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> I would like to call for comments about adding this ad-hoc reordering
> before CameraDevice configures pipeline handles.
> The purpose is that a pipeline handler can produce streams from which
> given configurations can be achieved.
>
> Best Regards,
> -Hiro
>
> On Wed, Oct 28, 2020 at 7:01 PM Hirokazu Honda <hiroh@chromium.org> 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@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
> > +{
> > +       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
Hirokazu Honda Nov. 4, 2020, 9:46 a.m. UTC | #3 
gentle ping!

On Wed, Oct 28, 2020 at 7:15 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> Definitely please ignore printStreamList(). I forgot removing that in
> uploading the patch.
>
> On Wed, Oct 28, 2020 at 7:04 PM Hirokazu Honda <hiroh@chromium.org> wrote:
> >
> > I would like to call for comments about adding this ad-hoc reordering
> > before CameraDevice configures pipeline handles.
> > The purpose is that a pipeline handler can produce streams from which
> > given configurations can be achieved.
> >
> > Best Regards,
> > -Hiro
> >
> > On Wed, Oct 28, 2020 at 7:01 PM Hirokazu Honda <hiroh@chromium.org> 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@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
> > > +{
> > > +       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
Jacopo Mondi Nov. 4, 2020, 2:48 p.m. UTC | #4 
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@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 ?

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 ?


> +{
> +	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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda Nov. 5, 2020, 11:29 a.m. UTC | #5 
Hi Jacopo,
Thanks for replying.

On Wed, Nov 4, 2020 at 11:48 PM Jacopo Mondi <jacopo@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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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
+{
+	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);