Re: [PATCH v3 1/2] virtio-video: Add virtio video device specification

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 06, 2020 at 07:20:57PM +0900, Keiichi Watanabe wrote:
> From: Dmitry Sepp <dmitry.sepp@xxxxxxxxxxxxxxx>
> 
> The virtio video encoder device and decoder device provide functionalities to
> encode and decode video stream respectively.
> Though video encoder and decoder are provided as different devices, they use a
> same protocol.
> 
> Signed-off-by: Dmitry Sepp <dmitry.sepp@xxxxxxxxxxxxxxx>
> Signed-off-by: Keiichi Watanabe <keiichiw@xxxxxxxxxxxx>

Finally found the time for a closer look.
Pretty good overall, some minor nits below ...

> +\begin{description}
> +\item[\field{version}] is the protocol version that the device talks.
> +  The device MUST set this to 0.

What is the intended use case for this?

Given that virtio has feature flags to negotiate support for optional
features and protocol extensions between driver and device, why do you
think this is needed?

> +The format description \field{virtio_video_format_desc} is defined as
> +follows:
> +\begin{lstlisting}
> +enum virtio_video_format {
> +        /* Raw formats */
> +        VIRTIO_VIDEO_FORMAT_RAW_MIN = 1,
> +        VIRTIO_VIDEO_FORMAT_ARGB8888 = VIRTIO_VIDEO_FORMAT_RAW_MIN,
> +        VIRTIO_VIDEO_FORMAT_BGRA8888,
> +        VIRTIO_VIDEO_FORMAT_NV12, /* 12  Y/CbCr 4:2:0  */
> +        VIRTIO_VIDEO_FORMAT_YUV420, /* 12  YUV 4:2:0     */
> +        VIRTIO_VIDEO_FORMAT_YVU420, /* 12  YVU 4:2:0     */
> +        VIRTIO_VIDEO_FORMAT_RAW_MAX = VIRTIO_VIDEO_FORMAT_YVU420,

I'm wondering what the *_MIN and *_MAX values here (and elsewhere) are
good for?  I doubt drivers would actually loop over formats from min to
max, I'd expect they check for specific formats they can handle instead.

If you want define the range for valid raw formats I'd suggest to leave
some room, so new formats can be added without changing MAX values, i.e.
use -- for example -- RAW_MIN = 0x100, RAW_MAX = 0x1ff, CODED_MIN=0x200,
CODED_MAX=0x2ff.  Or just drop them ...

> +struct virtio_video_query_control_level {
> +        le32 profile; /* One of VIRTIO_VIDEO_PROFILE_* */
                                                ^^^^^^^  LEVEL ?

cheers,
  Gerd




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux