[libcamera-devel,v3] android: jpeg: Split and pass the thumbnail planes to encoder
diff mbox series

Message ID 20210909154857.299746-1-umang.jain@ideasonboard.com
State Accepted
Delegated to: Umang Jain
Headers show
Series
  • [libcamera-devel,v3] android: jpeg: Split and pass the thumbnail planes to encoder
Related show

Commit Message

Umang Jain Sept. 9, 2021, 3:48 p.m. UTC 
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>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes in v3:
- Use planeSize() helpers recently introduced for readability.
---
 src/android/jpeg/post_processor_jpeg.cpp | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Paul Elder Sept. 10, 2021, 12:49 a.m. UTC | #1 
Hi Umang,

On Thu, Sep 09, 2021 at 09:18:57PM +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, 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>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes in v3:
> - Use planeSize() helpers recently introduced for readability.
> ---
>  src/android/jpeg/post_processor_jpeg.cpp | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 68d74842..ef2d98cc 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -72,7 +72,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>  		 */
>  		thumbnail->resize(rawThumbnail.size());
>  
> -		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
> +		/*
> +		 * 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 jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
>  							 *thumbnail, {}, quality);
>  		thumbnail->resize(jpeg_size);
>  
> -- 
> 2.31.0
>
Laurent Pinchart Sept. 10, 2021, 4:06 a.m. UTC | #2 
Hi Umang,

On Thu, Sep 09, 2021 at 09:18:57PM +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, 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>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> Changes in v3:
> - Use planeSize() helpers recently introduced for readability.
> ---
>  src/android/jpeg/post_processor_jpeg.cpp | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 68d74842..ef2d98cc 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -72,7 +72,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>  		 */
>  		thumbnail->resize(rawThumbnail.size());
>  
> -		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
> +		/*
> +		 * 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);

These should be yPlaneSize and uvPlaneSize.

> +		thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize });
> +		thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize });
> +
> +		int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
>  							 *thumbnail, {}, quality);
>  		thumbnail->resize(jpeg_size);
>
Umang Jain Sept. 10, 2021, 4:15 a.m. UTC | #3 
Hi Laurent,

On 9/10/21 9:36 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Thu, Sep 09, 2021 at 09:18:57PM +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, 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>
>> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>> Changes in v3:
>> - Use planeSize() helpers recently introduced for readability.
>> ---
>>   src/android/jpeg/post_processor_jpeg.cpp | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index 68d74842..ef2d98cc 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -72,7 +72,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>>   		 */
>>   		thumbnail->resize(rawThumbnail.size());
>>   
>> -		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
>> +		/*
>> +		 * 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);
> These should be yPlaneSize and uvPlaneSize.


oh yeah :-/

I had pushed the patch last night though. Fixup on top?

>
>> +		thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize });
>> +		thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize });
>> +
>> +		int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
>>   							 *thumbnail, {}, quality);
>>   		thumbnail->resize(jpeg_size);
>>
Laurent Pinchart Sept. 10, 2021, 4:23 a.m. UTC | #4 
Hi Umang,

On Fri, Sep 10, 2021 at 09:45:46AM +0530, Umang Jain wrote:
> On 9/10/21 9:36 AM, Laurent Pinchart wrote:
> > On Thu, Sep 09, 2021 at 09:18:57PM +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, 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>
> >> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >> Changes in v3:
> >> - Use planeSize() helpers recently introduced for readability.
> >> ---
> >>   src/android/jpeg/post_processor_jpeg.cpp | 17 ++++++++++++++++-
> >>   1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> index 68d74842..ef2d98cc 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -72,7 +72,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >>   		 */
> >>   		thumbnail->resize(rawThumbnail.size());
> >>   
> >> -		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
> >> +		/*
> >> +		 * 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);
> > These should be yPlaneSize and uvPlaneSize.
> 
> oh yeah :-/
> 
> I had pushed the patch last night though. Fixup on top?

If you don't mind :-)

> >> +		thumbnailPlanes.push_back({ rawThumbnail.data(), YPlaneSize });
> >> +		thumbnailPlanes.push_back({ rawThumbnail.data() + YPlaneSize, UVPlaneSize });
> >> +
> >> +		int jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
> >>   							 *thumbnail, {}, quality);
> >>   		thumbnail->resize(jpeg_size);
> >>

Patch
diff mbox series 

diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index 68d74842..ef2d98cc 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -72,7 +72,22 @@  void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
 		 */
 		thumbnail->resize(rawThumbnail.size());
 
-		int jpeg_size = thumbnailEncoder_.encode({ rawThumbnail },
+		/*
+		 * 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 jpeg_size = thumbnailEncoder_.encode(thumbnailPlanes,
 							 *thumbnail, {}, quality);
 		thumbnail->resize(jpeg_size);