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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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;
> >
> > 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).
Thank you for such a detailed explanation!
I'll define the constants as agreed above.

> > > 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
It seems that the number of changes discussed are begging
for the v2 of the protocol to be published ;)

Thank you,
Oleksandr

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