linux-kernel - Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f374662e-64f8-d280-21f8-892c70b6b29e@xs4all.nl>
Date:   Wed, 10 Apr 2019 16:39:47 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        linux-media@...r.kernel.org
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        Vikash Garodia <vgarodia@...eaurora.org>,
        Tomasz Figa <tfiga@...omium.org>,
        Alexandre Courbot <acourbot@...omium.org>,
        Malathi Gottam <mgottam@...eaurora.org>
Subject: Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients

On 4/10/19 4:21 PM, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 3/14/19 3:11 PM, Hans Verkuil wrote:
>> On 1/21/19 11:48 AM, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 1/18/19 11:13 AM, Hans Verkuil wrote:
>>>> On 1/16/19 1:37 PM, Stanimir Varbanov wrote:
>>>>> 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 | 5 ++++-
>>>>>  Documentation/media/uapi/v4l/pixfmt-v4l2.rst        | 3 ++-
>>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>>> index 7f82dad9013a..dbe0b74e9ba4 100644
>>>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2-mplane.rst
>>>>> @@ -30,7 +30,10 @@ 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 maximum number of bytes required
>>>>> +        to hold an image, and it is allowed to be set by the client.
>>>>>      * - __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..54b6d2b67bd7 100644
>>>>> --- a/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>>> +++ b/Documentation/media/uapi/v4l/pixfmt-v4l2.rst
>>>>> @@ -89,7 +89,8 @@ 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.
>>>>> +	maximum number of bytes required to hold an image, and it is
>>>>> +	allowed to be set by the client.
>>>>>      * - __u32
>>>>>        - ``colorspace``
>>>>>        - Image colorspace, from enum :c:type:`v4l2_colorspace`.
>>>>>
>>>>
>>>> Hmm. "maximum number of bytes required to hold an image": that's not actually true
>>>> for bitstream formats like MPEG. It's just the size of the buffer used to store the
>>>> bitstream, i.e. one buffer may actually contain multiple compressed images, or a
>>>> compressed image is split over multiple buffers.
>>>>
>>>
>>> Do you want me to change something in the current documentation, i.e.
>>> the quoted above?
>>
>> Hmm, it looks like this discussion stalled (i.e. I forgot to reply).
>>
>> How about this:
>>
>> "When the image consists of variable length compressed data this is the
>> number of bytes required by the encoder to support the worst-case
> 
> I don't think 'encoder' is the right word here:
> 
> s/encoder/encoder or decoder

or perhaps just 'codec'? Either is fine by me.

> 
>> compression scenario. Clients are allowed to set this field. However,
>> drivers may ignore the value or modify it."
> 
> Can we rephrase to:
> 
> "Clients are allowed to set sizeimage field, but however drivers my
> ignore the value or modify it."
> 

How about this:

"Clients are allowed to set the sizeimage field, but drivers may
 modify it."

'ignore the value' isn't right, since that suggests that the sizeimage
value that is returned may not be in actual use. Since sizeimage can now
be set by userspace, this means that a sanity check is needed in drivers
as well (i.e. what if userspace sets sizeimage to ~0?)

This should be tested in v4l2-compliance as well.

Regards,

	Hans