| Message ID | 20200902152236.69770-4-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Wed, Sep 02, 2020 at 05:22:27PM +0200, Jacopo Mondi wrote: > Add debug printouts to the CameraDevice::initializeStreamConfigurations() > function that helps to follow the process of building the stream s/helps/help/ > configurations map. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_device.cpp | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 8a8072123961..493d6cecde72 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -363,6 +363,9 @@ int CameraDevice::initializeStreamConfigurations() > const std::vector<PixelFormat> &libcameraFormats = > camera3Format.libcameraFormats; > > + LOG(HAL, Debug) << "Testing Android format: " > + << camera3Format.name; s/:// Maybe s/Testing/Mapping/ ? Not sure this is a test. I think I'd move this below the JPEG case, to avoid two debug messages for JPEG. > + > /* > * Fixed format mapping for JPEG. > * > @@ -375,6 +378,10 @@ int CameraDevice::initializeStreamConfigurations() > */ > if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > formatsMap_[androidFormat] = formats::MJPEG; > + LOG(HAL, Debug) << "Mapped Android format: " s/:// > + << camera3Format.name << " to: " s/:// Same in other messages below. > + << formats::MJPEG.toString() > + << " (fixed mapping)"; As this is always done, is there value in printing a message here ? > continue; > } > > @@ -385,6 +392,10 @@ int CameraDevice::initializeStreamConfigurations() > PixelFormat mappedFormat; > for (const PixelFormat &pixelFormat : libcameraFormats) { > > + LOG(HAL, Debug) << "Testing Android format: " > + << camera3Format.name << " with: " > + << pixelFormat.toString(); > + > /* > * The stream configuration size can be adjusted, > * not the pixel format. > @@ -420,14 +431,27 @@ int CameraDevice::initializeStreamConfigurations() > * stream configurations map, by testing the image resolutions. > */ > formatsMap_[androidFormat] = mappedFormat; > + LOG(HAL, Debug) << "Mapped Android format: " > + << camera3Format.name << " to: " > + << mappedFormat.toString(); > > for (const Size &res : cameraResolutions) { > cfg.pixelFormat = mappedFormat; > cfg.size = res; > > + std::stringstream ss; > + ss << "Testing (" << res.toString() << ")[" > + << mappedFormat.toString() << "]: "; Maybe use cfg.toString() ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > CameraConfiguration::Status status = cameraConfig->validate(); > - if (status != CameraConfiguration::Valid) > + if (status != CameraConfiguration::Valid) { > + ss << " not supported"; > + LOG(HAL, Debug) << ss.str(); > continue; > + } > + > + ss << " supported"; > + LOG(HAL, Debug) << ss.str(); > > streamConfigurations_.push_back({ res, androidFormat }); >
Hi, LGTM module a few nits. Re patch title, how about "Add more debug log in stream initialization process"? On Sun, Sep 6, 2020 at 2:55 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jacopo, > > Thank you for the patch. > > On Wed, Sep 02, 2020 at 05:22:27PM +0200, Jacopo Mondi wrote: > > Add debug printouts to the CameraDevice::initializeStreamConfigurations() > > function that helps to follow the process of building the stream > > s/helps/help/ > > > configurations map. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/android/camera_device.cpp | 26 +++++++++++++++++++++++++- > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 8a8072123961..493d6cecde72 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -363,6 +363,9 @@ int CameraDevice::initializeStreamConfigurations() > > const std::vector<PixelFormat> &libcameraFormats = > > camera3Format.libcameraFormats; > > > > + LOG(HAL, Debug) << "Testing Android format: " > > + << camera3Format.name; > > s/:// > > Maybe s/Testing/Mapping/ ? Not sure this is a test. > How about "Trying to map"? > I think I'd move this below the JPEG case, to avoid two debug messages > for JPEG. > I don't have a strong preference here, but I wonder if not moving is better? Therefore, we always pair debug log, Testing... - Mapped... Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > + > > /* > > * Fixed format mapping for JPEG. > > * > > @@ -375,6 +378,10 @@ int CameraDevice::initializeStreamConfigurations() > > */ > > if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > > formatsMap_[androidFormat] = formats::MJPEG; > > + LOG(HAL, Debug) << "Mapped Android format: " > > s/:// > > > + << camera3Format.name << " to: " > > s/:// > > Same in other messages below. > > > + << formats::MJPEG.toString() > > + << " (fixed mapping)"; > > As this is always done, is there value in printing a message here ? > > > continue; > > } > > > > @@ -385,6 +392,10 @@ int CameraDevice::initializeStreamConfigurations() > > PixelFormat mappedFormat; > > for (const PixelFormat &pixelFormat : libcameraFormats) { > > > > + LOG(HAL, Debug) << "Testing Android format: " > > + << camera3Format.name << " with: " > > + << pixelFormat.toString(); > > + > > /* > > * The stream configuration size can be adjusted, > > * not the pixel format. > > @@ -420,14 +431,27 @@ int CameraDevice::initializeStreamConfigurations() > > * stream configurations map, by testing the image resolutions. > > */ > > formatsMap_[androidFormat] = mappedFormat; > > + LOG(HAL, Debug) << "Mapped Android format: " > > + << camera3Format.name << " to: " > > + << mappedFormat.toString(); > > > > for (const Size &res : cameraResolutions) { > > cfg.pixelFormat = mappedFormat; > > cfg.size = res; > > > > + std::stringstream ss; > > + ss << "Testing (" << res.toString() << ")[" > > + << mappedFormat.toString() << "]: "; > > Maybe use cfg.toString() ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > CameraConfiguration::Status status = cameraConfig->validate(); > > - if (status != CameraConfiguration::Valid) > > + if (status != CameraConfiguration::Valid) { > > + ss << " not supported"; > > + LOG(HAL, Debug) << ss.str(); > > continue; > > + } > > + > > + ss << " supported"; > > + LOG(HAL, Debug) << ss.str(); > > > > streamConfigurations_.push_back({ res, androidFormat }); > > > > -- > Regards, > > Laurent Pinchart Best Regards, -Hiro > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On Mon, Sep 07, 2020 at 05:17:36PM +0900, Hirokazu Honda wrote: > Hi, > LGTM module a few nits. > > Re patch title, how about "Add more debug log in stream initialization process"? It would be nicer, but it breaks the 75-char rule on patch subjects, which is a pain to respect, but it's useful when displaying patches with git log in oneline mode (or git bisect, ot git based tools like tig or gitk) which breaks lines at that columns limit. Also, we still don't need to report fixes for backporting purposes, but when referring to other patches in the history in one commit message, reporting them in the canonical patch naming scheme: e21d2170f366 ("video: remove unnecessary platform_set_drvdata()") requires the subject of being of limited lenght to make it readable. > > On Sun, Sep 6, 2020 at 2:55 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Sep 02, 2020 at 05:22:27PM +0200, Jacopo Mondi wrote: > > > Add debug printouts to the CameraDevice::initializeStreamConfigurations() > > > function that helps to follow the process of building the stream > > > > s/helps/help/ > > > > > configurations map. > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/android/camera_device.cpp | 26 +++++++++++++++++++++++++- > > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 8a8072123961..493d6cecde72 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -363,6 +363,9 @@ int CameraDevice::initializeStreamConfigurations() > > > const std::vector<PixelFormat> &libcameraFormats = > > > camera3Format.libcameraFormats; > > > > > > + LOG(HAL, Debug) << "Testing Android format: " > > > + << camera3Format.name; > > > > s/:// > > > > Maybe s/Testing/Mapping/ ? Not sure this is a test. > > > > How about "Trying to map"? > > > I think I'd move this below the JPEG case, to avoid two debug messages > > for JPEG. > > > > I don't have a strong preference here, but I wonder if not moving is better? > Therefore, we always pair debug log, Testing... - Mapped... > Yes, I wanted this to be 'paired' like for other formats :) > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > Thanks j > > > + > > > /* > > > * Fixed format mapping for JPEG. > > > * > > > @@ -375,6 +378,10 @@ int CameraDevice::initializeStreamConfigurations() > > > */ > > > if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { > > > formatsMap_[androidFormat] = formats::MJPEG; > > > + LOG(HAL, Debug) << "Mapped Android format: " > > > > s/:// > > > > > + << camera3Format.name << " to: " > > > > s/:// > > > > Same in other messages below. > > > > > + << formats::MJPEG.toString() > > > + << " (fixed mapping)"; > > > > As this is always done, is there value in printing a message here ? > > > > > continue; > > > } > > > > > > @@ -385,6 +392,10 @@ int CameraDevice::initializeStreamConfigurations() > > > PixelFormat mappedFormat; > > > for (const PixelFormat &pixelFormat : libcameraFormats) { > > > > > > + LOG(HAL, Debug) << "Testing Android format: " > > > + << camera3Format.name << " with: " > > > + << pixelFormat.toString(); > > > + > > > /* > > > * The stream configuration size can be adjusted, > > > * not the pixel format. > > > @@ -420,14 +431,27 @@ int CameraDevice::initializeStreamConfigurations() > > > * stream configurations map, by testing the image resolutions. > > > */ > > > formatsMap_[androidFormat] = mappedFormat; > > > + LOG(HAL, Debug) << "Mapped Android format: " > > > + << camera3Format.name << " to: " > > > + << mappedFormat.toString(); > > > > > > for (const Size &res : cameraResolutions) { > > > cfg.pixelFormat = mappedFormat; > > > cfg.size = res; > > > > > > + std::stringstream ss; > > > + ss << "Testing (" << res.toString() << ")[" > > > + << mappedFormat.toString() << "]: "; > > > > Maybe use cfg.toString() ? > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + > > > CameraConfiguration::Status status = cameraConfig->validate(); > > > - if (status != CameraConfiguration::Valid) > > > + if (status != CameraConfiguration::Valid) { > > > + ss << " not supported"; > > > + LOG(HAL, Debug) << ss.str(); > > > continue; > > > + } > > > + > > > + ss << " supported"; > > > + LOG(HAL, Debug) << ss.str(); > > > > > > streamConfigurations_.push_back({ res, androidFormat }); > > > > > > > -- > > Regards, > > > > Laurent Pinchart > > Best Regards, > -Hiro > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 8a8072123961..493d6cecde72 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -363,6 +363,9 @@ int CameraDevice::initializeStreamConfigurations() const std::vector<PixelFormat> &libcameraFormats = camera3Format.libcameraFormats; + LOG(HAL, Debug) << "Testing Android format: " + << camera3Format.name; + /* * Fixed format mapping for JPEG. * @@ -375,6 +378,10 @@ int CameraDevice::initializeStreamConfigurations() */ if (androidFormat == HAL_PIXEL_FORMAT_BLOB) { formatsMap_[androidFormat] = formats::MJPEG; + LOG(HAL, Debug) << "Mapped Android format: " + << camera3Format.name << " to: " + << formats::MJPEG.toString() + << " (fixed mapping)"; continue; } @@ -385,6 +392,10 @@ int CameraDevice::initializeStreamConfigurations() PixelFormat mappedFormat; for (const PixelFormat &pixelFormat : libcameraFormats) { + LOG(HAL, Debug) << "Testing Android format: " + << camera3Format.name << " with: " + << pixelFormat.toString(); + /* * The stream configuration size can be adjusted, * not the pixel format. @@ -420,14 +431,27 @@ int CameraDevice::initializeStreamConfigurations() * stream configurations map, by testing the image resolutions. */ formatsMap_[androidFormat] = mappedFormat; + LOG(HAL, Debug) << "Mapped Android format: " + << camera3Format.name << " to: " + << mappedFormat.toString(); for (const Size &res : cameraResolutions) { cfg.pixelFormat = mappedFormat; cfg.size = res; + std::stringstream ss; + ss << "Testing (" << res.toString() << ")[" + << mappedFormat.toString() << "]: "; + CameraConfiguration::Status status = cameraConfig->validate(); - if (status != CameraConfiguration::Valid) + if (status != CameraConfiguration::Valid) { + ss << " not supported"; + LOG(HAL, Debug) << ss.str(); continue; + } + + ss << " supported"; + LOG(HAL, Debug) << ss.str(); streamConfigurations_.push_back({ res, androidFormat });