linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
@ 2019-01-16 12:37 Stanimir Varbanov
  2019-01-18  9:13 ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Stanimir Varbanov @ 2019-01-16 12:37 UTC (permalink / raw)
  To: linux-media
  Cc: Mauro Carvalho Chehab, Hans Verkuil, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam,
	Stanimir Varbanov

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@linaro.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`.
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
  2019-01-16 12:37 [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients Stanimir Varbanov
@ 2019-01-18  9:13 ` Hans Verkuil
  2019-01-21 10:48   ` Stanimir Varbanov
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2019-01-18  9:13 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media
  Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam

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@linaro.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.

Only for MJPEG is this statement true since each buffer will contain a single
compressed JPEG image.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
  2019-01-18  9:13 ` Hans Verkuil
@ 2019-01-21 10:48   ` Stanimir Varbanov
  2019-03-14 13:11     ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Stanimir Varbanov @ 2019-01-21 10:48 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam

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@linaro.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?

> Only for MJPEG is this statement true since each buffer will contain a single
> compressed JPEG image.
> 
> Regards,
> 
> 	Hans
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
  2019-01-21 10:48   ` Stanimir Varbanov
@ 2019-03-14 13:11     ` Hans Verkuil
  2019-04-10 14:21       ` Stanimir Varbanov
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2019-03-14 13:11 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media
  Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam

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@linaro.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
compression scenario. Clients are allowed to set this field. However,
drivers may ignore the value or modify it."

Regards,

	Hans

> 
>> Only for MJPEG is this statement true since each buffer will contain a single
>> compressed JPEG image.
>>
>> Regards,
>>
>> 	Hans
>>
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
  2019-03-14 13:11     ` Hans Verkuil
@ 2019-04-10 14:21       ` Stanimir Varbanov
  2019-04-10 14:39         ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Stanimir Varbanov @ 2019-04-10 14:21 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam

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@linaro.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

> 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."

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
  2019-04-10 14:21       ` Stanimir Varbanov
@ 2019-04-10 14:39         ` Hans Verkuil
  2019-04-10 14:52           ` Stanimir Varbanov
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2019-04-10 14:39 UTC (permalink / raw)
  To: Stanimir Varbanov, linux-media
  Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam

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@linaro.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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients
  2019-04-10 14:39         ` Hans Verkuil
@ 2019-04-10 14:52           ` Stanimir Varbanov
  0 siblings, 0 replies; 7+ messages in thread
From: Stanimir Varbanov @ 2019-04-10 14:52 UTC (permalink / raw)
  To: Hans Verkuil, linux-media
  Cc: Mauro Carvalho Chehab, linux-kernel, linux-arm-msm,
	Vikash Garodia, Tomasz Figa, Alexandre Courbot, Malathi Gottam



On 4/10/19 5:39 PM, Hans Verkuil wrote:
> 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@linaro.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.

OK, then just 'codec'.

> 
>>
>>> 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."

I'm fine with that.

I'll send with those changes and as regular patch. Thanks!

> 
> '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
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-04-10 14:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 12:37 [RFC PATCH] media/doc: Allow sizeimage to be set by v4l clients Stanimir Varbanov
2019-01-18  9:13 ` Hans Verkuil
2019-01-21 10:48   ` Stanimir Varbanov
2019-03-14 13:11     ` Hans Verkuil
2019-04-10 14:21       ` Stanimir Varbanov
2019-04-10 14:39         ` Hans Verkuil
2019-04-10 14:52           ` Stanimir Varbanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).