Re: [Xen-devel][PATCH 1/1] cameraif: add ABI for para-virtual camera

[Hans Verkuil <hverkuil@xs4all.nl>]

On 09/11/2018 08:52 AM, Oleksandr Andrushchenko wrote:
> Hi, Hans!
> 
> On 09/10/2018 03:26 PM, Hans Verkuil wrote:
>> On 09/10/2018 01:49 PM, Oleksandr Andrushchenko wrote:
>>> On 09/10/2018 02:09 PM, Hans Verkuil wrote:
>>>> On 09/10/2018 11:52 AM, Oleksandr Andrushchenko wrote:
>>>>> On 09/10/2018 12:04 PM, Hans Verkuil wrote:
>>>>>> On 09/10/2018 10:24 AM, Oleksandr Andrushchenko wrote:
>>>>>>> On 09/10/2018 10:53 AM, Hans Verkuil wrote:
>>>>>>>> Hi Oleksandr,
>>>>>>>>
>>>>>>>> On 09/10/2018 09:16 AM, Oleksandr Andrushchenko wrote:
>>>>>> <snip>
>>>>>>
>>>>>>>>>> I suspect that you likely will want to support such sources eventually, so
>>>>>>>>>> it pays to design this with that in mind.
>>>>>>>>> Again, I think that this is the backend to hide these
>>>>>>>>> use-cases from the frontend.
>>>>>>>> I'm not sure you can: say you are playing a bluray connected to the system
>>>>>>>> with HDMI, then if there is a resolution change, what do you do? You can tear
>>>>>>>> everything down and build it up again, or you can just tell frontends that
>>>>>>>> something changed and that they have to look at the new vcamera configuration.
>>>>>>>>
>>>>>>>> The latter seems to be more sensible to me. It is really not much that you
>>>>>>>> need to do: all you really need is an event signalling that something changed.
>>>>>>>> In V4L2 that's the V4L2_EVENT_SOURCE_CHANGE.
>>>>>>> well, this complicates things a lot as I'll have to
>>>>>>> re-allocate buffers - right?
>>>>>> Right. Different resolutions means different sized buffers and usually lots of
>>>>>> changes throughout the whole video pipeline, which in this case can even
>>>>>> go into multiple VMs.
>>>>>>
>>>>>> One additional thing to keep in mind for the future: V4L2_EVENT_SOURCE_CHANGE
>>>>>> has a flags field that tells userspace what changed. Right now that is just the
>>>>>> resolution, but in the future you can expect flags for cases where just the
>>>>>> colorspace information changes, but not the resolution.
>>>>>>
>>>>>> Which reminds me of two important missing pieces of information in your protocol:
>>>>>>
>>>>>> 1) You need to communicate the colorspace data:
>>>>>>
>>>>>> - colorspace
>>>>>> - xfer_func
>>>>>> - ycbcr_enc/hsv_enc (unlikely you ever want to support HSV pixelformats, so I
>>>>>>      think you can ignore hsv_enc)
>>>>>> - quantization
>>>>>>
>>>>>> See https://hverkuil.home.xs4all.nl/spec/uapi/v4l/pixfmt-v4l2.html#c.v4l2_pix_format
>>>>>> and the links to the colorspace sections in the V4L2 spec for details).
>>>>>>
>>>>>> This information is part of the format, it is reported by the driver.
>>>>> I'll take a look and think what can be put and how into the protocol,
>>>>> do you think I'll have to implement all the above for
>>>>> this stage?
>>>> Yes. Without it VMs will have no way of knowing how to reproduce the right colors.
>>>> They don't *have* to use this information, but it should be there. For cameras
>>>> this isn't all that important, for SDTV/HDTV sources this becomes more relevant
>>>> (esp. the quantization and ycbcr_enc information) and for sources with BT.2020/HDR
>>>> formats this is critical.
>>> ok, then I'll add the following to the set_config request/response:
>>>
>>>       uint32_t colorspace;
>>>       uint32_t xfer_func;
>>>       uint32_t ycbcr_enc;
>>>       uint32_t quantization;
> Yet another question here: are the above (color space, xfer etc.) and
> display aspect ratio defined per pixel_format or per pixel_format + 
> resolution?
> 
> If per pixel_format then
> 
> .../vcamera/1/formats/YUYV/display-aspect-ratio = "59/58"
> 
> or if per resolution
> 
> .../vcamera/1/formats/YUYV/640x480/display-aspect-ratio = "59/58"

They are totally independent of resolution or pixelformat, with the
exception of ycbcr_enc which is of course ignored for RGB pixelformats.

They are set by the driver, never by the application.

For HDMI sources these values can change depending on what source is
connected, so they are not fixed and you need to query them whenever
a new source is connected. In fact, then can change midstream, but we
do not have good support for that at the moment.

Regards,

	Hans