| Message ID | 20230915-libyuv-convert-v1-3-1e5bcf68adac@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Mattijs On Fri, Sep 15, 2023 at 09:57:27AM +0200, Mattijs Korpershoek via libcamera-devel wrote: > PostProcessorYuv assumes that both source and destination pixel formats > will always be formats::NV12. > This might change in the future, for example when doing pixel format > conversion via libyuv. > > Add the necessary plumbing so that other formats can be added easily in > the future. > > Also increase plane number to 3, since some YUV format [1] are > fully planar (and thus have 3 planes). > > [1] https://docs.kernel.org/userspace-api/media/v4l/pixfmt-yuv-planar.html#fully-planar-yuv-formats > Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > --- > src/android/yuv/post_processor_yuv.cpp | 26 +++++++++++++++----------- > src/android/yuv/post_processor_yuv.h | 12 ++++++++---- > 2 files changed, 23 insertions(+), 15 deletions(-) > > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > index d58090db14ee..734bb85b7351 100644 > --- a/src/android/yuv/post_processor_yuv.cpp > +++ b/src/android/yuv/post_processor_yuv.cpp > @@ -90,18 +90,18 @@ void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuf > bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > const CameraBuffer &destination) const > { > - if (source.planes().size() != 2) { > + if (source.planes().size() != sourceNumPlanes_) { > LOG(YUV, Error) << "Invalid number of source planes: " > << source.planes().size(); > return false; > } > - if (destination.numPlanes() != 2) { > + if (destination.numPlanes() != destinationNumPlanes_) { > LOG(YUV, Error) << "Invalid number of destination planes: " > << destination.numPlanes(); > return false; > } > > - for (unsigned int i = 0; i < 2; i++) { > + for (unsigned int i = 0; i < sourceNumPlanes_; i++) { > if (source.planes()[i].length < sourceLength_[i]) { > LOG(YUV, Error) > << "The source planes lengths are too small, " > @@ -112,7 +112,7 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > return false; > } > } > - for (unsigned int i = 0; i < 2; i++) { > + for (unsigned int i = 0; i < destinationNumPlanes_; i++) { > if (destination.plane(i).size() < destinationLength_[i]) { > LOG(YUV, Error) > << "The destination planes lengths are too small, " > @@ -132,18 +132,22 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg, > { > sourceSize_ = inCfg.size; > destinationSize_ = outCfg.size; > + sourceFormat_ = inCfg.pixelFormat; > + destinationFormat_ = outCfg.pixelFormat; > > - const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(formats::NV12); > - for (unsigned int i = 0; i < 2; i++) { > + const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(sourceFormat_); > + sourceNumPlanes_ = sourceInfo.numPlanes(); > + for (unsigned int i = 0; i < sourceInfo.numPlanes(); i++) { Can't you use the just assigned sourceNumPlanes_ ? > sourceStride_[i] = inCfg.stride; > sourceLength_[i] = sourceInfo.planeSize(sourceSize_.height, i, > sourceStride_[i]); > } > > - const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(formats::NV12); > - for (unsigned int i = 0; i < 2; i++) { > - destinationStride_[i] = sourceInfo.stride(destinationSize_.width, i, 1); > - destinationLength_[i] = sourceInfo.planeSize(destinationSize_.height, i, > - destinationStride_[i]); > + const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(destinationFormat_); > + destinationNumPlanes_ = destinationInfo.numPlanes(); > + for (unsigned int i = 0; i < destinationInfo.numPlanes(); i++) { ditto > + destinationStride_[i] = destinationInfo.stride(destinationSize_.width, i, 1); > + destinationLength_[i] = destinationInfo.planeSize(destinationSize_.height, i, > + destinationStride_[i]); > } > } > diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h > index a7ac17c564b6..bfe35f46c6dc 100644 > --- a/src/android/yuv/post_processor_yuv.h > +++ b/src/android/yuv/post_processor_yuv.h > @@ -28,8 +28,12 @@ private: > > libcamera::Size sourceSize_; > libcamera::Size destinationSize_; > - unsigned int sourceLength_[2] = {}; > - unsigned int destinationLength_[2] = {}; > - unsigned int sourceStride_[2] = {}; > - unsigned int destinationStride_[2] = {}; > + libcamera::PixelFormat sourceFormat_; > + libcamera::PixelFormat destinationFormat_; > + unsigned int sourceLength_[3] = {}; > + unsigned int destinationLength_[3] = {}; > + unsigned int sourceStride_[3] = {}; > + unsigned int destinationStride_[3] = {}; > + unsigned int sourceNumPlanes_; > + unsigned int destinationNumPlanes_; The rest looks good. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > }; > > -- > 2.41.0 >
Hi Jacopo, Thank you for your review. On ven., sept. 22, 2023 at 12:58, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > Hi Mattijs > > On Fri, Sep 15, 2023 at 09:57:27AM +0200, Mattijs Korpershoek via libcamera-devel wrote: >> PostProcessorYuv assumes that both source and destination pixel formats >> will always be formats::NV12. >> This might change in the future, for example when doing pixel format >> conversion via libyuv. >> >> Add the necessary plumbing so that other formats can be added easily in >> the future. >> >> Also increase plane number to 3, since some YUV format [1] are >> fully planar (and thus have 3 planes). >> >> [1] https://docs.kernel.org/userspace-api/media/v4l/pixfmt-yuv-planar.html#fully-planar-yuv-formats >> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> >> --- >> src/android/yuv/post_processor_yuv.cpp | 26 +++++++++++++++----------- >> src/android/yuv/post_processor_yuv.h | 12 ++++++++---- >> 2 files changed, 23 insertions(+), 15 deletions(-) >> >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp >> index d58090db14ee..734bb85b7351 100644 >> --- a/src/android/yuv/post_processor_yuv.cpp >> +++ b/src/android/yuv/post_processor_yuv.cpp >> @@ -90,18 +90,18 @@ void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuf >> bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, >> const CameraBuffer &destination) const >> { >> - if (source.planes().size() != 2) { >> + if (source.planes().size() != sourceNumPlanes_) { >> LOG(YUV, Error) << "Invalid number of source planes: " >> << source.planes().size(); >> return false; >> } >> - if (destination.numPlanes() != 2) { >> + if (destination.numPlanes() != destinationNumPlanes_) { >> LOG(YUV, Error) << "Invalid number of destination planes: " >> << destination.numPlanes(); >> return false; >> } >> >> - for (unsigned int i = 0; i < 2; i++) { >> + for (unsigned int i = 0; i < sourceNumPlanes_; i++) { >> if (source.planes()[i].length < sourceLength_[i]) { >> LOG(YUV, Error) >> << "The source planes lengths are too small, " >> @@ -112,7 +112,7 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, >> return false; >> } >> } >> - for (unsigned int i = 0; i < 2; i++) { >> + for (unsigned int i = 0; i < destinationNumPlanes_; i++) { >> if (destination.plane(i).size() < destinationLength_[i]) { >> LOG(YUV, Error) >> << "The destination planes lengths are too small, " >> @@ -132,18 +132,22 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg, >> { >> sourceSize_ = inCfg.size; >> destinationSize_ = outCfg.size; >> + sourceFormat_ = inCfg.pixelFormat; >> + destinationFormat_ = outCfg.pixelFormat; >> >> - const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(formats::NV12); >> - for (unsigned int i = 0; i < 2; i++) { >> + const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(sourceFormat_); >> + sourceNumPlanes_ = sourceInfo.numPlanes(); >> + for (unsigned int i = 0; i < sourceInfo.numPlanes(); i++) { > > Can't you use the just assigned sourceNumPlanes_ ? Will do for v2 > >> sourceStride_[i] = inCfg.stride; >> sourceLength_[i] = sourceInfo.planeSize(sourceSize_.height, i, >> sourceStride_[i]); >> } >> >> - const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(formats::NV12); >> - for (unsigned int i = 0; i < 2; i++) { >> - destinationStride_[i] = sourceInfo.stride(destinationSize_.width, i, 1); >> - destinationLength_[i] = sourceInfo.planeSize(destinationSize_.height, i, >> - destinationStride_[i]); >> + const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(destinationFormat_); >> + destinationNumPlanes_ = destinationInfo.numPlanes(); >> + for (unsigned int i = 0; i < destinationInfo.numPlanes(); i++) { > > ditto Will do for v2 > >> + destinationStride_[i] = destinationInfo.stride(destinationSize_.width, i, 1); >> + destinationLength_[i] = destinationInfo.planeSize(destinationSize_.height, i, >> + destinationStride_[i]); >> } >> } >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h >> index a7ac17c564b6..bfe35f46c6dc 100644 >> --- a/src/android/yuv/post_processor_yuv.h >> +++ b/src/android/yuv/post_processor_yuv.h >> @@ -28,8 +28,12 @@ private: >> >> libcamera::Size sourceSize_; >> libcamera::Size destinationSize_; >> - unsigned int sourceLength_[2] = {}; >> - unsigned int destinationLength_[2] = {}; >> - unsigned int sourceStride_[2] = {}; >> - unsigned int destinationStride_[2] = {}; >> + libcamera::PixelFormat sourceFormat_; >> + libcamera::PixelFormat destinationFormat_; >> + unsigned int sourceLength_[3] = {}; >> + unsigned int destinationLength_[3] = {}; >> + unsigned int sourceStride_[3] = {}; >> + unsigned int destinationStride_[3] = {}; >> + unsigned int sourceNumPlanes_; >> + unsigned int destinationNumPlanes_; > > The rest looks good. > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Will apply your tag > >> }; >> >> -- >> 2.41.0 >>
On Sun, Sep 24, 2023 at 02:18:17PM +0200, Mattijs Korpershoek via libcamera-devel wrote: > On ven., sept. 22, 2023 at 12:58, Jacopo Mondi wrote: > > On Fri, Sep 15, 2023 at 09:57:27AM +0200, Mattijs Korpershoek via libcamera-devel wrote: > >> PostProcessorYuv assumes that both source and destination pixel formats > >> will always be formats::NV12. > >> This might change in the future, for example when doing pixel format s/might/will/ s/for example // > >> conversion via libyuv. If you intend to have two separate paragraphs, you need a blank line between the first and second sentence. Otherwise (which I think is what you intended here), you shouldn't have a line break after the first sentence. > >> > >> Add the necessary plumbing so that other formats can be added easily in > >> the future. > >> > >> Also increase plane number to 3, since some YUV format [1] are > >> fully planar (and thus have 3 planes). > >> > >> [1] https://docs.kernel.org/userspace-api/media/v4l/pixfmt-yuv-planar.html#fully-planar-yuv-formats > >> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > >> --- > >> src/android/yuv/post_processor_yuv.cpp | 26 +++++++++++++++----------- > >> src/android/yuv/post_processor_yuv.h | 12 ++++++++---- > >> 2 files changed, 23 insertions(+), 15 deletions(-) > >> > >> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp > >> index d58090db14ee..734bb85b7351 100644 > >> --- a/src/android/yuv/post_processor_yuv.cpp > >> +++ b/src/android/yuv/post_processor_yuv.cpp > >> @@ -90,18 +90,18 @@ void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuf > >> bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > >> const CameraBuffer &destination) const > >> { > >> - if (source.planes().size() != 2) { > >> + if (source.planes().size() != sourceNumPlanes_) { > >> LOG(YUV, Error) << "Invalid number of source planes: " > >> << source.planes().size(); > >> return false; > >> } > >> - if (destination.numPlanes() != 2) { > >> + if (destination.numPlanes() != destinationNumPlanes_) { > >> LOG(YUV, Error) << "Invalid number of destination planes: " > >> << destination.numPlanes(); > >> return false; > >> } > >> > >> - for (unsigned int i = 0; i < 2; i++) { > >> + for (unsigned int i = 0; i < sourceNumPlanes_; i++) { > >> if (source.planes()[i].length < sourceLength_[i]) { > >> LOG(YUV, Error) > >> << "The source planes lengths are too small, " > >> @@ -112,7 +112,7 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, > >> return false; > >> } > >> } > >> - for (unsigned int i = 0; i < 2; i++) { > >> + for (unsigned int i = 0; i < destinationNumPlanes_; i++) { > >> if (destination.plane(i).size() < destinationLength_[i]) { > >> LOG(YUV, Error) > >> << "The destination planes lengths are too small, " > >> @@ -132,18 +132,22 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg, > >> { > >> sourceSize_ = inCfg.size; > >> destinationSize_ = outCfg.size; > >> + sourceFormat_ = inCfg.pixelFormat; > >> + destinationFormat_ = outCfg.pixelFormat; > >> > >> - const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(formats::NV12); > >> - for (unsigned int i = 0; i < 2; i++) { > >> + const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(sourceFormat_); > >> + sourceNumPlanes_ = sourceInfo.numPlanes(); > >> + for (unsigned int i = 0; i < sourceInfo.numPlanes(); i++) { > > > > Can't you use the just assigned sourceNumPlanes_ ? > > Will do for v2 > > >> sourceStride_[i] = inCfg.stride; > >> sourceLength_[i] = sourceInfo.planeSize(sourceSize_.height, i, > >> sourceStride_[i]); > >> } > >> > >> - const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(formats::NV12); > >> - for (unsigned int i = 0; i < 2; i++) { > >> - destinationStride_[i] = sourceInfo.stride(destinationSize_.width, i, 1); > >> - destinationLength_[i] = sourceInfo.planeSize(destinationSize_.height, i, > >> - destinationStride_[i]); > >> + const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(destinationFormat_); > >> + destinationNumPlanes_ = destinationInfo.numPlanes(); > >> + for (unsigned int i = 0; i < destinationInfo.numPlanes(); i++) { > > > > ditto > > Will do for v2 > > >> + destinationStride_[i] = destinationInfo.stride(destinationSize_.width, i, 1); > >> + destinationLength_[i] = destinationInfo.planeSize(destinationSize_.height, i, > >> + destinationStride_[i]); > >> } > >> } > >> diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h > >> index a7ac17c564b6..bfe35f46c6dc 100644 > >> --- a/src/android/yuv/post_processor_yuv.h > >> +++ b/src/android/yuv/post_processor_yuv.h > >> @@ -28,8 +28,12 @@ private: > >> > >> libcamera::Size sourceSize_; > >> libcamera::Size destinationSize_; > >> - unsigned int sourceLength_[2] = {}; > >> - unsigned int destinationLength_[2] = {}; > >> - unsigned int sourceStride_[2] = {}; > >> - unsigned int destinationStride_[2] = {}; > >> + libcamera::PixelFormat sourceFormat_; > >> + libcamera::PixelFormat destinationFormat_; > >> + unsigned int sourceLength_[3] = {}; > >> + unsigned int destinationLength_[3] = {}; > >> + unsigned int sourceStride_[3] = {}; > >> + unsigned int destinationStride_[3] = {}; > >> + unsigned int sourceNumPlanes_; > >> + unsigned int destinationNumPlanes_; You can use an std::vector<unsigned int> to store sourceLength_, and drop the sourceNumPlanes_ variable. Same for the destination. Actually, I would probably make it struct PlaneLayout { unsigned int length; unsigned int stride; }; std::vector<Plane> sourcePlanes_; std::vector<Plane> destinationPlanes_; (names to be bikeshedded) > > The rest looks good. > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Will apply your tag > > >> }; > >>
diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp index d58090db14ee..734bb85b7351 100644 --- a/src/android/yuv/post_processor_yuv.cpp +++ b/src/android/yuv/post_processor_yuv.cpp @@ -90,18 +90,18 @@ void PostProcessorYuv::process(Camera3RequestDescriptor::StreamBuffer *streamBuf bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, const CameraBuffer &destination) const { - if (source.planes().size() != 2) { + if (source.planes().size() != sourceNumPlanes_) { LOG(YUV, Error) << "Invalid number of source planes: " << source.planes().size(); return false; } - if (destination.numPlanes() != 2) { + if (destination.numPlanes() != destinationNumPlanes_) { LOG(YUV, Error) << "Invalid number of destination planes: " << destination.numPlanes(); return false; } - for (unsigned int i = 0; i < 2; i++) { + for (unsigned int i = 0; i < sourceNumPlanes_; i++) { if (source.planes()[i].length < sourceLength_[i]) { LOG(YUV, Error) << "The source planes lengths are too small, " @@ -112,7 +112,7 @@ bool PostProcessorYuv::isValidBuffers(const FrameBuffer &source, return false; } } - for (unsigned int i = 0; i < 2; i++) { + for (unsigned int i = 0; i < destinationNumPlanes_; i++) { if (destination.plane(i).size() < destinationLength_[i]) { LOG(YUV, Error) << "The destination planes lengths are too small, " @@ -132,18 +132,22 @@ void PostProcessorYuv::calculateLengths(const StreamConfiguration &inCfg, { sourceSize_ = inCfg.size; destinationSize_ = outCfg.size; + sourceFormat_ = inCfg.pixelFormat; + destinationFormat_ = outCfg.pixelFormat; - const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(formats::NV12); - for (unsigned int i = 0; i < 2; i++) { + const PixelFormatInfo &sourceInfo = PixelFormatInfo::info(sourceFormat_); + sourceNumPlanes_ = sourceInfo.numPlanes(); + for (unsigned int i = 0; i < sourceInfo.numPlanes(); i++) { sourceStride_[i] = inCfg.stride; sourceLength_[i] = sourceInfo.planeSize(sourceSize_.height, i, sourceStride_[i]); } - const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(formats::NV12); - for (unsigned int i = 0; i < 2; i++) { - destinationStride_[i] = sourceInfo.stride(destinationSize_.width, i, 1); - destinationLength_[i] = sourceInfo.planeSize(destinationSize_.height, i, - destinationStride_[i]); + const PixelFormatInfo &destinationInfo = PixelFormatInfo::info(destinationFormat_); + destinationNumPlanes_ = destinationInfo.numPlanes(); + for (unsigned int i = 0; i < destinationInfo.numPlanes(); i++) { + destinationStride_[i] = destinationInfo.stride(destinationSize_.width, i, 1); + destinationLength_[i] = destinationInfo.planeSize(destinationSize_.height, i, + destinationStride_[i]); } } diff --git a/src/android/yuv/post_processor_yuv.h b/src/android/yuv/post_processor_yuv.h index a7ac17c564b6..bfe35f46c6dc 100644 --- a/src/android/yuv/post_processor_yuv.h +++ b/src/android/yuv/post_processor_yuv.h @@ -28,8 +28,12 @@ private: libcamera::Size sourceSize_; libcamera::Size destinationSize_; - unsigned int sourceLength_[2] = {}; - unsigned int destinationLength_[2] = {}; - unsigned int sourceStride_[2] = {}; - unsigned int destinationStride_[2] = {}; + libcamera::PixelFormat sourceFormat_; + libcamera::PixelFormat destinationFormat_; + unsigned int sourceLength_[3] = {}; + unsigned int destinationLength_[3] = {}; + unsigned int sourceStride_[3] = {}; + unsigned int destinationStride_[3] = {}; + unsigned int sourceNumPlanes_; + unsigned int destinationNumPlanes_; };
PostProcessorYuv assumes that both source and destination pixel formats will always be formats::NV12. This might change in the future, for example when doing pixel format conversion via libyuv. Add the necessary plumbing so that other formats can be added easily in the future. Also increase plane number to 3, since some YUV format [1] are fully planar (and thus have 3 planes). [1] https://docs.kernel.org/userspace-api/media/v4l/pixfmt-yuv-planar.html#fully-planar-yuv-formats Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> --- src/android/yuv/post_processor_yuv.cpp | 26 +++++++++++++++----------- src/android/yuv/post_processor_yuv.h | 12 ++++++++---- 2 files changed, 23 insertions(+), 15 deletions(-)