Re: [PATCH RFC 1/4] V4L: Add V4L2_CID_FRAMESIZE image source class control

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

Hi,

On 08/24/2012 12:41 AM, Laurent Pinchart wrote:
> On Thursday 23 August 2012 21:24:12 Sakari Ailus wrote:
>> Sylwester Nawrocki wrote:
>>>> On Thu, Aug 23, 2012 at 11:51:26AM +0200, Sylwester Nawrocki wrote:
>>>>> The V4L2_CID_FRAMESIZE control determines maximum number
>>>>> of media bus samples transmitted within a single data frame.
>>>>> It is useful for determining size of data buffer at the
>>>>> receiver side.
>>>>>
>>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>> ---
>>>>>
>>>>>   Documentation/DocBook/media/v4l/controls.xml | 12 ++++++++++++
>>>>>   drivers/media/v4l2-core/v4l2-ctrls.c         |  2 ++
>>>>>   include/linux/videodev2.h                    |  1 +
>>>>>   3 files changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>>>>> b/Documentation/DocBook/media/v4l/controls.xml index 93b9c68..ad5d4e5
>>>>> 100644
>>>>> --- a/Documentation/DocBook/media/v4l/controls.xml
>>>>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>>>>> @@ -4184,6 +4184,18 @@ interface and may change in the future.</para>
>>>>>
>>>>>   	    conversion.
>>>>>   	    </entry>
>>>>>   	  
>>>>>   	  </row>
>>>>>
>>>>> +	  <row>
>>>>> +	    <entry
>>>>> spanname="id"><constant>V4L2_CID_FRAMESIZE</constant></entry>
>>>>> +	    <entry>integer</entry>
>>>>> +	  </row>
>>>>> +	  <row>
>>>>> +	    <entry spanname="descr">Maximum size of a data frame in media bus
>>>>> +	      sample units. This control determines maximum number of samples
>>>>> +	      transmitted per single compressed data frame. For generic raw
>>>>> +	      pixel formats the value of this control is undefined. This is
>>>>> +	      a read-only control.
>>>>> +	    </entry>
>>>>> +	  </row>
>>>>>
>>>>>   	  <row><entry></entry></row>
>>>>>   	
>>>>>   	</tbody>
>>>>>   	
>>>>>         </tgroup>
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> b/drivers/media/v4l2-core/v4l2-ctrls.c index b6a2ee7..0043fd2 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>> @@ -727,6 +727,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>>
>>>>>   	case V4L2_CID_VBLANK:			return "Vertical Blanking";
>>>>>   	case V4L2_CID_HBLANK:			return "Horizontal Blanking";
>>>>>   	case V4L2_CID_ANALOGUE_GAIN:		return "Analogue Gain";
>>>>>
>>>>> +	case V4L2_CID_FRAMESIZE:		return "Maximum Frame Size";
>>>>
>>>> I would put this to the image processing class, as the control isn't
>>>> related to image capture. Jpeg encoding (or image compression in
>>>> general) after all is related to image processing rather than capturing
>>>> it.
>>>
>>> All right, might make more sense that way. Let me move it to the image
>>> processing class then. It probably also makes sense to name it
>>> V4L2_CID_FRAME_SIZE, rather than V4L2_CID_FRAMESIZE.
>>
>> Hmm. While we're at it, as the size is maximum --- it can be lower ---
>> how about V4L2_CID_MAX_FRAME_SIZE or V4L2_CID_MAX_FRAME_SAMPLES, as the
>> unit is samples?
>
>> Does sample in this context mean pixels for uncompressed formats and
>> bytes (octets) for compressed formats? It's important to define it as
>> we're also using the term "sample" to refer to data units transferred
>> over a parallel bus per a clock cycle.
> 
> I agree with Sakari here, I find the documentation quite vague, I wouldn't 
> understand what the control is meant for from the documentation only.

I thought it was clear enough:

Maximum size of a data frame in media bus sample units.
                             ^^^^^^^^^^^^^^^^^^^^^^^^^
So that means the unit is a number of bits clocked by a single clock
pulse on parallel video bus... :) But how is media bus sample defined
in case of CSI bus ? Looks like "media bus sample" is a useless term
for our purpose.

I thought it was better than just 8-bit byte, because the data receiver
(bridge) could layout data received from video bus in various ways in
memory, e.g. add some padding. OTOH, would any padding make sense
for compressed streams ? It would break the content, wouldn't it ?

So I would propose to use 8-bit byte as a unit for this control and
name it V4L2_CID_MAX_FRAME_SIZE. All in all it's not really tied
to the media bus.

>> On serial busses the former meaning is more obvious.

--

Regards,
Sylwester