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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

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"

>>
>> With this respect, I will need to put some OS agnostic constants
>> into the protocol, so if backend and frontend are not Linux/V4L2
>> based they can still talk to each other.
>> I see that V4L2 already defines constants for the above: [1], [2], [3], [4].
>>
>> Do you think I can define the same replacing V4L2_ prefix
>> with XENCAMERA_, e.g. V4L2_XFER_FUNC_SRGB -> XENCAMERA_XFER_FUNC_SRGB?
> Yes.
>
>> Do I need to define all those or there can be some subset of the
>> above for my simpler use-case?
> Most of these defines directly map to standards. I would skip the following
> defines:
>
> V4L2_COLORSPACE_DEFAULT (not applicable)
> V4L2_COLORSPACE_470_SYSTEM_*  (rarely used, if received by the HW the Xen backend
> 			should map this to V4L2_COLORSPACE_SMPTE170M)
> V4L2_COLORSPACE_JPEG (historical V4L2 artifact, see here how to map:
> 	 https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-details.html#col-jpeg)
>
> V4L2_COLORSPACE_SMPTE240M (rarely used, map to V4L2_COLORSPACE_SMPTE170M if seen in backend)
>
> V4L2_XFER_FUNC_SMPTE240M (rarely used, map to V4L2_XFER_FUNC_709)
>
> V4L2_YCBCR_ENC_SMPTE240M (rarely used, map to V4L2_YCBCR_ENC_709)
>
> While V4L2 allows 0 (DEFAULT) values for xfer_func, ycbcr_enc and quantization, and
> provides macros to map default values to the actual values (for legacy reasons),
> the Xen backend should always fill this in explicitly, using those same mapping
> macros (see e.g. V4L2_MAP_XFER_FUNC_DEFAULT).
>
> The V4L2 spec has extensive information on colorspaces (sections 2.14-2.17).
>
>>> The vivid driver can actually reproduce all combinations, so that's a good driver
>>> to test this with.
>> You mean I can use it on backend side instead of real HW camera and
>> test all the configurations possible/those of interest?
> Right.
>
> Regards,
>
> 	Hans
Thank you,
Oleksandr