Message-ID: <5a94d216-dc61-968e-2eda-8f460c42b4ca@linaro.org>
Date:   Tue, 7 May 2019 19:54:01 +0300
From:   Stanimir Varbanov <stanimir.varbanov@...aro.org>
To:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] media/doc: Allow sizeimage to be set by v4l clients

Hi Mauro,

Thanks for comments!

On 5/2/19 4:29 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 2 May 2019 15:16:54 +0200
> Hans Verkuil <hverkuil@...all.nl> escreveu:
> 
>> On 5/2/19 2:55 PM, Mauro Carvalho Chehab wrote:
>>> Em Fri, 12 Apr 2019 18:59:15 +0300
>>> Stanimir Varbanov <stanimir.varbanov@...aro.org> escreveu:
>>>   
>>>> This changes v4l2_pix_format and v4l2_plane_pix_format sizeimage
>>>> field description to allow v4l clients to set bigger image size
>>>> in case of variable length compressed data.
>>>>
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@...aro.org>
>>>> ---
>>>>  Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst | 13 ++++++++++++-
>>>>  Documentation/media/uapi/v4l/pixfmt-v4l2.rst        | 11 ++++++++++-
>>>>  2 files changed, 22 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>> index 5688c816e334..005428a8121e 100644
>>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>> @@ -31,7 +31,18 @@ describing all planes of that format.
>>>>  
>>>>      * - __u32
>>>>        - ``sizeimage``
>>>> -      - Maximum size in bytes required for image data in this plane.
>>>> +      - Maximum size in bytes required for image data in this plane,
>>>> +	set by the driver. When the image consists of variable length
>>>> +	compressed data this is the number of bytes required by the
>>>> +	codec to support the worst-case compression scenario.
>>>> +
>>>> +	For uncompressed images the driver will set the value. For
>>>> +	variable length compressed data clients are allowed to set
>>>> +	the sizeimage field, but the driver may ignore it and set the
>>>> +	value itself, or it may modify the provided value based on
>>>> +	alignment requirements or minimum/maximum size requirements.
>>>> +	If the client wants to leave this to the driver, then it should
>>>> +	set sizeimage to 0.
>>>>      * - __u32
>>>>        - ``bytesperline``
>>>>        - Distance in bytes between the leftmost pixels in two adjacent
>>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>> index 71eebfc6d853..0f7771151db9 100644
>>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>> @@ -89,7 +89,16 @@ Single-planar format structure
>>>>        - Size in bytes of the buffer to hold a complete image, set by the
>>>>  	driver. Usually this is ``bytesperline`` times ``height``. When
>>>>  	the image consists of variable length compressed data this is the
>>>> -	maximum number of bytes required to hold an image.
>>>> +	number of bytes required by the codec to support the worst-case
>>>> +	compression scenario.
>>>> +
>>>> +	For uncompressed images the driver will set the value. For
>>>> +	variable length compressed data clients are allowed to set
>>>> +	the sizeimage field, but the driver may ignore it and set the
>>>> +	value itself, or it may modify the provided value based on
>>>> +	alignment requirements or minimum/maximum size requirements.
>>>> +	If the client wants to leave this to the driver, then it should
>>>> +	set sizeimage to 0.  
>>>
>>> It is very confusing to understand what you meant by the above paragraph,
>>> as you inverted the sentence order and forgot a comma.
>>>
>>> I would, instead, write the phrases using the direct order, and break
>>> into two paragraphs, e. g., changing the above to:
>>>
>>> 	"The driver will set the value for uncompressed images.
>>>
>>> 	Clients are allowed to set the sizeimage field for variable length
>>> 	compressed data, but the driver may ignore it and set the
>>> 	value itself, or it may modify the provided value based on
>>> 	alignment requirements or minimum/maximum size requirements.
>>> 	If the client wants to leave this to the driver, then it should
>>> 	set sizeimage to 0."
>>>
>>> That makes it a lot easier to read, hopefully preventing mistakes from
>>> app and driver developers when reading about sizeimage.
>>>
>>> Yet, I'm not too comfortable on letting this too generic. I mean,
>>> how an app writer would know what formats are "variable length
>>> compressed data", specially since libv4l may actually change that.  
>>
>> It's actually quite clearly defined: compressed formats set the
>> V4L2_FMT_FLAG_COMPRESSED flag in VIDIOC_ENUMFMT.
> 
> Ok, so let's be explicit here, e. g. something like:
> 
>  	"Clients are allowed to set the sizeimage field for variable length
>  	compressed data flagged with V4L2_FMT_FLAG_COMPRESSED at
> 	VIDIOC_ENUMFMT, but the driver may ignore it and set the
>  	value itself, or it may modify the provided value based on
>  	alignment requirements or minimum/maximum size requirements.
>  	If the client wants to leave this to the driver, then it should
>  	set sizeimage to 0."
> 
> That makes clear for app developers when they can use this new
> feature.

OK, I will resend with that description.

> 
> That still leads us to what happens at libv4l with sizeimage
> for a compressed format that got uncompressed by the library, in
> order to ensure that a change like this won't cause breakages at
> existing userspace apps.

libv4l can decompress formats like MJPEG, right? I mean it isn't to
decompress MPEG/H264 for example.

-- 
-- 
regards,
Stan

Powered by blists - more mailing lists