| Message ID | 20200902104730.43451-3-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 02/09/2020 11:47, Jacopo Mondi wrote: > When producing the list of image resolution to claim as supported by the > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the > time, there was no JPEG support in place at all. > > With the introduction of support for JPEG compression, reporting > non-valid sizes as supported obviously causes troubles. > > In order to avoid reporting non-supported resolutions as supported, > produce the list of available JPEG sizes by using the ones supported > by the YCbCr_420_888 format, from which the JPEG stream is encoded. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------ > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index ad0d7fd15d90..8a8072123961 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations() > const std::vector<PixelFormat> &libcameraFormats = > camera3Format.libcameraFormats; > > + /* > + * Fixed format mapping for JPEG. > + * > + * The list of supported JPEG resolutions is generated > + * from the list of resolutions supported by > + * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced. > + * > + * \todo Wire the JPEG encoder interface to query the list > + * of supported resolutions. As we require NV12 for android, I think this is fine. The encoder 'could' encode other formats, but it only needs to encode NV12 - as there must always be a frame of that format right? (I envisage we'll have to add a pixel-convertor to take YUYV webcams to NV12, so even then, we'll still have an NV12 frame to encode). > + */ > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > + formatsMap_[androidFormat] = formats::MJPEG; > + continue; > + } > + > /* > * Test the libcamera formats that can produce images > * compatible with the format defined by Android. > */ > PixelFormat mappedFormat; > for (const PixelFormat &pixelFormat : libcameraFormats) { > - /* \todo Fixed mapping for JPEG. */ > - if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > - mappedFormat = formats::MJPEG; > - break; > - } > > /* > * The stream configuration size can be adjusted, > @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations() > cfg.size = res; > > CameraConfiguration::Status status = cameraConfig->validate(); > - /* > - * Unconditionally report we can produce JPEG. > - * > - * \todo The JPEG stream will be implemented as an > - * HAL-only stream, but some cameras can produce it > - * directly. As of now, claim support for JPEG without > - * inspecting where the JPEG stream is produced. > - */ > - if (androidFormat != HAL_PIXEL_FORMAT_BLOB && > - status != CameraConfiguration::Valid) > + if (status != CameraConfiguration::Valid) > continue; > > streamConfigurations_.push_back({ res, androidFormat }); > + > + /* > + * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888 > + * from which JPEG is produced, add an entry for > + * the JPEG stream. > + * > + * \todo Wire the JPEG encoder to query the supported > + * sizes provided a list of formats it can encode. I don't understand this comment. What do you need from the JPEG encoder? The supported sizes are whatever it's given.... It shouldn't change the size. > + */ > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) > + streamConfigurations_.push_back( > + { res, HAL_PIXEL_FORMAT_BLOB }); > } > } > >
Hi Kieran, On Wed, Sep 02, 2020 at 01:55:49PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 02/09/2020 11:47, Jacopo Mondi wrote: > > When producing the list of image resolution to claim as supported by the > > camera HAL, the JPEG stream was assumed to be 'always valid' as, at the > > time, there was no JPEG support in place at all. > > > > With the introduction of support for JPEG compression, reporting > > non-valid sizes as supported obviously causes troubles. > > > > In order to avoid reporting non-supported resolutions as supported, > > produce the list of available JPEG sizes by using the ones supported > > by the YCbCr_420_888 format, from which the JPEG stream is encoded. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------ > > 1 file changed, 28 insertions(+), 15 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index ad0d7fd15d90..8a8072123961 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations() > > const std::vector<PixelFormat> &libcameraFormats = > > camera3Format.libcameraFormats; > > > > + /* > > + * Fixed format mapping for JPEG. > > + * > > + * The list of supported JPEG resolutions is generated > > + * from the list of resolutions supported by > > + * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced. > > + * > > + * \todo Wire the JPEG encoder interface to query the list > > + * of supported resolutions. > > As we require NV12 for android, I think this is fine. > The encoder 'could' encode other formats, but it only needs to encode > NV12 - as there must always be a frame of that format right? > > > (I envisage we'll have to add a pixel-convertor to take YUYV webcams to > NV12, so even then, we'll still have an NV12 frame to encode). > > > > + */ > > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > > + formatsMap_[androidFormat] = formats::MJPEG; > > + continue; > > + } > > + > > /* > > * Test the libcamera formats that can produce images > > * compatible with the format defined by Android. > > */ > > PixelFormat mappedFormat; > > for (const PixelFormat &pixelFormat : libcameraFormats) { > > - /* \todo Fixed mapping for JPEG. */ > > - if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > > - mappedFormat = formats::MJPEG; > > - break; > > - } > > > > /* > > * The stream configuration size can be adjusted, > > @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations() > > cfg.size = res; > > > > CameraConfiguration::Status status = cameraConfig->validate(); > > - /* > > - * Unconditionally report we can produce JPEG. > > - * > > - * \todo The JPEG stream will be implemented as an > > - * HAL-only stream, but some cameras can produce it > > - * directly. As of now, claim support for JPEG without > > - * inspecting where the JPEG stream is produced. > > - */ > > - if (androidFormat != HAL_PIXEL_FORMAT_BLOB && > > - status != CameraConfiguration::Valid) > > + if (status != CameraConfiguration::Valid) > > continue; > > > > streamConfigurations_.push_back({ res, androidFormat }); > > + > > + /* > > + * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888 > > + * from which JPEG is produced, add an entry for > > + * the JPEG stream. > > + * > > + * \todo Wire the JPEG encoder to query the supported > > + * sizes provided a list of formats it can encode. > > I don't understand this comment. > > What do you need from the JPEG encoder? > > The supported sizes are whatever it's given.... It shouldn't change the > size. Currently we create the encoder at configureStream() time, as we only have one. Going forward I suppose we'll have multiple classes implementing the (currently minimal) Encoder interface, as on some platforms we could have some HW accelerated implementations. I also assumed other implementation might have requirements in terms of alignment, maximum and minimum supported sizes and so on, and supported formats the JPEG images can be produced from. The idea here is to be able to instantiate Encoders early, or at least identify the one in use earlier, and query it to learn about the above mentioned constraints, which should be reflected in the available formats reported to the framework. Does this make any sense to you ? Thanks j > > > + */ > > + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) > > + streamConfigurations_.push_back( > > + { res, HAL_PIXEL_FORMAT_BLOB }); > > } > > } > > > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 02/09/2020 14:15, Jacopo Mondi wrote: > Hi Kieran, > > On Wed, Sep 02, 2020 at 01:55:49PM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 02/09/2020 11:47, Jacopo Mondi wrote: >>> When producing the list of image resolution to claim as supported by the >>> camera HAL, the JPEG stream was assumed to be 'always valid' as, at the >>> time, there was no JPEG support in place at all. >>> >>> With the introduction of support for JPEG compression, reporting >>> non-valid sizes as supported obviously causes troubles. >>> >>> In order to avoid reporting non-supported resolutions as supported, >>> produce the list of available JPEG sizes by using the ones supported >>> by the YCbCr_420_888 format, from which the JPEG stream is encoded. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------ >>> 1 file changed, 28 insertions(+), 15 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index ad0d7fd15d90..8a8072123961 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations() >>> const std::vector<PixelFormat> &libcameraFormats = >>> camera3Format.libcameraFormats; >>> >>> + /* >>> + * Fixed format mapping for JPEG. >>> + * >>> + * The list of supported JPEG resolutions is generated >>> + * from the list of resolutions supported by >>> + * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced. >>> + * >>> + * \todo Wire the JPEG encoder interface to query the list >>> + * of supported resolutions. >> >> As we require NV12 for android, I think this is fine. >> The encoder 'could' encode other formats, but it only needs to encode >> NV12 - as there must always be a frame of that format right? >> >> >> (I envisage we'll have to add a pixel-convertor to take YUYV webcams to >> NV12, so even then, we'll still have an NV12 frame to encode). >> >> >>> + */ >>> + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { >>> + formatsMap_[androidFormat] = formats::MJPEG; >>> + continue; >>> + } >>> + >>> /* >>> * Test the libcamera formats that can produce images >>> * compatible with the format defined by Android. >>> */ >>> PixelFormat mappedFormat; >>> for (const PixelFormat &pixelFormat : libcameraFormats) { >>> - /* \todo Fixed mapping for JPEG. */ >>> - if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { >>> - mappedFormat = formats::MJPEG; >>> - break; >>> - } >>> >>> /* >>> * The stream configuration size can be adjusted, >>> @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations() >>> cfg.size = res; >>> >>> CameraConfiguration::Status status = cameraConfig->validate(); >>> - /* >>> - * Unconditionally report we can produce JPEG. >>> - * >>> - * \todo The JPEG stream will be implemented as an >>> - * HAL-only stream, but some cameras can produce it >>> - * directly. As of now, claim support for JPEG without >>> - * inspecting where the JPEG stream is produced. >>> - */ >>> - if (androidFormat != HAL_PIXEL_FORMAT_BLOB && >>> - status != CameraConfiguration::Valid) >>> + if (status != CameraConfiguration::Valid) >>> continue; >>> >>> streamConfigurations_.push_back({ res, androidFormat }); >>> + >>> + /* >>> + * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888 >>> + * from which JPEG is produced, add an entry for >>> + * the JPEG stream. >>> + * >>> + * \todo Wire the JPEG encoder to query the supported >>> + * sizes provided a list of formats it can encode. >> >> I don't understand this comment. >> >> What do you need from the JPEG encoder? >> >> The supported sizes are whatever it's given.... It shouldn't change the >> size. > > Currently we create the encoder at configureStream() time, as we only > have one. Going forward I suppose we'll have multiple classes > implementing the (currently minimal) Encoder interface, as on some > platforms we could have some HW accelerated implementations. > > I also assumed other implementation might have requirements in terms > of alignment, maximum and minimum supported sizes and so on, and > supported formats the JPEG images can be produced from. > > The idea here is to be able to instantiate Encoders early, or at least > identify the one in use earlier, and query it to learn about the above > mentioned constraints, which should be reflected in the available > formats reported to the framework. > > Does this make any sense to you ? Yes, thanks that helps a lot - I was in the wrong mindset - of only thinking about the existing software jpeg encoder. Seems I missed this: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Thanks > j > >> >>> + */ >>> + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) >>> + streamConfigurations_.push_back( >>> + { res, HAL_PIXEL_FORMAT_BLOB }); >>> } >>> } >>> >>> >> >> -- >> Regards >> -- >> Kieran
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index ad0d7fd15d90..8a8072123961 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations() const std::vector<PixelFormat> &libcameraFormats = camera3Format.libcameraFormats; + /* + * Fixed format mapping for JPEG. + * + * The list of supported JPEG resolutions is generated + * from the list of resolutions supported by + * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced. + * + * \todo Wire the JPEG encoder interface to query the list + * of supported resolutions. + */ + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { + formatsMap_[androidFormat] = formats::MJPEG; + continue; + } + /* * Test the libcamera formats that can produce images * compatible with the format defined by Android. */ PixelFormat mappedFormat; for (const PixelFormat &pixelFormat : libcameraFormats) { - /* \todo Fixed mapping for JPEG. */ - if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { - mappedFormat = formats::MJPEG; - break; - } /* * The stream configuration size can be adjusted, @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations() cfg.size = res; CameraConfiguration::Status status = cameraConfig->validate(); - /* - * Unconditionally report we can produce JPEG. - * - * \todo The JPEG stream will be implemented as an - * HAL-only stream, but some cameras can produce it - * directly. As of now, claim support for JPEG without - * inspecting where the JPEG stream is produced. - */ - if (androidFormat != HAL_PIXEL_FORMAT_BLOB && - status != CameraConfiguration::Valid) + if (status != CameraConfiguration::Valid) continue; streamConfigurations_.push_back({ res, androidFormat }); + + /* + * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888 + * from which JPEG is produced, add an entry for + * the JPEG stream. + * + * \todo Wire the JPEG encoder to query the supported + * sizes provided a list of formats it can encode. + */ + if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888) + streamConfigurations_.push_back( + { res, HAL_PIXEL_FORMAT_BLOB }); } }
When producing the list of image resolution to claim as supported by the camera HAL, the JPEG stream was assumed to be 'always valid' as, at the time, there was no JPEG support in place at all. With the introduction of support for JPEG compression, reporting non-valid sizes as supported obviously causes troubles. In order to avoid reporting non-supported resolutions as supported, produce the list of available JPEG sizes by using the ones supported by the YCbCr_420_888 format, from which the JPEG stream is encoded. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 15 deletions(-)