[libcamera-devel,1/3] android: jpeg: Return encoded bytes size from PostProcessorJpeg

10253 diff mbox series

Returning 0 from PostProcessJpeg::process() is not really helpful.
Also, one expects that the process() returns the size of the output
data from the processor.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/android/jpeg/post_processor_jpeg.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Umang,

Thank you for the patch.

On Mon, Oct 26, 2020 at 07:31:32PM +0530, Umang Jain wrote:
> Returning 0 from PostProcessJpeg::process() is not really helpful.
> Also, one expects that the process() returns the size of the output
> data from the processor.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/android/jpeg/post_processor_jpeg.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 93acfe5..c56f1b2 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -101,5 +101,5 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	const uint32_t jpeg_orientation = 0;
>  	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>  
> -	return 0;
> +	return jpeg_size;

Is this needed by the rest of the series ? If not, I'd rather skip it
for now, as returning the output data size will not always be as
straightforward as returning an integer. For post-processors that
produce multi-planar formats, we'll possibly need a size per plane. I'd
rather wait until we get an actual use case for the size in the caller.

>  }

Hi Laurent,

On 10/27/20 2:37 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Oct 26, 2020 at 07:31:32PM +0530, Umang Jain wrote:
>> Returning 0 from PostProcessJpeg::process() is not really helpful.
>> Also, one expects that the process() returns the size of the output
>> data from the processor.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>   src/android/jpeg/post_processor_jpeg.cpp | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index 93acfe5..c56f1b2 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -101,5 +101,5 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>>   	const uint32_t jpeg_orientation = 0;
>>   	metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
>>   
>> -	return 0;
>> +	return jpeg_size;
> Is this needed by the rest of the series ? If not, I'd rather skip it
> for now, as returning the output data size will not always be as
> straightforward as returning an integer. For post-processors that
> produce multi-planar formats, we'll possibly need a size per plane. I'd
> rather wait until we get an actual use case for the size in the caller.
No, it's not needed by rest of the series as such (and I will take a 
deeper look today to double-check it). I needed this when I tested these 
patches, in the instance where I needed to write the encoded thumbnail 
out to a file for inspection. I  need to know the no. of bytes I need to 
write to file from destination span. And yes, I agree with your analysis 
that we might need to rework this entirely to support returning 
multi-planer formats.


>
>>   }