| Message ID | 20230116002808.16014-7-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Thank you for the patch! Only nits of the format. On Mon, Jan 16, 2023 at 8:28 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > The usual way to signal errors in libcamera is to return an error code. > Change the Encoder::generateThumbnail() function to return an int > instead of signaling errors by not resizing the thumbnail vector. This > allows propagating error codes to the callers. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/jpeg/encoder.h | 8 +-- > src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++----------- > src/android/jpeg/encoder_libjpeg.h | 8 +-- > src/android/jpeg/post_processor_jpeg.cpp | 6 +- > 4 files changed, 48 insertions(+), 44 deletions(-) > > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h > index 5f9ef890023a..d15f22e67830 100644 > --- a/src/android/jpeg/encoder.h > +++ b/src/android/jpeg/encoder.h > @@ -22,8 +22,8 @@ public: > libcamera::Span<uint8_t> destination, > libcamera::Span<const uint8_t> exifData, > unsigned int quality) = 0; > - virtual void generateThumbnail(const libcamera::FrameBuffer > &source, > - const libcamera::Size &targetSize, > - unsigned int quality, > - std::vector<unsigned char> > *thumbnail) = 0; > + virtual int generateThumbnail(const libcamera::FrameBuffer &source, > + const libcamera::Size &targetSize, > + unsigned int quality, > + std::vector<unsigned char> > *thumbnail) = 0; > }; > diff --git a/src/android/jpeg/encoder_libjpeg.cpp > b/src/android/jpeg/encoder_libjpeg.cpp > index 9bbf1abbe09c..8f4df7899dfd 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -89,53 +89,57 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, > Span<uint8_t> dest, > return captureEncoder_.encode(frame.planes(), dest, exifData, > quality); > } > > -void EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer > &source, > - const libcamera::Size &targetSize, > - unsigned int quality, > - std::vector<unsigned char> > *thumbnail) > +int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer > &source, > + const libcamera::Size &targetSize, > + unsigned int quality, > + std::vector<unsigned char> > *thumbnail) > { > /* Stores the raw scaled-down thumbnail bytes. */ > std::vector<unsigned char> rawThumbnail; > > thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); > + if (rawThumbnail.empty()) > + return -EINVAL; > > StreamConfiguration thCfg; > thCfg.size = targetSize; > thCfg.pixelFormat = thumbnailer_.pixelFormat(); > int ret = thumbnailEncoder_.configure(thCfg); > + if (ret) > + return ret; > > - if (!rawThumbnail.empty() && !ret) { > - /* > - * \todo Avoid value-initialization of all elements of the > - * vector. - */ > - thumbnail->resize(rawThumbnail.size()); > + /* > + * \todo Avoid value-initialization of all elements of the > + * vector. > nit: I guess |vector| could be merged in the above line, as the line is not that long now? (Is the limit 80 characters?) > + */ > + thumbnail->resize(rawThumbnail.size()); > > - /* > - * Split planes manually as the encoder expects a vector of > - * planes. > ditto > - * > - * \todo Pass a vector of planes directly to > - * Thumbnailer::createThumbnailer above and remove the > manual > - * planes split from here. > - */ > - std::vector<Span<uint8_t>> thumbnailPlanes; > - const PixelFormatInfo &formatNV12 = > - PixelFormatInfo::info(formats::NV12); > - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0); > - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1); > - thumbnailPlanes.push_back({ rawThumbnail.data(), > yPlaneSize }); > - thumbnailPlanes.push_back({ rawThumbnail.data() + > yPlaneSize, > - uvPlaneSize }); > + /* > + * Split planes manually as the encoder expects a vector of > + * planes. > + * > + * \todo Pass a vector of planes directly to > + * Thumbnailer::createThumbnailer above and remove the manual > + * planes split from here. > + */ > + std::vector<Span<uint8_t>> thumbnailPlanes; > + const PixelFormatInfo &formatNV12 = > + PixelFormatInfo::info(formats::NV12); > + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0); > + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1); > + thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize }); > + thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, > + uvPlaneSize }); > > - int jpegSize = thumbnailEncoder_.encode(thumbnailPlanes, > *thumbnail, > - {}, quality); > - thumbnail->resize(jpegSize); > + int jpegSize = thumbnailEncoder_.encode(thumbnailPlanes, > *thumbnail, > + {}, quality); > + thumbnail->resize(jpegSize); > > - LOG(JPEG, Debug) > - << "Thumbnail compress returned " > - << jpegSize << " bytes"; > - } > ditto > + LOG(JPEG, Debug) > + << "Thumbnail compress returned " > + << jpegSize << " bytes"; > + > + return 0; > } > > EncoderLibJpeg::Encoder::Encoder() > diff --git a/src/android/jpeg/encoder_libjpeg.h > b/src/android/jpeg/encoder_libjpeg.h > index a022a72e02bf..9cff4f1b42e3 100644 > --- a/src/android/jpeg/encoder_libjpeg.h > +++ b/src/android/jpeg/encoder_libjpeg.h > @@ -28,10 +28,10 @@ public: > libcamera::Span<uint8_t> destination, > libcamera::Span<const uint8_t> exifData, > unsigned int quality) override; > - void generateThumbnail(const libcamera::FrameBuffer &source, > - const libcamera::Size &targetSize, > - unsigned int quality, > - std::vector<unsigned char> *thumbnail) > override; > + int generateThumbnail(const libcamera::FrameBuffer &source, > + const libcamera::Size &targetSize, > + unsigned int quality, > + std::vector<unsigned char> *thumbnail) > override; > > private: > class Encoder > diff --git a/src/android/jpeg/post_processor_jpeg.cpp > b/src/android/jpeg/post_processor_jpeg.cpp > index 69b18a2e5945..5df89383e1af 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -115,9 +115,9 @@ void > PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu > > if (thumbnailSize != Size(0, 0)) { > std::vector<unsigned char> thumbnail; > - encoder_->generateThumbnail(source, thumbnailSize, > - quality, &thumbnail); > - if (!thumbnail.empty()) > + ret = encoder_->generateThumbnail(source, > thumbnailSize, > + quality, > &thumbnail); > + if (!ret) > exif.setThumbnail(std::move(thumbnail), > Exif::Compression::JPEG); > } > > -- > Regards, > > Laurent Pinchart > >
diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h index 5f9ef890023a..d15f22e67830 100644 --- a/src/android/jpeg/encoder.h +++ b/src/android/jpeg/encoder.h @@ -22,8 +22,8 @@ public: libcamera::Span<uint8_t> destination, libcamera::Span<const uint8_t> exifData, unsigned int quality) = 0; - virtual void generateThumbnail(const libcamera::FrameBuffer &source, - const libcamera::Size &targetSize, - unsigned int quality, - std::vector<unsigned char> *thumbnail) = 0; + virtual int generateThumbnail(const libcamera::FrameBuffer &source, + const libcamera::Size &targetSize, + unsigned int quality, + std::vector<unsigned char> *thumbnail) = 0; }; diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index 9bbf1abbe09c..8f4df7899dfd 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -89,53 +89,57 @@ int EncoderLibJpeg::encode(const FrameBuffer &source, Span<uint8_t> dest, return captureEncoder_.encode(frame.planes(), dest, exifData, quality); } -void EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source, - const libcamera::Size &targetSize, - unsigned int quality, - std::vector<unsigned char> *thumbnail) +int EncoderLibJpeg::generateThumbnail(const libcamera::FrameBuffer &source, + const libcamera::Size &targetSize, + unsigned int quality, + std::vector<unsigned char> *thumbnail) { /* Stores the raw scaled-down thumbnail bytes. */ std::vector<unsigned char> rawThumbnail; thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); + if (rawThumbnail.empty()) + return -EINVAL; StreamConfiguration thCfg; thCfg.size = targetSize; thCfg.pixelFormat = thumbnailer_.pixelFormat(); int ret = thumbnailEncoder_.configure(thCfg); + if (ret) + return ret; - if (!rawThumbnail.empty() && !ret) { - /* - * \todo Avoid value-initialization of all elements of the - * vector. - */ - thumbnail->resize(rawThumbnail.size()); + /* + * \todo Avoid value-initialization of all elements of the + * vector. + */ + thumbnail->resize(rawThumbnail.size()); - /* - * Split planes manually as the encoder expects a vector of - * planes. - * - * \todo Pass a vector of planes directly to - * Thumbnailer::createThumbnailer above and remove the manual - * planes split from here. - */ - std::vector<Span<uint8_t>> thumbnailPlanes; - const PixelFormatInfo &formatNV12 = - PixelFormatInfo::info(formats::NV12); - size_t yPlaneSize = formatNV12.planeSize(targetSize, 0); - size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1); - thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize }); - thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, - uvPlaneSize }); + /* + * Split planes manually as the encoder expects a vector of + * planes. + * + * \todo Pass a vector of planes directly to + * Thumbnailer::createThumbnailer above and remove the manual + * planes split from here. + */ + std::vector<Span<uint8_t>> thumbnailPlanes; + const PixelFormatInfo &formatNV12 = + PixelFormatInfo::info(formats::NV12); + size_t yPlaneSize = formatNV12.planeSize(targetSize, 0); + size_t uvPlaneSize = formatNV12.planeSize(targetSize, 1); + thumbnailPlanes.push_back({ rawThumbnail.data(), yPlaneSize }); + thumbnailPlanes.push_back({ rawThumbnail.data() + yPlaneSize, + uvPlaneSize }); - int jpegSize = thumbnailEncoder_.encode(thumbnailPlanes, *thumbnail, - {}, quality); - thumbnail->resize(jpegSize); + int jpegSize = thumbnailEncoder_.encode(thumbnailPlanes, *thumbnail, + {}, quality); + thumbnail->resize(jpegSize); - LOG(JPEG, Debug) - << "Thumbnail compress returned " - << jpegSize << " bytes"; - } + LOG(JPEG, Debug) + << "Thumbnail compress returned " + << jpegSize << " bytes"; + + return 0; } EncoderLibJpeg::Encoder::Encoder() diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h index a022a72e02bf..9cff4f1b42e3 100644 --- a/src/android/jpeg/encoder_libjpeg.h +++ b/src/android/jpeg/encoder_libjpeg.h @@ -28,10 +28,10 @@ public: libcamera::Span<uint8_t> destination, libcamera::Span<const uint8_t> exifData, unsigned int quality) override; - void generateThumbnail(const libcamera::FrameBuffer &source, - const libcamera::Size &targetSize, - unsigned int quality, - std::vector<unsigned char> *thumbnail) override; + int generateThumbnail(const libcamera::FrameBuffer &source, + const libcamera::Size &targetSize, + unsigned int quality, + std::vector<unsigned char> *thumbnail) override; private: class Encoder diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 69b18a2e5945..5df89383e1af 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -115,9 +115,9 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu if (thumbnailSize != Size(0, 0)) { std::vector<unsigned char> thumbnail; - encoder_->generateThumbnail(source, thumbnailSize, - quality, &thumbnail); - if (!thumbnail.empty()) + ret = encoder_->generateThumbnail(source, thumbnailSize, + quality, &thumbnail); + if (!ret) exif.setThumbnail(std::move(thumbnail), Exif::Compression::JPEG); }
The usual way to signal errors in libcamera is to return an error code. Change the Encoder::generateThumbnail() function to return an int instead of signaling errors by not resizing the thumbnail vector. This allows propagating error codes to the callers. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/android/jpeg/encoder.h | 8 +-- src/android/jpeg/encoder_libjpeg.cpp | 70 +++++++++++++----------- src/android/jpeg/encoder_libjpeg.h | 8 +-- src/android/jpeg/post_processor_jpeg.cpp | 6 +- 4 files changed, 48 insertions(+), 44 deletions(-)