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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 09/11/18 09:14, Oleksandr Andrushchenko wrote:
> On 09/11/2018 10:04 AM, Hans Verkuil wrote:
>> 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.
> Ah, great, then I'll define colorspace, xfer_func, quantization
> and display aspect ratio as part of virtual camera device configuration
> (as vcamera represents a single source) and ycbcr_enc as a part
> of pixel format configuration (one ycbcr_enc per each
> pixel format)
> 
> Does this sound ok?

Uh, no :-)

ycbcr_enc is not tied to specific pixel formats. The Y'CbCr encoding tells
you how the Y'CbCr values were derived from the R'G'B' values. So this only
makes sense if you are in fact receiving Y'CbCr pixels, otherwise you just
ignore it.

It's up to you what value to assign to ycbcr_enc in that case: V4L2 doesn't
have any hard requirements for that AFAIK, although it will most likely be
set to 0 (V4L2_YCBCR_ENC_DEFAULT).

Regards,

        Hans

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel