| Message ID | 20210908094949.111140-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 6:50 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, we are passing a contiguous buffer > which is treated as only one plane (instead of two, as thumbnail is NV12). > > Hence, split the thumbnail data into respective planes according to NV12. > This fixes a crash in encoding of thumbnails. > > Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes in v2: > - Fix basic split logic error. > - Add Jacopo's suggestion as a \todo since it's a superior method to fix > this. > --- > src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 68d74842..1bf507a4 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > int ret = thumbnailEncoder_.configure(thCfg); > > if (!rawThumbnail.empty() && !ret) { > + thumbnail->resize(rawThumbnail.size()); > + > /* > - * \todo Avoid value-initialization of all elements of the > - * vector. I think this comment should not be removed. > + * 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. > */ > - thumbnail->resize(rawThumbnail.size()); > + std::vector<Span<uint8_t>> thumbnailPlanes; > + unsigned int thumbnailSize = targetSize.height * targetSize.width; > + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); > + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, > + thumbnailSize / 2 }); Shall PixelFormatInfo::planeSize() be used? With nits, Reviewed-by: Hirokazu Honda <hiroh@chromium.org> -Hiro > > - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > *thumbnail, {}, quality); > thumbnail->resize(jpeg_size); > > -- > 2.31.0 >
Hi Hiro? On 9/8/21 3:32 PM, Hirokazu Honda wrote: > Hi Umang, thank you for the patch. > > On Wed, Sep 8, 2021 at 6:50 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, we are passing a contiguous buffer >> which is treated as only one plane (instead of two, as thumbnail is NV12). >> >> Hence, split the thumbnail data into respective planes according to NV12. >> This fixes a crash in encoding of thumbnails. >> >> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> Changes in v2: >> - Fix basic split logic error. >> - Add Jacopo's suggestion as a \todo since it's a superior method to fix >> this. >> --- >> src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >> index 68d74842..1bf507a4 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, >> int ret = thumbnailEncoder_.configure(thCfg); >> >> if (!rawThumbnail.empty() && !ret) { >> + thumbnail->resize(rawThumbnail.size()); >> + >> /* >> - * \todo Avoid value-initialization of all elements of the >> - * vector. > I think this comment should not be removed. > >> + * 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. >> */ >> - thumbnail->resize(rawThumbnail.size()); >> + std::vector<Span<uint8_t>> thumbnailPlanes; >> + unsigned int thumbnailSize = targetSize.height * targetSize.width; >> + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); >> + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, >> + thumbnailSize / 2 }); > Shall PixelFormatInfo::planeSize() be used? Used for what? The " / 2" part? Also, Did you mean PixelFormatInfo::numPlanes() instead? I won't worry too much here, as the Thumbnail class is tightly bound to NV12. So that planes are constant. See Thumbnailer::configure When we add support for more diverse thumbnailing for formats, I expect this to be changed in a major way. > > With nits, > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > -Hiro >> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, >> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, >> *thumbnail, {}, quality); >> thumbnail->resize(jpeg_size); >> >> -- >> 2.31.0 >>
Hi Umang, On Thu, Sep 9, 2021 at 3:38 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi Hiro? > > On 9/8/21 3:32 PM, Hirokazu Honda wrote: > > Hi Umang, thank you for the patch. > > > > On Wed, Sep 8, 2021 at 6:50 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, we are passing a contiguous buffer > >> which is treated as only one plane (instead of two, as thumbnail is NV12). > >> > >> Hence, split the thumbnail data into respective planes according to NV12. > >> This fixes a crash in encoding of thumbnails. > >> > >> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> Changes in v2: > >> - Fix basic split logic error. > >> - Add Jacopo's suggestion as a \todo since it's a superior method to fix > >> this. > >> --- > >> src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- > >> 1 file changed, 14 insertions(+), 4 deletions(-) > >> > >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > >> index 68d74842..1bf507a4 100644 > >> --- a/src/android/jpeg/post_processor_jpeg.cpp > >> +++ b/src/android/jpeg/post_processor_jpeg.cpp > >> @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > >> int ret = thumbnailEncoder_.configure(thCfg); > >> > >> if (!rawThumbnail.empty() && !ret) { > >> + thumbnail->resize(rawThumbnail.size()); > >> + > >> /* > >> - * \todo Avoid value-initialization of all elements of the > >> - * vector. > > I think this comment should not be removed. > > > >> + * 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. > >> */ > >> - thumbnail->resize(rawThumbnail.size()); > >> + std::vector<Span<uint8_t>> thumbnailPlanes; > >> + unsigned int thumbnailSize = targetSize.height * targetSize.width; > >> + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); > >> + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, > >> + thumbnailSize / 2 }); > > Shall PixelFormatInfo::planeSize() be used? > > > Used for what? The " / 2" part? > > Also, Did you mean PixelFormatInfo::numPlanes() instead? I won't worry > too much here, as the Thumbnail class is tightly bound to NV12. So that > planes are constant. See Thumbnailer::configure > > When we add support for more diverse thumbnailing for formats, I expect > this to be changed in a major way. > I would size_t YPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 0); size_t UVPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 1); thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize }); thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize }); I don't have a strong opinion. Up to you. -Hiro > > > > > With nits, > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > -Hiro > >> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > >> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > >> *thumbnail, {}, quality); > >> thumbnail->resize(jpeg_size); > >> > >> -- > >> 2.31.0 > >>
Hi Hiro, On 9/9/21 12:26 PM, Hirokazu Honda wrote: > Hi Umang, > > On Thu, Sep 9, 2021 at 3:38 PM Umang Jain <umang.jain@ideasonboard.com> wrote: >> Hi Hiro? >> >> On 9/8/21 3:32 PM, Hirokazu Honda wrote: >>> Hi Umang, thank you for the patch. >>> >>> On Wed, Sep 8, 2021 at 6:50 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, we are passing a contiguous buffer >>>> which is treated as only one plane (instead of two, as thumbnail is NV12). >>>> >>>> Hence, split the thumbnail data into respective planes according to NV12. >>>> This fixes a crash in encoding of thumbnails. >>>> >>>> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") >>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>> --- >>>> Changes in v2: >>>> - Fix basic split logic error. >>>> - Add Jacopo's suggestion as a \todo since it's a superior method to fix >>>> this. >>>> --- >>>> src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- >>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >>>> index 68d74842..1bf507a4 100644 >>>> --- a/src/android/jpeg/post_processor_jpeg.cpp >>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp >>>> @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, >>>> int ret = thumbnailEncoder_.configure(thCfg); >>>> >>>> if (!rawThumbnail.empty() && !ret) { >>>> + thumbnail->resize(rawThumbnail.size()); >>>> + >>>> /* >>>> - * \todo Avoid value-initialization of all elements of the >>>> - * vector. >>> I think this comment should not be removed. >>> >>>> + * 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. >>>> */ >>>> - thumbnail->resize(rawThumbnail.size()); >>>> + std::vector<Span<uint8_t>> thumbnailPlanes; >>>> + unsigned int thumbnailSize = targetSize.height * targetSize.width; >>>> + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); >>>> + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, >>>> + thumbnailSize / 2 }); >>> Shall PixelFormatInfo::planeSize() be used? >> >> Used for what? The " / 2" part? >> >> Also, Did you mean PixelFormatInfo::numPlanes() instead? I won't worry >> too much here, as the Thumbnail class is tightly bound to NV12. So that >> planes are constant. See Thumbnailer::configure >> >> When we add support for more diverse thumbnailing for formats, I expect >> this to be changed in a major way. >> > I would > size_t YPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 0); > size_t UVPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 1); > thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize }); > thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize }); Ok, I see. These helpers are quite recent, and I wasn't that familiar with these helpers. These are more readable, I think. I'll wait for more reviews to pitch in meanwhile. > > I don't have a strong opinion. Up to you. > -Hiro >>> With nits, >>> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> >>> >>> -Hiro >>>> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, >>>> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, >>>> *thumbnail, {}, quality); >>>> thumbnail->resize(jpeg_size); >>>> >>>> -- >>>> 2.31.0 >>>>
Hi Umang, On Thu, Sep 09, 2021 at 12:54:18PM +0530, Umang Jain wrote: > Hi Hiro, > > On 9/9/21 12:26 PM, Hirokazu Honda wrote: > > Hi Umang, > > > > On Thu, Sep 9, 2021 at 3:38 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > > Hi Hiro? > > > > > > On 9/8/21 3:32 PM, Hirokazu Honda wrote: > > > > Hi Umang, thank you for the patch. > > > > > > > > On Wed, Sep 8, 2021 at 6:50 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, we are passing a contiguous buffer > > > > > which is treated as only one plane (instead of two, as thumbnail is NV12). > > > > > > > > > > Hence, split the thumbnail data into respective planes according to NV12. > > > > > This fixes a crash in encoding of thumbnails. > > > > > > > > > > Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > > > --- > > > > > Changes in v2: > > > > > - Fix basic split logic error. > > > > > - Add Jacopo's suggestion as a \todo since it's a superior method to fix > > > > > this. > > > > > --- > > > > > src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- > > > > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > > > > > index 68d74842..1bf507a4 100644 > > > > > --- a/src/android/jpeg/post_processor_jpeg.cpp > > > > > +++ b/src/android/jpeg/post_processor_jpeg.cpp > > > > > @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, > > > > > int ret = thumbnailEncoder_.configure(thCfg); > > > > > > > > > > if (!rawThumbnail.empty() && !ret) { > > > > > + thumbnail->resize(rawThumbnail.size()); > > > > > + > > > > > /* > > > > > - * \todo Avoid value-initialization of all elements of the > > > > > - * vector. > > > > I think this comment should not be removed. But please move it below the following paragraph to keep todo items at the end of the comment block > > > > > > > > > + * 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. > > > > > */ > > > > > - thumbnail->resize(rawThumbnail.size()); > > > > > + std::vector<Span<uint8_t>> thumbnailPlanes; > > > > > + unsigned int thumbnailSize = targetSize.height * targetSize.width; > > > > > + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); > > > > > + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, > > > > > + thumbnailSize / 2 }); > > > > Shall PixelFormatInfo::planeSize() be used? > > > > > > Used for what? The " / 2" part? > > > > > > Also, Did you mean PixelFormatInfo::numPlanes() instead? I won't worry > > > too much here, as the Thumbnail class is tightly bound to NV12. So that > > > planes are constant. See Thumbnailer::configure > > > > > > When we add support for more diverse thumbnailing for formats, I expect > > > this to be changed in a major way. > > > > > I would > > size_t YPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 0); > > size_t UVPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 1); > > thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize }); > > thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize }); > > Ok, I see. These helpers are quite recent, and I wasn't that familiar with > these helpers. > > These are more readable, I think. I'll wait for more reviews to pitch in > meanwhile. Whatever you decide to use: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > > > > I don't have a strong opinion. Up to you. > > -Hiro > > > > With nits, > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > -Hiro > > > > > - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, > > > > > + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, > > > > > *thumbnail, {}, quality); > > > > > thumbnail->resize(jpeg_size); > > > > > > > > > > -- > > > > > 2.31.0 > > > > >
Hi Jacopo, On 9/9/21 3:39 PM, Jacopo Mondi wrote: > Hi Umang, > > On Thu, Sep 09, 2021 at 12:54:18PM +0530, Umang Jain wrote: >> Hi Hiro, >> >> On 9/9/21 12:26 PM, Hirokazu Honda wrote: >>> Hi Umang, >>> >>> On Thu, Sep 9, 2021 at 3:38 PM Umang Jain <umang.jain@ideasonboard.com> wrote: >>>> Hi Hiro? >>>> >>>> On 9/8/21 3:32 PM, Hirokazu Honda wrote: >>>>> Hi Umang, thank you for the patch. >>>>> >>>>> On Wed, Sep 8, 2021 at 6:50 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, we are passing a contiguous buffer >>>>>> which is treated as only one plane (instead of two, as thumbnail is NV12). >>>>>> >>>>>> Hence, split the thumbnail data into respective planes according to NV12. >>>>>> This fixes a crash in encoding of thumbnails. >>>>>> >>>>>> Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>>> --- >>>>>> Changes in v2: >>>>>> - Fix basic split logic error. >>>>>> - Add Jacopo's suggestion as a \todo since it's a superior method to fix >>>>>> this. >>>>>> --- >>>>>> src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- >>>>>> 1 file changed, 14 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp >>>>>> index 68d74842..1bf507a4 100644 >>>>>> --- a/src/android/jpeg/post_processor_jpeg.cpp >>>>>> +++ b/src/android/jpeg/post_processor_jpeg.cpp >>>>>> @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, >>>>>> int ret = thumbnailEncoder_.configure(thCfg); >>>>>> >>>>>> if (!rawThumbnail.empty() && !ret) { >>>>>> + thumbnail->resize(rawThumbnail.size()); >>>>>> + >>>>>> /* >>>>>> - * \todo Avoid value-initialization of all elements of the >>>>>> - * vector. >>>>> I think this comment should not be removed. > But please move it below the following paragraph to keep todo items at > the end of the comment block I am going to undo the deletion so it will stay as is. I'll make sure any \todos are at end of comment block > >>>>>> + * 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. >>>>>> */ >>>>>> - thumbnail->resize(rawThumbnail.size()); >>>>>> + std::vector<Span<uint8_t>> thumbnailPlanes; >>>>>> + unsigned int thumbnailSize = targetSize.height * targetSize.width; >>>>>> + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); >>>>>> + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, >>>>>> + thumbnailSize / 2 }); >>>>> Shall PixelFormatInfo::planeSize() be used? >>>> Used for what? The " / 2" part? >>>> >>>> Also, Did you mean PixelFormatInfo::numPlanes() instead? I won't worry >>>> too much here, as the Thumbnail class is tightly bound to NV12. So that >>>> planes are constant. See Thumbnailer::configure >>>> >>>> When we add support for more diverse thumbnailing for formats, I expect >>>> this to be changed in a major way. >>>> >>> I would >>> size_t YPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 0); >>> size_t UVPlaneSize = PixelFormatInfo(formats::NV12)::planeSize(targetSize, 1); >>> thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize }); >>> thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize }); >> Ok, I see. These helpers are quite recent, and I wasn't that familiar with >> these helpers. >> >> These are more readable, I think. I'll wait for more reviews to pitch in >> meanwhile. > Whatever you decide to use: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks, I'll apply Hiro's suggestion and test once before pushing to master. This is slightly urgent patch to merge since CTS will fail to run without this. So I'll merge it in a few minutes. > > Thanks > j >> >>> I don't have a strong opinion. Up to you. >>> -Hiro >>>>> With nits, >>>>> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> >>>>> >>>>> -Hiro >>>>>> - int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail }, >>>>>> + int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes, >>>>>> *thumbnail, {}, quality); >>>>>> thumbnail->resize(jpeg_size); >>>>>> >>>>>> -- >>>>>> 2.31.0 >>>>>>
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 68d74842..1bf507a4 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -66,13 +66,23 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source, int ret = thumbnailEncoder_.configure(thCfg); if (!rawThumbnail.empty() && !ret) { + thumbnail->resize(rawThumbnail.size()); + /* - * \todo Avoid value-initialization of all elements of the - * vector. + * 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. */ - thumbnail->resize(rawThumbnail.size()); + std::vector<Span<uint8_t>> thumbnailPlanes; + unsigned int thumbnailSize = targetSize.height * targetSize.width; + thumbnailPlanes.push_back({ rawThumbnail.data(), thumbnailSize }); + thumbnailPlanes.push_back({ rawThumbnail.data() + thumbnailSize, + thumbnailSize / 2 }); - 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, we are passing a contiguous buffer which is treated as only one plane (instead of two, as thumbnail is NV12). Hence, split the thumbnail data into respective planes according to NV12. This fixes a crash in encoding of thumbnails. Fixes: 894ca69f6043("android: jpeg: Support multi-planar buffers") Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- Changes in v2: - Fix basic split logic error. - Add Jacopo's suggestion as a \todo since it's a superior method to fix this. --- src/android/jpeg/post_processor_jpeg.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)