| Message ID | 20210908062316.49466-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, thank you for the patch. On Wed, Sep 8, 2021 at 3:23 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > After multi-planar support was introduced for jpeg encoding as well, > EncoderLibJpeg::encode() expects a vector of planes as the source of > framebuffer to be encoded. Currently, this is broken for thumbnail > encoding as the thumbnail generated is directly passed as a single > plane. > > Hence, piece the thumbnail data as planes if the parent Framebuffer is > multi-planar. This fixes the encoding of the corresponding thumbnail. > > Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 68d74842..d896581f 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > int ret = thumbnailEncoder_.configure(thCfg); > > if (!rawThumbnail.empty() && !ret) { > - /* > - * \todo Avoid value-initialization of all elements of the > - * vector. > - */ > - thumbnail->resize(rawThumbnail.size()); > + std::vector<Span<uint8_t>> thumbnailPlanes; > + for (int i = 0; i < source.planes().size(); i++) { > + unsigned int thumbnailSize = > + targetSize.height * targetSize.width; > + Span<uint8_t> plane{ > + rawThumbnail.data() + (i * thumbnailSize), > + thumbnailSize This is not correct. Since rawThumbanil is NV12, the second plane size is a half of the first plane size. > + }; > + thumbnailPlanes.push_back(plane); > + } Uhm, I would avoid introducing the dependency how rawThumbnail is created in thumbnailer_. Since this will be resolved by my patch series [1] later, I am fine with this as a temporary solution. By the way, this is related to Launrent's change, should this be a part of the series? +Laurent Pinchart for visibility. [1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423 -Hiro > > - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > *thumbnail, {}, quality); > thumbnail->resize(jpeg_size); > > -- > 2.31.1 >
Hi Hiro, On 9/8/21 1:20 PM, Hirokazu Honda wrote: > Hi Umang, thank you for the patch. > > On Wed, Sep 8, 2021 at 3:23 PM Umang Jain <umang.jain@ideasonboard.com> wrote: >> After multi-planar support was introduced for jpeg encoding as well, >> EncoderLibJpeg::encode() expects a vector of planes as the source of >> framebuffer to be encoded. Currently, this is broken for thumbnail >> encoding as the thumbnail generated is directly passed as a single >> plane. >> >> Hence, piece the thumbnail data as planes if the parent Framebuffer is >> multi-planar. This fixes the encoding of the corresponding thumbnail. >> >> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >> index 68d74842..d896581f 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, >> int ret = thumbnailEncoder_.configure(thCfg); >> >> if (!rawThumbnail.empty() && !ret) { >> - /* >> - * \todo Avoid value-initialization of all elements of the >> - * vector. >> - */ >> - thumbnail->resize(rawThumbnail.size()); >> + std::vector<Span<uint8_t>> thumbnailPlanes; >> + for (int i = 0; i < source.planes().size(); i++) { >> + unsigned int thumbnailSize = >> + targetSize.height * targetSize.width; >> + Span<uint8_t> plane{ >> + rawThumbnail.data() + (i * thumbnailSize), >> + thumbnailSize > This is not correct. Since rawThumbanil is NV12, the second plane size > is a half of the first plane size. Oh oops, yes you are right. Forgot to take into consideration :( Will need to think more a bit hmm! >> + }; >> + thumbnailPlanes.push_back(plane); >> + } > Uhm, I would avoid introducing the dependency how rawThumbnail is > created in thumbnailer_. > Since this will be resolved by my patch series [1] later, I am fine > with this as a temporary solution. > By the way, this is related to Launrent's change, should this be a > part of the series? The series is merged as far as I can see. And thumbnailing is broken on master. So, we need to fix it > > +Laurent Pinchart for visibility. > > [1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423 > > -Hiro >> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, >> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, >> *thumbnail, {}, quality); >> thumbnail->resize(jpeg_size); >> >> -- >> 2.31.1 >>
Hi Umang, On Wed, Sep 8, 2021 at 4:54 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Hiro, > > On 9/8/21 1:20 PM, Hirokazu Honda wrote: > > Hi Umang, thank you for the patch. > > > > On Wed, Sep 8, 2021 at 3:23 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > >> After multi-planar support was introduced for jpeg encoding as well, > >> EncoderLibJpeg::encode() expects a vector of planes as the source of > >> framebuffer to be encoded. Currently, this is broken for thumbnail > >> encoding as the thumbnail generated is directly passed as a single > >> plane. > >> > >> Hence, piece the thumbnail data as planes if the parent Framebuffer is > >> multi-planar. This fixes the encoding of the corresponding thumbnail. > >> > >> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ > >> 1 file changed, 11 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > >> index 68d74842..d896581f 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.cpp > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > >> int ret = thumbnailEncoder_.configure(thCfg); > >> > >> if (!rawThumbnail.empty() && !ret) { > >> - /* > >> - * \todo Avoid value-initialization of all elements of the > >> - * vector. > >> - */ > >> - thumbnail->resize(rawThumbnail.size()); > >> + std::vector<Span<uint8_t>> thumbnailPlanes; > >> + for (int i = 0; i < source.planes().size(); i++) { > >> + unsigned int thumbnailSize = > >> + targetSize.height * targetSize.width; > >> + Span<uint8_t> plane{ > >> + rawThumbnail.data() + (i * thumbnailSize), > >> + thumbnailSize > > This is not correct. Since rawThumbanil is NV12, the second plane size > > is a half of the first plane size. > > > Oh oops, yes you are right. Forgot to take into consideration :( > > Will need to think more a bit hmm! > > >> + }; > >> + thumbnailPlanes.push_back(plane); > >> + } > > Uhm, I would avoid introducing the dependency how rawThumbnail is > > created in thumbnailer_. > > Since this will be resolved by my patch series [1] later, I am fine > > with this as a temporary solution. > > By the way, this is related to Launrent's change, should this be a > > part of the series? > > > The series is merged as far as I can see. And thumbnailing is broken on > master. So, we need to fix it > Oh I didn't realize it. :-) Let's go ahead then. -Hiro > > > > +Laurent Pinchart for visibility. > > > > [1] https://patchwork.libcamera.org/project/libcamera/list/?series=2423 > > > > -Hiro > >> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > >> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > >> *thumbnail, {}, quality); > >> thumbnail->resize(jpeg_size); > >> > >> -- > >> 2.31.1 > >>
Hi Umang On Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote: > After multi-planar support was introduced for jpeg encoding as well, > EncoderLibJpeg::encode() expects a vector of planes as the source of > framebuffer to be encoded. Currently, this is broken for thumbnail > encoding as the thumbnail generated is directly passed as a single > plane. > > Hence, piece the thumbnail data as planes if the parent Framebuffer is Did you mean 'place', as in 'to place' as a verb ? > multi-planar. This fixes the encoding of the corresponding thumbnail. > > Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 68d74842..d896581f 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > int ret = thumbnailEncoder_.configure(thCfg); > > if (!rawThumbnail.empty() && !ret) { > - /* > - * \todo Avoid value-initialization of all elements of the > - * vector. > - */ > - thumbnail->resize(rawThumbnail.size()); > + std::vector<Span<uint8_t>> thumbnailPlanes; > + for (int i = 0; i < source.planes().size(); i++) { > + unsigned int thumbnailSize = > + targetSize.height * targetSize.width; > + Span<uint8_t> plane{ > + rawThumbnail.data() + (i * thumbnailSize), > + thumbnailSize > + }; So, I never really understood the thumbnailer bits but this looks surprising to me. Are all the planes of the same size ? Aren't we assuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr plane be half in size ? So, I haven't followed much the fallout of the multi-planar support but what I see here is that now the libjpeg encoder requires the Y and CbCr planes to be stored in different 'planes()' so I understand you have to artificially create those planes as a consequence of the fact that createThumbnail() scales-down the image into a single span of chars /* Stores the raw scaled-down thumbnail bytes. */ std::vector<unsigned char> rawThumbnail; thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); Wouldn't it be better to: /* Stores the raw scaled-down thumbnail bytes. */ std::vector<Span<unsigned char>> rawThumbnail; thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); And have the raw thumbnail already displaced in two planes ? The code seems to be there dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2]; dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2]; You just need to point dstC to the second plane if I'm not mistaken. Thanks j > + thumbnailPlanes.push_back(plane); > + } > > - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > *thumbnail, {}, quality); > thumbnail->resize(jpeg_size); > > -- > 2.31.1 >
Hi Jacopo, On Wed, Sep 08, 2021 at 10:05:03AM +0200, Jacopo Mondi wrote: > On Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote: > > After multi-planar support was introduced for jpeg encoding as well, > > EncoderLibJpeg::encode() expects a vector of planes as the source of > > framebuffer to be encoded. Currently, this is broken for thumbnail > > encoding as the thumbnail generated is directly passed as a single > > plane. > > > > Hence, piece the thumbnail data as planes if the parent Framebuffer is > > Did you mean 'place', as in 'to place' as a verb ? > > > multi-planar. This fixes the encoding of the corresponding thumbnail. > > > > Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > > index 68d74842..d896581f 100644 > > --- a/src/android/jpeg/post_processor_jpeg.cpp > > +++ b/src/android/jpeg/post_processor_jpeg.cpp > > @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > > int ret = thumbnailEncoder_.configure(thCfg); > > > > if (!rawThumbnail.empty() && !ret) { > > - /* > > - * \todo Avoid value-initialization of all elements of the > > - * vector. > > - */ > > - thumbnail->resize(rawThumbnail.size()); > > + std::vector<Span<uint8_t>> thumbnailPlanes; > > + for (int i = 0; i < source.planes().size(); i++) { > > + unsigned int thumbnailSize = > > + targetSize.height * targetSize.width; > > + Span<uint8_t> plane{ > > + rawThumbnail.data() + (i * thumbnailSize), > > + thumbnailSize > > + }; > > So, I never really understood the thumbnailer bits but this looks > surprising to me. Are all the planes of the same size ? Aren't we > assuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr > plane be half in size ? You're right. Hiro has reported the same :-) > So, I haven't followed much the fallout of the multi-planar support > but what I see here is that now the libjpeg encoder requires the > Y and CbCr planes to be stored in different 'planes()' so I > understand you have to artificially create those planes as a > consequence of the fact that createThumbnail() scales-down the image > into a single span of chars > > /* Stores the raw scaled-down thumbnail bytes. */ > std::vector<unsigned char> rawThumbnail; > thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); > > Wouldn't it be better to: > /* Stores the raw scaled-down thumbnail bytes. */ > std::vector<Span<unsigned char>> rawThumbnail; > thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); > > And have the raw thumbnail already displaced in two planes ? The NV12 thumbnail doesn't have to be stored in two separate memory buffers, it's just that the encoder now needs to be told where the two planes are, even if they're contiguous. We could split the planes, but I'm not sure it's worth it. I'd go for the simplest solution for now, it will be cleaned up once we get an Image class in libcamera. > The code seems to be there > > dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2]; > dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2]; > > You just need to point dstC to the second plane if I'm not mistaken. > > > + thumbnailPlanes.push_back(plane); > > + } > > > > - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > > + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > > *thumbnail, {}, quality); > > thumbnail->resize(jpeg_size); > >
Hi Jacopo On 9/8/21 1:35 PM, Jacopo Mondi wrote: > Hi Umang > > On Wed, Sep 08, 2021 at 11:53:16AM +0530, Umang Jain wrote: >> After multi-planar support was introduced for jpeg encoding as well, >> EncoderLibJpeg::encode() expects a vector of planes as the source of >> framebuffer to be encoded. Currently, this is broken for thumbnail >> encoding as the thumbnail generated is directly passed as a single >> plane. >> >> Hence, piece the thumbnail data as planes if the parent Framebuffer is > Did you mean 'place', as in 'to place' as a verb ? place? I wrote 'piece'. I was going to write 'chop', okay, nevermind... > >> multi-planar. This fixes the encoding of the corresponding thumbnail. >> >> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >> index 68d74842..d896581f 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, >> int ret = thumbnailEncoder_.configure(thCfg); >> >> if (!rawThumbnail.empty() && !ret) { >> - /* >> - * \todo Avoid value-initialization of all elements of the >> - * vector. >> - */ >> - thumbnail->resize(rawThumbnail.size()); >> + std::vector<Span<uint8_t>> thumbnailPlanes; >> + for (int i = 0; i < source.planes().size(); i++) { >> + unsigned int thumbnailSize = >> + targetSize.height * targetSize.width; >> + Span<uint8_t> plane{ >> + rawThumbnail.data() + (i * thumbnailSize), >> + thumbnailSize >> + }; > So, I never really understood the thumbnailer bits but this looks > surprising to me. Are all the planes of the same size ? Aren't we > assuming NV12 as the format of the rawThumbnail ? Shouldn't the CbCr > plane be half in size ? > > So, I haven't followed much the fallout of the multi-planar support > but what I see here is that now the libjpeg encoder requires the > Y and CbCr planes to be stored in different 'planes()' so I > understand you have to artificially create those planes as a > consequence of the fact that createThumbnail() scales-down the image > into a single span of chars > > /* Stores the raw scaled-down thumbnail bytes. */ > std::vector<unsigned char> rawThumbnail; > thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); > > Wouldn't it be better to: > /* Stores the raw scaled-down thumbnail bytes. */ > std::vector<Span<unsigned char>> rawThumbnail; > thumbnailer_.createThumbnail(source, targetSize, &rawThumbnail); Yes this is definitely better. I would also need to ensure correct Span is used to store the scaled down bits - which will probably change quite a bit of pointer arithmetic in the Thumbnailer code. It's not hard to do per say, but I guess Laurent is in favor of a quick and simpler patch for now (and I too). I have added your suggestion as a \todo though in v2, so the idea doesn't gets lost in future. > > And have the raw thumbnail already displaced in two planes ? > The code seems to be there > > dstC[(y / 2) * tw + x + 0] = srcCb[(sourceX / 2) * 2]; > dstC[(y / 2) * tw + x + 1] = srcCr[(sourceX / 2) * 2]; > > You just need to point dstC to the second plane if I'm not mistaken. > > Thanks > j > > > >> + thumbnailPlanes.push_back(plane); >> + } >> >> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, >> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, >> *thumbnail, {}, quality); >> thumbnail->resize(jpeg_size); >> >> -- >> 2.31.1 >>
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 68d74842..d896581f 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -66,13 +66,18 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, int ret = thumbnailEncoder_.configure(thCfg); if (!rawThumbnail.empty() && !ret) { - /* - * \todo Avoid value-initialization of all elements of the - * vector. - */ - thumbnail->resize(rawThumbnail.size()); + std::vector<Span<uint8_t>> thumbnailPlanes; + for (int i = 0; i < source.planes().size(); i++) { + unsigned int thumbnailSize = + targetSize.height * targetSize.width; + Span<uint8_t> plane{ + rawThumbnail.data() + (i * thumbnailSize), + thumbnailSize + }; + thumbnailPlanes.push_back(plane); + } - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, *thumbnail, {}, quality); thumbnail->resize(jpeg_size);
After multi-planar support was introduced for jpeg encoding as well, EncoderLibJpeg::encode() expects a vector of planes as the source of framebuffer to be encoded. Currently, this is broken for thumbnail encoding as the thumbnail generated is directly passed as a single plane. Hence, piece the thumbnail data as planes if the parent Framebuffer is multi-planar. This fixes the encoding of the corresponding thumbnail. Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/android/jpeg/post_processor_jpeg.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)