Re: [PATCH v2 0/1] Virtio Video V4L2 driver

[Dmitry Sepp <dmitry.sepp@opensynergy.com>]

Hi Nicolas,

On Freitag, 13. März 2020 03:29:44 CET Nicolas Dufresne wrote:
> Hi Dimitry,
> 
> Le jeudi 12 mars 2020 à 11:29 +0100, Dmitry Sepp a écrit :
> > Hi Hans,
> > 
> > I'm not sure about crosvm, for us it is probably still feasible to
> > implement FWHT in the device (but it is unfortunately not supposed to be
> > upstreamed yet).
> > 
> > The main question is what would be the proper user-space tool to test
> > that? Is v4l2-ctl OK for that? As for gstreamer, AFAIK it does not
> > respect the v4l2 Video Decoder Interface Spec and we have seen some
> > issues with it.
> GStreamer element has been implemented to support what real drivers do,
> not what the spec suggest should be done. AFAIC, not all drivers have
> been updated with all the new features required by the spec. And the
> new features are not compatible with drivers that are not ported, so it
> creates a lot of complexity for userspace to stay backward compatible.
> Though, the spec should allow drivers to stay backward compatible, if
> not, we'd be very happy to learn why.
> 
> About the other issues, I'd be very happy to learn what they are, it's
> the first feedback I get from your team thus far. Please feel free to
> file issues or send me (or gstreamer-devel list) an email.

I guess the issues we've observed are related to your point from above: spec 
vs real HW.

We had that issue with buffer pool configuration for instance: https://
gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/672

Also it would be nice to wait for metadata to be decoded and only then send 
V4L2_CID_MIN_BUFFERS_FOR_CAPTURE. Currently gstreamer queries that on pipeline 
start, so for the virtio-video driver we simply have to return 0.

Best regards,
Dmitry.

> 
> In term of userspace, please consider FFMPEG also, as it's support for
> V4L2 Stateful CODECs has gained momentum. It is again implemented to
> support real drivers, but started much more recently targetting the
> Qualcomm/Venus driver, so it didn't have to suffer all the legacy and
> directions changes in the V4L2 API since 2011.
> 
> As for the virtio video driver, it will remain useless to non-
> chromeos/chrome users as long as it's not supported by any userspace.
> I'd be very happy so see more contribution into FFMPEG and GStreamer to
> implement the features that, for now, are only implemented in the
> spec.
> 
> From your comment, bridging a Linux driver in the host through virtio-
> video seems like a huge tasks. I believe this is an important issue to
> address in the long term. If you could provide more details, I think it
> would benefit to readers understanding.
> 
> > Best regards,
> > Dmitry.
> > 
> > On Donnerstag, 12. März 2020 10:54:35 CET Hans Verkuil wrote:
> > > On 3/12/20 10:49 AM, Keiichi Watanabe wrote:
> > > > Hi Hans,
> > > > 
> > > > On Wed, Mar 11, 2020 at 10:26 PM Hans Verkuil <hverkuil@xs4all.nl> 
wrote:
> > > > > Hi Dmitry,
> > > > > 
> > > > > On 2/18/20 9:27 PM, Dmitry Sepp wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > This is a v4l2 virtio video driver for the virtio-video device
> > > > > > specification v3 [1].
> > > > > > 
> > > > > > The first version of the driver was introduced here [2].
> > > > > > 
> > > > > > Changes v1 -> v2:
> > > > > > * support the v3 spec (mostly)
> > > > > > * add a module parameter to ask for pages from ZONE_DMA
> > > > > > 
> > > > > > What is not implemented:
> > > > > > * Plane layout flags should be used to propagate number of planes
> > > > > > to
> > > > > > 
> > > > > >   user-space
> > > > > > 
> > > > > > * There is no real use of stream creation with bitstream format in
> > > > > > the
> > > > > > 
> > > > > >   parameter list. The driver just uses the first bitstream format
> > > > > >   from
> > > > > >   the list.
> > > > > > 
> > > > > > * Setting bitrate is done in a different way compared to the spec.
> > > > > > This
> > > > > > 
> > > > > >   is because it has been already agreed on that the way the spec
> > > > > >   currently describes it requires changes.
> > > > > > 
> > > > > > Potential improvements:
> > > > > > * Do not send stream_create from open. Use corresponding state
> > > > > > machine
> > > > > > 
> > > > > >   condition to do this.
> > > > > > 
> > > > > > * Do not send stream_destroy from close. Do it in reqbufs(0).
> > > > > > * Cache format and control settings. Reduce calls to the device.
> > > > > 
> > > > > Some general notes:
> > > > > 
> > > > > Before this can be merged it needs to pass v4l2-compliance.
> > > > > 
> > > > > I also strongly recommend adding support for V4L2_PIX_FMT_FWHT to
> > > > > allow testing with the vicodec emulation driver. This will also
> > > > > allow testing all sorts of corner cases without requiring special
> > > > > hardware.
> > > > 
> > > > I agree that it's great if we could test virtio-video with vicodec,
> > > > but I wonder if supporting FWHT is actually needed for the initial
> > > > patch.
> > > > Though it wouldn't be difficult to support FWHT in the driver, we also
> > > > needs to support it in the host's hypervisor to test it. It's not easy
> > > > for our hypervisor to support FWHT, as it doesn't talk to v4l2 driver
> > > > directly.
> > > > Without the host-side implementation, it makes no sense to support it.
> > > > Also, if we support FWHT, we should have the format in a list of
> > > > supported formats in the virtio specification. However, I'm not sure
> > > > if FWHT is a general codec enough to be added in the spec, which
> > > > shouldn't be specific to Linux.
> > > 
> > > Good point, I didn't know that.
> > > 
> > > Is it possible to add support for FWHT under a linux debug/test option?
> > > 
> > > It's really the main reason for having this, since you would never use
> > > this in production code. But it is so nice to have for regression
> > > testing.
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 	
> > > > Best regards,
> > > > Keiichi
> > > > 
> > > > > Regards,
> > > > > 
> > > > >         Hans
> > > > > > 
> > > > > > Best regards,
> > > > > > Dmitry.
> > > > > > 
> > > > > > [1] https://markmail.org/message/dmw3pr4fuajvarth
> > > > > > [2] https://markmail.org/message/wnnv6r6myvgb5at6