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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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?
> Regards,
>
>         Hans
Thank you,
Oleksandr

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