| Message ID | 20200805153745.30378-1-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Wed, Aug 05, 2020 at 05:37:45PM +0200, 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> > --- > > Patch to be applied on top of Kieran's JPEG work. > --- > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index ec8ca934842a..6a9a038a2b53 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() > */ > formatsMap_[androidFormat] = mappedFormat; > > + /* > + * Stop here for JPEG streams: the JPEG supported sizes will > + * be tested later using the here recorded non-blob stream sizes. > + */ > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) > + continue; > + > for (const Size &res : cameraResolutions) { > cfg.pixelFormat = mappedFormat; > 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 (cameraConfig->validate() != CameraConfiguration::Valid) > continue; > > streamConfigurations_.push_back({ res, androidFormat }); > } > } > > + /* > + * Insert the JPEG sizes by using the ones recorded for YUV streams > + * from which JPEG is produced. > + */ > + std::vector<Camera3StreamConfiguration> jpegConfigurations; > + jpegConfigurations.reserve(cameraResolutions.size()); > + > + for (const auto &config : streamConfigurations_) { > + /* \todo JPEG can be produced from other formats too! */ Another todo item, the android.scaler.availableStreamConfigurations documentation lists required resolutions (see https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html which very annoyingly googlesource.com can't display as html in a web browser). JPEG android.sensor.info.activeArraySize Any JPEG 1920x1080 (1080p) Any if 1080p <= activeArraySize JPEG 1280x720 (720) Any if 720p <= activeArraySize JPEG 640x480 (480p) Any if 480p <= activeArraySize JPEG 320x240 (240p) Any if 240p <= activeArraySize > + if (config.androidFormat != HAL_PIXEL_FORMAT_YCbCr_420_888) > + continue; > + > + jpegConfigurations.push_back({ config.resolution, > + HAL_PIXEL_FORMAT_BLOB }); > + } > + > + for (auto const jpegConfig : jpegConfigurations) > + streamConfigurations_.push_back(jpegConfig); > + > LOG(HAL, Debug) << "Collected stream configuration map: "; > for (const auto &entry : streamConfigurations_) > LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
Hi Laurent, +Tomasz, Hiro and Han-Lin as there's a cros question at the end :) +Kieran for the Compressor interface part On Wed, Aug 05, 2020 at 07:53:41PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Wed, Aug 05, 2020 at 05:37:45PM +0200, 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> > > --- > > > > Patch to be applied on top of Kieran's JPEG work. > > --- > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index ec8ca934842a..6a9a038a2b53 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() > > */ > > formatsMap_[androidFormat] = mappedFormat; > > > > + /* > > + * Stop here for JPEG streams: the JPEG supported sizes will > > + * be tested later using the here recorded non-blob stream sizes. > > + */ > > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) > > + continue; > > + > > for (const Size &res : cameraResolutions) { > > cfg.pixelFormat = mappedFormat; > > 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 (cameraConfig->validate() != CameraConfiguration::Valid) > > continue; > > > > streamConfigurations_.push_back({ res, androidFormat }); > > } > > } > > > > + /* > > + * Insert the JPEG sizes by using the ones recorded for YUV streams > > + * from which JPEG is produced. > > + */ > > + std::vector<Camera3StreamConfiguration> jpegConfigurations; > > + jpegConfigurations.reserve(cameraResolutions.size()); > > + > > + for (const auto &config : streamConfigurations_) { > > + /* \todo JPEG can be produced from other formats too! */ > > Another todo item, the android.scaler.availableStreamConfigurations > documentation lists required resolutions (see > https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html > which very annoyingly googlesource.com can't display as html in a web browser). > > JPEG android.sensor.info.activeArraySize Any > JPEG 1920x1080 (1080p) Any if 1080p <= activeArraySize > JPEG 1280x720 (720) Any if 720p <= activeArraySize > JPEG 640x480 (480p) Any if 480p <= activeArraySize > JPEG 320x240 (240p) Any if 240p <= activeArraySize > Those are already mandatory to be supported if I'm not mistaken. we feed cameraResolutions for all the non-jpeg formats here: for (const Size &res : cameraResolutions) { cfg.pixelFormat = mappedFormat; cfg.size = res; if (cameraConfig->validate() != CameraConfiguration::Valid) continue; streamConfigurations_.push_back({ res, androidFormat }); } Then we use the sizes associated with the mandatory HAL_PIXEL_FORMAT_YCbCr_420_888 to add entries for jpeg. Have I missed something on this part ? Looking at the code now, we could create an initJpeg() functions, as all the jpeg streams are basically just ignored now and initialized at the end of the function. Maybe it's really time to break this file apart, it's already quite big and terse. What if we delegate the jpeg initalization part to the Compressor interface (looking at it now, we could have gone for Encoder as a name maybe). As of now it would basically require two more methods: a way to get the a metadata pack with the values of ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES and ANDROID_JPEG_MAX_SIZE initalized, and a vector of supported sizes given the vector of the ones so far collected. Then we'll probably need a factory to create the right Compressor, but that's for later and we should pobably first discuss how to decide "which" encoder to initialize before getting to the "how". In example, how does it work today in CrOS and what are the requirements going forward ? Thanks j > > + if (config.androidFormat != HAL_PIXEL_FORMAT_YCbCr_420_888) > > + continue; > > + > > + jpegConfigurations.push_back({ config.resolution, > > + HAL_PIXEL_FORMAT_BLOB }); > > + } > > + > > + for (auto const jpegConfig : jpegConfigurations) > > + streamConfigurations_.push_back(jpegConfig); > > + > > LOG(HAL, Debug) << "Collected stream configuration map: "; > > for (const auto &entry : streamConfigurations_) > > LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - " > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Wed, Aug 5, 2020 at 11:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Laurent, > +Tomasz, Hiro and Han-Lin as there's a cros question at the end :) > +Kieran for the Compressor interface part > > On Wed, Aug 05, 2020 at 07:53:41PM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > On Wed, Aug 05, 2020 at 05:37:45PM +0200, 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> > > > --- > > > > > > Patch to be applied on top of Kieran's JPEG work. > > > --- > > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index ec8ca934842a..6a9a038a2b53 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() > > > */ > > > formatsMap_[androidFormat] = mappedFormat; > > > > > > + /* > > > + * Stop here for JPEG streams: the JPEG supported sizes will > > > + * be tested later using the here recorded non-blob stream sizes. > > > + */ > > > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) > > > + continue; > > > + > > > for (const Size &res : cameraResolutions) { > > > cfg.pixelFormat = mappedFormat; > > > 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 (cameraConfig->validate() != CameraConfiguration::Valid) > > > continue; > > > > > > streamConfigurations_.push_back({ res, androidFormat }); > > > } > > > } > > > > > > + /* > > > + * Insert the JPEG sizes by using the ones recorded for YUV streams > > > + * from which JPEG is produced. > > > + */ > > > + std::vector<Camera3StreamConfiguration> jpegConfigurations; > > > + jpegConfigurations.reserve(cameraResolutions.size()); > > > + > > > + for (const auto &config : streamConfigurations_) { > > > + /* \todo JPEG can be produced from other formats too! */ > > > > Another todo item, the android.scaler.availableStreamConfigurations > > documentation lists required resolutions (see > > https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html > > which very annoyingly googlesource.com can't display as html in a web browser). > > > > JPEG android.sensor.info.activeArraySize Any > > JPEG 1920x1080 (1080p) Any if 1080p <= activeArraySize > > JPEG 1280x720 (720) Any if 720p <= activeArraySize > > JPEG 640x480 (480p) Any if 480p <= activeArraySize > > JPEG 320x240 (240p) Any if 240p <= activeArraySize > > > > Those are already mandatory to be supported if I'm not mistaken. > > we feed cameraResolutions for all the non-jpeg formats here: > > for (const Size &res : cameraResolutions) { > cfg.pixelFormat = mappedFormat; > cfg.size = res; > > if (cameraConfig->validate() != CameraConfiguration::Valid) > continue; > > streamConfigurations_.push_back({ res, androidFormat }); > } > > Then we use the sizes associated with the mandatory > HAL_PIXEL_FORMAT_YCbCr_420_888 to add entries for jpeg. Have I missed > something on this part ? > > Looking at the code now, we could create an initJpeg() functions, as > all the jpeg streams are basically just ignored now and > initialized at the end of the function. > > Maybe it's really time to break this file apart, it's already quite > big and terse. What if we delegate the jpeg initalization part to the > Compressor interface (looking at it now, we could have gone for > Encoder as a name maybe). > > As of now it would basically require two > more methods: a way to get the a metadata pack with the values of > ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES and ANDROID_JPEG_MAX_SIZE > initalized, and a vector of supported sizes given the vector of the > ones so far collected. > > Then we'll probably need a factory to create the right Compressor, but > that's for later and we should pobably first discuss how to decide > "which" encoder to initialize before getting to the "how". > > In example, how does it work today in CrOS and what are the > requirements going forward ? If you mean JPEG resolution advertisement, I think it depends on the camera hardware. If it's a UVC camera, it often has higher resolution capture capabilities with MJPEG rather than YUV. At the same time, Android requires YUV capture at "RECORD" resolutions, which are typically higher (e.g. 1920x1080) than what a UVC camera can output at reasonable framerate without resorting to MJPEG, so we have to advertise any resolution supported by MJPEG as supported for YUV streams and decode the MJPEG frames in the HAL to provide YUV frames to the user. For RAW cameras, the JPEG streams are usually provided from YUV streams anyway, so I believe reporting all the resolutions supported by YUV as supported by JPEG should be correct. If you mean JPEG encoder selection, we have the encoding abstracted using a generic userspace interface exposed from Chrome (called JEA - JpegEncodeAccelerator), which automatically uses whatever available in the system. Currently it provides 3 implementations - V4L2, VAAPI and software. Best regards, Tomasz
Hi Tomasz, On Thu, Aug 06, 2020 at 01:04:47PM +0200, Tomasz Figa wrote: > Hi Jacopo, > > On Wed, Aug 5, 2020 at 11:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Laurent, > > +Tomasz, Hiro and Han-Lin as there's a cros question at the end :) > > +Kieran for the Compressor interface part > > > > On Wed, Aug 05, 2020 at 07:53:41PM +0300, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > On Wed, Aug 05, 2020 at 05:37:45PM +0200, 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> > > > > --- > > > > > > > > Patch to be applied on top of Kieran's JPEG work. > > > > --- > > > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index ec8ca934842a..6a9a038a2b53 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() > > > > */ > > > > formatsMap_[androidFormat] = mappedFormat; > > > > > > > > + /* > > > > + * Stop here for JPEG streams: the JPEG supported sizes will > > > > + * be tested later using the here recorded non-blob stream sizes. > > > > + */ > > > > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) > > > > + continue; > > > > + > > > > for (const Size &res : cameraResolutions) { > > > > cfg.pixelFormat = mappedFormat; > > > > 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 (cameraConfig->validate() != CameraConfiguration::Valid) > > > > continue; > > > > > > > > streamConfigurations_.push_back({ res, androidFormat }); > > > > } > > > > } > > > > > > > > + /* > > > > + * Insert the JPEG sizes by using the ones recorded for YUV streams > > > > + * from which JPEG is produced. > > > > + */ > > > > + std::vector<Camera3StreamConfiguration> jpegConfigurations; > > > > + jpegConfigurations.reserve(cameraResolutions.size()); > > > > + > > > > + for (const auto &config : streamConfigurations_) { > > > > + /* \todo JPEG can be produced from other formats too! */ > > > > > > Another todo item, the android.scaler.availableStreamConfigurations > > > documentation lists required resolutions (see > > > https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html > > > which very annoyingly googlesource.com can't display as html in a web browser). > > > > > > JPEG android.sensor.info.activeArraySize Any > > > JPEG 1920x1080 (1080p) Any if 1080p <= activeArraySize > > > JPEG 1280x720 (720) Any if 720p <= activeArraySize > > > JPEG 640x480 (480p) Any if 480p <= activeArraySize > > > JPEG 320x240 (240p) Any if 240p <= activeArraySize > > > > > > > Those are already mandatory to be supported if I'm not mistaken. > > > > we feed cameraResolutions for all the non-jpeg formats here: > > > > for (const Size &res : cameraResolutions) { > > cfg.pixelFormat = mappedFormat; > > cfg.size = res; > > > > if (cameraConfig->validate() != CameraConfiguration::Valid) > > continue; > > > > streamConfigurations_.push_back({ res, androidFormat }); > > } > > > > Then we use the sizes associated with the mandatory > > HAL_PIXEL_FORMAT_YCbCr_420_888 to add entries for jpeg. Have I missed > > something on this part ? > > > > Looking at the code now, we could create an initJpeg() functions, as > > all the jpeg streams are basically just ignored now and > > initialized at the end of the function. > > > > Maybe it's really time to break this file apart, it's already quite > > big and terse. What if we delegate the jpeg initalization part to the > > Compressor interface (looking at it now, we could have gone for > > Encoder as a name maybe). > > > > As of now it would basically require two > > more methods: a way to get the a metadata pack with the values of > > ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES and ANDROID_JPEG_MAX_SIZE > > initalized, and a vector of supported sizes given the vector of the > > ones so far collected. > > > > Then we'll probably need a factory to create the right Compressor, but > > that's for later and we should pobably first discuss how to decide > > "which" encoder to initialize before getting to the "how". > > > > In example, how does it work today in CrOS and what are the > > requirements going forward ? > > If you mean JPEG resolution advertisement, I think it depends on the > camera hardware. > > If it's a UVC camera, it often has higher resolution capture > capabilities with MJPEG rather than YUV. At the same time, Android > requires YUV capture at "RECORD" resolutions, which are typically > higher (e.g. 1920x1080) than what a UVC camera can output at > reasonable framerate without resorting to MJPEG, so we have to > advertise any resolution supported by MJPEG as supported for YUV > streams and decode the MJPEG frames in the HAL to provide YUV frames > to the user. > > For RAW cameras, the JPEG streams are usually provided from YUV > streams anyway, so I believe reporting all the resolutions supported > by YUV as supported by JPEG should be correct. Thanks, that's usefull to know. > > If you mean JPEG encoder selection, we have the encoding abstracted > using a generic userspace interface exposed from Chrome (called JEA - > JpegEncodeAccelerator), which automatically uses whatever available in > the system. Currently it provides 3 implementations - V4L2, VAAPI and > software. Yep, I meant this part. My first thought was a factory in our Camera HAL, but the criteria to decide which encode to instantiate should be defined first. Otherwise the alternative is for downstream to maintain a version "which does the right thing" and I think we want to avoid it. Thanks j > > Best regards, > Tomasz
Hi Jacopo, On 05/08/2020 22:33, Jacopo Mondi wrote: > Hi Laurent, > +Tomasz, Hiro and Han-Lin as there's a cros question at the end :) > +Kieran for the Compressor interface part > > On Wed, Aug 05, 2020 at 07:53:41PM +0300, Laurent Pinchart wrote: >> Hi Jacopo, >> >> On Wed, Aug 05, 2020 at 05:37:45PM +0200, 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> >>> --- >>> >>> Patch to be applied on top of Kieran's JPEG work. >>> --- >>> src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- >>> 1 file changed, 27 insertions(+), 11 deletions(-) >>> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>> index ec8ca934842a..6a9a038a2b53 100644 >>> --- a/src/android/camera_device.cpp >>> +++ b/src/android/camera_device.cpp >>> @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() >>> */ >>> formatsMap_[androidFormat] = mappedFormat; >>> >>> + /* >>> + * Stop here for JPEG streams: the JPEG supported sizes will >>> + * be tested later using the here recorded non-blob stream sizes. >>> + */ >>> + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) >>> + continue; >>> + >>> for (const Size &res : cameraResolutions) { >>> cfg.pixelFormat = mappedFormat; >>> 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 (cameraConfig->validate() != CameraConfiguration::Valid) >>> continue; >>> >>> streamConfigurations_.push_back({ res, androidFormat }); >>> } >>> } >>> >>> + /* >>> + * Insert the JPEG sizes by using the ones recorded for YUV streams >>> + * from which JPEG is produced. >>> + */ >>> + std::vector<Camera3StreamConfiguration> jpegConfigurations; >>> + jpegConfigurations.reserve(cameraResolutions.size()); >>> + >>> + for (const auto &config : streamConfigurations_) { >>> + /* \todo JPEG can be produced from other formats too! */ >> >> Another todo item, the android.scaler.availableStreamConfigurations >> documentation lists required resolutions (see >> https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html >> which very annoyingly googlesource.com can't display as html in a web browser). >> >> JPEG android.sensor.info.activeArraySize Any >> JPEG 1920x1080 (1080p) Any if 1080p <= activeArraySize >> JPEG 1280x720 (720) Any if 720p <= activeArraySize >> JPEG 640x480 (480p) Any if 480p <= activeArraySize >> JPEG 320x240 (240p) Any if 240p <= activeArraySize >> > > Those are already mandatory to be supported if I'm not mistaken. > > we feed cameraResolutions for all the non-jpeg formats here: > > for (const Size &res : cameraResolutions) { > cfg.pixelFormat = mappedFormat; > cfg.size = res; > > if (cameraConfig->validate() != CameraConfiguration::Valid) > continue; > > streamConfigurations_.push_back({ res, androidFormat }); > } > > Then we use the sizes associated with the mandatory > HAL_PIXEL_FORMAT_YCbCr_420_888 to add entries for jpeg. Have I missed > something on this part ? > > Looking at the code now, we could create an initJpeg() functions, as > all the jpeg streams are basically just ignored now and > initialized at the end of the function. > > Maybe it's really time to break this file apart, it's already quite > big and terse. What if we delegate the jpeg initalization part to the > Compressor interface (looking at it now, we could have gone for > Encoder as a name maybe). I certainly agree we need to split camera_device up. But for me, I would instead consider that the compressor(/encoder) shouldn't know about anything except doing that job. Management of the stream should be handled by a CameraStream interface (i.e. the struct CameraStream promoted to a full class, with it's own camera_stream.cpp). That's the layer that should be dealing with the interface between android streams and libcamera streams IMO. It should then be up to any instance of a CameraStream to decide if and how to handle the requirements for that stream, which may be to use a libcamera stream, or to use/implement a more configurable stream which might include scaler/convertor/compressors as required. As for using the name Encoder, yes that's fine by me. I'll rename throughout my series now, I don't yet know if that will require a repost, but I had hoped to push already. I'll see if it warrants a v5 posting to the list or not depending on the churn. -- Kieran > > As of now it would basically require two > more methods: a way to get the a metadata pack with the values of > ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES and ANDROID_JPEG_MAX_SIZE > initalized, and a vector of supported sizes given the vector of the > ones so far collected. > > Then we'll probably need a factory to create the right Compressor, but > that's for later and we should pobably first discuss how to decide > "which" encoder to initialize before getting to the "how". > > In example, how does it work today in CrOS and what are the > requirements going forward ? > > Thanks > j > >>> + if (config.androidFormat != HAL_PIXEL_FORMAT_YCbCr_420_888) >>> + continue; >>> + >>> + jpegConfigurations.push_back({ config.resolution, >>> + HAL_PIXEL_FORMAT_BLOB }); >>> + } >>> + >>> + for (auto const jpegConfig : jpegConfigurations) >>> + streamConfigurations_.push_back(jpegConfig); >>> + >>> LOG(HAL, Debug) << "Collected stream configuration map: "; >>> for (const auto &entry : streamConfigurations_) >>> LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - " >> >> -- >> Regards, >> >> Laurent Pinchart
On Thu, Aug 6, 2020 at 3:19 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Tomasz, > > On Thu, Aug 06, 2020 at 01:04:47PM +0200, Tomasz Figa wrote: > > Hi Jacopo, > > > > On Wed, Aug 5, 2020 at 11:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Laurent, > > > +Tomasz, Hiro and Han-Lin as there's a cros question at the end :) > > > +Kieran for the Compressor interface part > > > > > > On Wed, Aug 05, 2020 at 07:53:41PM +0300, Laurent Pinchart wrote: > > > > Hi Jacopo, > > > > > > > > On Wed, Aug 05, 2020 at 05:37:45PM +0200, 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> > > > > > --- > > > > > > > > > > Patch to be applied on top of Kieran's JPEG work. > > > > > --- > > > > > src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- > > > > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > index ec8ca934842a..6a9a038a2b53 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() > > > > > */ > > > > > formatsMap_[androidFormat] = mappedFormat; > > > > > > > > > > + /* > > > > > + * Stop here for JPEG streams: the JPEG supported sizes will > > > > > + * be tested later using the here recorded non-blob stream sizes. > > > > > + */ > > > > > + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) > > > > > + continue; > > > > > + > > > > > for (const Size &res : cameraResolutions) { > > > > > cfg.pixelFormat = mappedFormat; > > > > > 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 (cameraConfig->validate() != CameraConfiguration::Valid) > > > > > continue; > > > > > > > > > > streamConfigurations_.push_back({ res, androidFormat }); > > > > > } > > > > > } > > > > > > > > > > + /* > > > > > + * Insert the JPEG sizes by using the ones recorded for YUV streams > > > > > + * from which JPEG is produced. > > > > > + */ > > > > > + std::vector<Camera3StreamConfiguration> jpegConfigurations; > > > > > + jpegConfigurations.reserve(cameraResolutions.size()); > > > > > + > > > > > + for (const auto &config : streamConfigurations_) { > > > > > + /* \todo JPEG can be produced from other formats too! */ > > > > > > > > Another todo item, the android.scaler.availableStreamConfigurations > > > > documentation lists required resolutions (see > > > > https://android.googlesource.com/platform/system/media/+/refs/heads/master/camera/docs/docs.html > > > > which very annoyingly googlesource.com can't display as html in a web browser). > > > > > > > > JPEG android.sensor.info.activeArraySize Any > > > > JPEG 1920x1080 (1080p) Any if 1080p <= activeArraySize > > > > JPEG 1280x720 (720) Any if 720p <= activeArraySize > > > > JPEG 640x480 (480p) Any if 480p <= activeArraySize > > > > JPEG 320x240 (240p) Any if 240p <= activeArraySize > > > > > > > > > > Those are already mandatory to be supported if I'm not mistaken. > > > > > > we feed cameraResolutions for all the non-jpeg formats here: > > > > > > for (const Size &res : cameraResolutions) { > > > cfg.pixelFormat = mappedFormat; > > > cfg.size = res; > > > > > > if (cameraConfig->validate() != CameraConfiguration::Valid) > > > continue; > > > > > > streamConfigurations_.push_back({ res, androidFormat }); > > > } > > > > > > Then we use the sizes associated with the mandatory > > > HAL_PIXEL_FORMAT_YCbCr_420_888 to add entries for jpeg. Have I missed > > > something on this part ? > > > > > > Looking at the code now, we could create an initJpeg() functions, as > > > all the jpeg streams are basically just ignored now and > > > initialized at the end of the function. > > > > > > Maybe it's really time to break this file apart, it's already quite > > > big and terse. What if we delegate the jpeg initalization part to the > > > Compressor interface (looking at it now, we could have gone for > > > Encoder as a name maybe). > > > > > > As of now it would basically require two > > > more methods: a way to get the a metadata pack with the values of > > > ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES and ANDROID_JPEG_MAX_SIZE > > > initalized, and a vector of supported sizes given the vector of the > > > ones so far collected. > > > > > > Then we'll probably need a factory to create the right Compressor, but > > > that's for later and we should pobably first discuss how to decide > > > "which" encoder to initialize before getting to the "how". > > > > > > In example, how does it work today in CrOS and what are the > > > requirements going forward ? > > > > If you mean JPEG resolution advertisement, I think it depends on the > > camera hardware. > > > > If it's a UVC camera, it often has higher resolution capture > > capabilities with MJPEG rather than YUV. At the same time, Android > > requires YUV capture at "RECORD" resolutions, which are typically > > higher (e.g. 1920x1080) than what a UVC camera can output at > > reasonable framerate without resorting to MJPEG, so we have to > > advertise any resolution supported by MJPEG as supported for YUV > > streams and decode the MJPEG frames in the HAL to provide YUV frames > > to the user. > > > > For RAW cameras, the JPEG streams are usually provided from YUV > > streams anyway, so I believe reporting all the resolutions supported > > by YUV as supported by JPEG should be correct. > > Thanks, that's usefull to know. Also note that for MJPEG output from UVC cameras, it would be a waste to re-encode the YUV back into JPEG, so we instead just perform a lightweight conversion of the MJPEG frame into a proper JPEG (AFAIR just some headers need to be added). > > > > > If you mean JPEG encoder selection, we have the encoding abstracted > > using a generic userspace interface exposed from Chrome (called JEA - > > JpegEncodeAccelerator), which automatically uses whatever available in > > the system. Currently it provides 3 implementations - V4L2, VAAPI and > > software. > > Yep, I meant this part. My first thought was a factory in our Camera > HAL, but the criteria to decide which encode to instantiate should be > defined first. Otherwise the alternative is for downstream to maintain > a version "which does the right thing" and I think we want to avoid > it. In Chrome OS, the camera service may not have access to the JPEG encoder hardware (which could be exposed via VAAPI), so we would always want to delegate the encoding to JEA. Best regards, Tomasz
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index ec8ca934842a..6a9a038a2b53 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -398,27 +398,43 @@ int CameraDevice::initializeStreamConfigurations() */ formatsMap_[androidFormat] = mappedFormat; + /* + * Stop here for JPEG streams: the JPEG supported sizes will + * be tested later using the here recorded non-blob stream sizes. + */ + if (androidFormat == HAL_PIXEL_FORMAT_BLOB) + continue; + for (const Size &res : cameraResolutions) { cfg.pixelFormat = mappedFormat; 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 (cameraConfig->validate() != CameraConfiguration::Valid) continue; streamConfigurations_.push_back({ res, androidFormat }); } } + /* + * Insert the JPEG sizes by using the ones recorded for YUV streams + * from which JPEG is produced. + */ + std::vector<Camera3StreamConfiguration> jpegConfigurations; + jpegConfigurations.reserve(cameraResolutions.size()); + + for (const auto &config : streamConfigurations_) { + /* \todo JPEG can be produced from other formats too! */ + if (config.androidFormat != HAL_PIXEL_FORMAT_YCbCr_420_888) + continue; + + jpegConfigurations.push_back({ config.resolution, + HAL_PIXEL_FORMAT_BLOB }); + } + + for (auto const jpegConfig : jpegConfigurations) + streamConfigurations_.push_back(jpegConfig); + LOG(HAL, Debug) << "Collected stream configuration map: "; for (const auto &entry : streamConfigurations_) LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
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> --- Patch to be applied on top of Kieran's JPEG work. --- src/android/camera_device.cpp | 38 +++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 11 deletions(-) -- 2.27.0