[PATCH v3 0/2] Virtio video device specification

[PATCH v3 0/2] Virtio video device specification

[Keiichi Watanabe]

Hi,
Here is the 3rd version of virtio-video patches.

This patch set consists of two changes.
The first patch adds the virtio-video protocol. This is an updated version of v2 patch [1].
The second patch adds a new feature to use exported objects from different virtio devices, which are proposed in [2], as video buffers.

PDFs are avaliable below:
* full version [3]
* only virtio-video section (first patch) [4]
* only virtio-video section (first+second patch) [5]

Best regards,
Keiichi

[1] https://markmail.org/thread/yb25fim2dqfuktgf
[2] https://markmail.org/message/2p5zgfanuv3fgwcu
[3] https://drive.google.com/file/d/1Kl3M6weAm0vV1kj9A5dq2yypLXe_6TS2/view?usp=sharing
[4] https://drive.google.com/file/d/1fN_6lft3RPuFKOnBm6P6XCy3fF7wwnu5/view?usp=sharing
[5] https://drive.google.com/file/d/1jOsS2WdVhL4PpcWLO8Zukq5J0fXDiWn-/view?usp=sharing

Changes v2 -> v3:
* Rename controlq -> commandq.
* Add {QUERY,GET,SET}_CONTROL for bitrate, profile and level.
* Update the definition of virtio_video_format_desc.
  - Remove fields for profiles and levels.
  - Define fields for memory layouts.
  - Stop using FOURCC and define enum virtio_video_format.
* Add a feature flag for non-contiguous memories.
* Add a new section for buffer lifecycle.
* Change RESOURCE_DESTROY to RESOURCE_DESTROY_ALL.
* Add fields in virtio_video_config.
* Remove constants like *_UNDEFINED or *_UNSPEC.
* Rename some constants and structs.
* Change structures and orders of subsections and paragraphs.
* Add more detailed description for each command.
* Address review comments.
* Add a feature for exported objects as a separate patch.

Dmitry Sepp (1):
  virtio-video: Add virtio video device specification

Keiichi Watanabe (1):
  virtio-video: Define a feature for exported objects from different
    virtio devices

 .gitignore                        |    1 +
 content.tex                       |    1 +
 images/video-buffer-lifecycle.dot |   18 +
 make-setup-generated.sh           |    8 +
 virtio-video.tex                  | 1030 +++++++++++++++++++++++++++++
 5 files changed, 1058 insertions(+)
 create mode 100644 .gitignore
 create mode 100644 images/video-buffer-lifecycle.dot
 create mode 100644 virtio-video.tex

--
2.25.0.341.g760bfbb309-goog

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

[Keiichi Watanabe]

From: Dmitry Sepp <dmitry.sepp@opensynergy.com>

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@opensynergy.com>
Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
---
 .gitignore                        |   1 +
 content.tex                       |   1 +
 images/video-buffer-lifecycle.dot |  18 +
 make-setup-generated.sh           |   8 +
 virtio-video.tex                  | 988 ++++++++++++++++++++++++++++++
 5 files changed, 1016 insertions(+)
 create mode 100644 .gitignore
 create mode 100644 images/video-buffer-lifecycle.dot
 create mode 100644 virtio-video.tex

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..31272c2
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1 @@
+/images/generated/
diff --git a/content.tex b/content.tex
index b91a132..b75a40f 100644
--- a/content.tex
+++ b/content.tex
@@ -6062,6 +6062,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-fs.tex}
 \input{virtio-rpmb.tex}
 \input{virtio-iommu.tex}
+\input{virtio-video.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/images/video-buffer-lifecycle.dot b/images/video-buffer-lifecycle.dot
new file mode 100644
index 0000000..98f379b
--- /dev/null
+++ b/images/video-buffer-lifecycle.dot
@@ -0,0 +1,18 @@
+digraph {
+  graph [ rankdir = LR, layout = dot ];
+
+  init [style = invis]
+  destroyed  [style = invis]
+  created [label="Created", shape=circle]
+  dequeued [label="Dequeued", shape=circle]
+  queued [label="Queued", shape=circle]
+
+  init -> created [label="RESOURCE_CREATE"]
+
+  created -> queued [label="RESOURCE_QUEUE is sent"]
+  dequeued -> queued [label="RESOURCE_QUEUE\n is sent"]
+  queued -> dequeued [label="RESOURCE_QUEUE\n has returned"]
+
+  created -> destroyed [label="RESOURCE_DESTROY_ALL"]
+  dequeued -> destroyed [label="RESOURCE_DESTROY_ALL"]
+}
diff --git a/make-setup-generated.sh b/make-setup-generated.sh
index f15d148..4caff72 100755
--- a/make-setup-generated.sh
+++ b/make-setup-generated.sh
@@ -61,3 +61,11 @@ cat > setup-generated.tex <<EOF
 \newcommand{\virtiodraftstagename}{$STAGENAME}
 \newcommand{\virtiodraftoasisstagename}{$OASISSTAGENAME}
 EOF
+
+# Generate PNG from DOT
+mkdir -p images/generated
+for file in images/*.dot
+do
+    BASENAME=`basename "$file" .dot`
+    dot -Tpng -o images/generated/${BASENAME}.png ${file}
+done
diff --git a/virtio-video.tex b/virtio-video.tex
new file mode 100644
index 0000000..2eeee53
--- /dev/null
+++ b/virtio-video.tex
@@ -0,0 +1,988 @@
+\section{Video Device}\label{sec:Device Types / Video Device}
+
+The virtio video encoder device and decoder device are virtual devices
+that supports encoding and decoding respectively. While the encoder
+and the decoder are different devices, they use the same protocol.
+
+\subsection{Device ID}
+\label{sec:Device Types / Video Device / Device ID}
+
+\begin{description}
+\item[30] encoder device
+\item[31] decoder device
+\end{description}
+
+\subsection{Virtqueues}
+\label{sec:Device Types / Video Device / Virtqueues}
+
+\begin{description}
+\item[0] commandq - queue for sending commands.
+\item[1] eventq - queue for sending events happened in the device.
+\end{description}
+
+\subsection{Feature bits}
+\label{sec:Device Types / Video Device / Feature bits}
+
+\begin{description}
+\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)] Guest pages can be used
+  for video buffers.
+\item[VIRTIO_VIDEO_F_RESOURCE_NON_CONTIG (1)] The device can use
+  non-contiguous memories for video buffers. Without this flag, the
+  driver and device MUST use video buffers that are contiguous in the
+  device-side.
+\end{description}
+
+\devicenormative{\subsubsection}{Feature bits}{Device Types / Video
+  Device / Feature bits}
+
+The device MUST offer VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES.
+
+\subsection{Device configuration layout}
+\label{sec:Device Types / Video Device / Device configuration layout}
+
+Video device configuration uses the following layout structure:
+
+\begin{lstlisting}
+struct virtio_video_config {
+        le32 version;
+        le32 max_caps_length;
+        le32 max_resp_length;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{version}] is the protocol version that the device talks.
+  The device MUST set this to 0.
+\item[\field{max_caps_length}] defines the maximum length of a
+  descriptor required to call VIRTIO_VIDEO_CMD_QUERY_CAPABILITY in
+  bytes. The device MUST set this value.
+\item[\field{max_resp_length}] defines the maximum length of a
+  descriptor required to call a command other than
+  VIRTIO_VIDEO_CMD_QUERY_CAPABILITY in bytes. The device MUST set this
+  value.
+\end{description}
+
+\subsection{Device Initialization}
+\label{sec:Device Types / Video Device / Device Initialization}
+
+\devicenormative{\subsubsection}{Device Initialization}{Device Types /
+  Video Device / Device Initialization}
+
+The driver SHOULD query device capability by using the
+VIRTIO_VIDEO_CMD_QUERY_CAPABILITY and use that information for the
+initial setup.
+
+\subsection{Device Operation}
+\label{sec:Device Types / Video Device / Device Operation}
+
+The driver allocates input and output buffers and queues the buffers
+to the device. The device performs operations on the buffers according
+to the function in question.
+
+\subsubsection{Command Virtqueue}
+
+\paragraph{Device Operation: Command header}
+
+All commands and responses on the command virtqueue have a fixed
+header using the following layout structure and definitions:
+
+\begin{lstlisting}
+  enum virtio_video_cmd_type {
+        /* Command */
+        VIRTIO_VIDEO_CMD_QUERY_CAPABILITY = 0x0100,
+        VIRTIO_VIDEO_CMD_STREAM_CREATE,
+        VIRTIO_VIDEO_CMD_STREAM_DESTROY,
+        VIRTIO_VIDEO_CMD_STREAM_DRAIN,
+        VIRTIO_VIDEO_CMD_RESOURCE_CREATE,
+        VIRTIO_VIDEO_CMD_RESOURCE_QUEUE,
+        VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL,
+        VIRTIO_VIDEO_CMD_QUEUE_CLEAR,
+        VIRTIO_VIDEO_CMD_GET_PARAMS,
+        VIRTIO_VIDEO_CMD_SET_PARAMS,
+        VIRTIO_VIDEO_CMD_QUERY_CONTROL,
+        VIRTIO_VIDEO_CMD_GET_CONTROL,
+        VIRTIO_VIDEO_CMD_SET_CONTROL,
+
+        /* Response */
+        VIRTIO_VIDEO_RESP_OK_NODATA = 0x0200,
+        VIRTIO_VIDEO_RESP_OK_QUERY_CAPABILITY,
+        VIRTIO_VIDEO_RESP_OK_RESOURCE_QUEUE,
+        VIRTIO_VIDEO_RESP_OK_GET_PARAMS,
+        VIRTIO_VIDEO_RESP_OK_QUERY_CONTROL,
+        VIRTIO_VIDEO_RESP_OK_GET_CONTROL,
+
+        VIRTIO_VIDEO_RESP_ERR_INVALID_OPERATION = 0x0300,
+        VIRTIO_VIDEO_RESP_ERR_OUT_OF_MEMORY,
+        VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID,
+        VIRTIO_VIDEO_RESP_ERR_INVALID_RESOURCE_ID,
+        VIRTIO_VIDEO_RESP_ERR_INVALID_PARAMETER,
+        VIRTIO_VIDEO_RESP_ERR_UNSUPPORTED_CONTROL,
+};
+
+struct virtio_video_cmd_hdr {
+        le32 type;
+        le32 stream_id;
+};
+\end{lstlisting}
+
+The fixed header \field{virtio_video_cmd_hdr} in each message includes
+the following field:
+\begin{description}
+\item[\field{type}] specifies the type of the driver command
+  (VIRTIO_VIDEO_CMD_*) or the device response (VIRTIO_VIDEO_RESP_*).
+\item[\field{stream_id}] specifies a target stream if needed.
+\end{description}
+
+On success the device will return VIRTIO_VIDEO_RESP_OK_NODATA in case
+there is no payload. Otherwise the \field{type} field will indicate
+the kind of payload.
+
+On error the device will return one of the VIRTIO_VIDEO_RESP_ERR_*
+error codes.
+
+\paragraph{Device Operation: Query device capability}
+
+\begin{description}
+\item[VIRTIO_VIDEO_CMD_QUERY_CAPABILITY] Retrieve information about
+supported formats.
+
+The driver uses \field{virtio_video_query_capability} to send a
+query request.
+
+\begin{lstlisting}
+enum virtio_video_queue_type {
+        VIRTIO_VIDEO_QUEUE_TYPE_INPUT = 0x100,
+        VIRTIO_VIDEO_QUEUE_TYPE_OUTPUT,
+};
+
+struct virtio_video_query_capability {
+        struct virtio_video_ctrl_hdr hdr;
+        le32 queue_type;
+        u8 padding[4];
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{queue_type}] is the queue type that the driver asks
+information about. The driver MUST set either
+\field{VIRTIO_VIDEO_QUEUE_TYPE_INPUT} or
+\field{VIRTIO_VIDEO_QUEUE_TYPE_OUTPUT}.
+\end{description}
+
+The device responds to VIRTIO_VIDEO_CMD_QUERY_CAPABILITY with
+\field{virtio_video_query_capability_resp}.
+\begin{lstlisting}
+struct virtio_video_query_capability_resp {
+        struct virtio_video_cmd_hdr hdr;
+        le32 num_descs;
+        u8 padding[4];
+        /* Followed by struct virtio_video_format_desc descs[] */
+};
+\end{lstlisting}
+
+The device MUST return its capability with \field{
+  virtio_video_capability_resp} that includes the following fields:
+\begin{description}
+\item[\field{num_descs}] is a number of \field{virtio_video_format_desc}
+  that follow. The value MUST not exceed 64.
+\end{description}
+
+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,
+
+        /* Coded formats */
+        VIRTIO_VIDEO_FORMAT_CODED_MIN = 0x1000,
+        VIRTIO_VIDEO_FORMAT_MPEG2 = VIRTIO_VIDEO_FORMAT_CODED_MIN, /* MPEG-2 Part 2 */
+        VIRTIO_VIDEO_FORMAT_MPEG4, /* MPEG-4 Part 2 */
+        VIRTIO_VIDEO_FORMAT_H264, /* H.264 */
+        VIRTIO_VIDEO_FORMAT_HEVC, /* HEVC aka H.265*/
+        VIRTIO_VIDEO_FORMAT_VP8, /* VP8 */
+        VIRTIO_VIDEO_FORMAT_VP9, /* VP9 */
+        VIRTIO_VIDEO_FORMAT_CODED_MAX = VIRTIO_VIDEO_FORMAT_VP9,
+};
+
+enum virtio_video_planes_layout_flag {
+        VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER = 1 << 0,
+        VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE = 1 << 1,
+};
+
+struct virtio_video_format_desc {
+        le64 mask;
+        le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
+        le32 planes_layout; /* Bitmask with VIRTIO_VIDEO_PLANES_LAYOUT_* */
+        le32 plane_align;
+        le32 num_frames;
+        /* Followed by struct virtio_video_format_frame frames[] */
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{mask}] is a bitset that represents the supported
+  combination of input and output format. If \textit{i}-th bit is set
+  in \field{mask} of \textit{j}-th \field{virtio_video_format_desc}
+  for input, the device supports encoding or decoding from the
+  \textit{j}-th input format to \textit{i}-th output format.
+\item[\field{format}] specifies an image format. The device MUST set
+  one of \field{enum virtio_video_format}.
+\item[\field{planes_layout}] is a bitmask representing a set of plane
+  layout types the device supports. This driver MUST ignore this field
+  for encoded formats.
+  \begin{description}
+  \item[\field{VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER}] The device
+    expects planes in one buffer laid out one after another.
+  \item[\field{VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE}] The device
+    expects planes to be located in separate buffers.
+  \end{description}
+\item[\field{plane_align}] is a plane alignment the device require
+  when multiple planes are located in one buffer. The driver MUST
+  ignore this field if \field{format} is a bitstream format or
+  \field{planes_layout} doesn't have
+  \field{VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER} bit.
+\item[\field{num_frames}] is the number of
+  \field{virtio_video_format_frame} that follows.
+\end{description}
+
+The frame information \field{virtio_video_format_frame} is defined as
+follows:
+\begin{lstlisting}
+struct virtio_video_format_range {
+        le32 min;
+        le32 max;
+        le32 step;
+        u8 padding[4];
+};
+
+struct virtio_video_format_frame {
+        struct virtio_video_format_range width;
+        struct virtio_video_format_range height;
+        le32 num_rates;
+        u8 padding[4];
+        /* Followed by struct virtio_video_format_range frame_rates[] */
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{width, height}] represents a range of resolutions
+  supported by the device. If its \field{step} is not applicable, its
+  \field{min} is equal to its \field{max}.
+\item[\field{num_rates}] is the number of
+  \field{virtio_video_format_ranges} that follows to represent
+  supported frame rates.
+\end{description}
+\end{description}
+
+\devicenormative{\subparagraph}{Device Operation: Query device
+  capability}{Device Types / Video Device / Device Operation / Device
+  Operation: Query device capability}
+
+The total size of the device response MUST not exceed
+\field{max_cap_length} in bytes reported in the device configuration.
+
+\paragraph{Device Operation: Create streams}
+
+To process buffers, the device needs to associate them with a certain
+video stream (essentially, a context). Streams are created by
+VIRTIO_VIDEO_CMD_STREAM_CREATE with a default set of parameters
+determined by the device.
+
+\begin{description}
+\item[VIRTIO_VIDEO_CMD_STREAM_CREATE] Create a video stream (context)
+  within the device.
+
+\begin{lstlisting}
+enum virtio_video_mem_type {
+        VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES,
+};
+
+struct virtio_video_stream_create {
+        struct virtio_video_cmd_hdr hdr;
+        le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
+        le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
+        le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
+        u8 padding[4];
+        u8 tag[64];
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{in_mem_type, out_mem_type}] is a type of buffer
+  management for input /output buffers. The driver MUST set a value in
+  \field{enum virtio_video_mem_type} that the device reported a
+  corresponding feature bit.
+\begin{description}
+\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
+\end{description}
+\item[\field{coded_format}] is the encoded format that will be
+  processed.
+\item[\field{tag}] is the name associated with this stream. The tag
+  MUST be encoded in UTF-8 and NUL-terminated.
+\end{description}
+
+The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
+to an integer that is not used before. If a used value is passed as
+\field{stream_id}, the device MUST reports an error with
+VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.
+
+\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
+  (context) within the device.
+
+\begin{lstlisting}
+struct virtio_video_stream_destroy {
+        struct virtio_video_cmd_hdr hdr;
+};
+\end{lstlisting}
+\end{description}
+
+\paragraph{Device Operation: Import resources}
+
+\begin{itemize*}
+\item Use VIRTIO_VIDEO_CMD_RESOURCE_CREATE to import a resource to use
+  it as a video buffer.
+\item Use VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL to invalidate all the
+  resources imported by VIRTIO_VIDEO_CMD_RESOURCE_CREATE so far.
+\end{itemize*}
+
+\begin{description}
+\item[VIRTIO_VIDEO_CMD_RESOURCE_CREATE] Create a resource descriptor
+  within the device.
+
+The driver sends \field{virtio_video_resource_create} defined as
+follows:
+\begin{lstlisting}
+#define VIRTIO_VIDEO_MAX_PLANES 8
+
+struct virtio_video_resource_create {
+        struct virtio_video_cmd_hdr hdr;
+        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
+        le32 resource_id;
+        le32 planes_layout;
+        le32 num_planes;
+        le32 plane_offsets[VIRTIO_VIDEO_MAX_PLANES];
+        le32 num_entries[VIRTIO_VIDEO_MAX_PLANES];
+        /* Followed by struct virtio_video_mem_entry entries[] */
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{queue_type}] specifies a direction of queue which the
+  resource will be used for.
+\item[\field{resource_id}] is an identifier of the resource. The
+  driver MUST set it to an integer that is not used before for the
+  given \field{queue_type}. If the specified \field{resource_id} is
+  already in use, the device MUST report an error with
+  VIRTIO_VIDEO_RESP_ERR_INVALID_RESOURCE_ID.
+\item[\field{planes_layout}] specifies a plane layout. The driver MUST
+  set only one bit of \field{virtio_video_planes_layout_flag} that is
+  supported for a current raw format.
+\item[\field{num_planes}] specifies a number of planes.
+\item[\field{plane_offsets}] specifies offsets for each plane.
+\item[\field{num_entries}] is an array of numbers of \field{entries}
+  memory entries for each plane. If \field{planes_layout} is
+  VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER, only the first element is
+  used.
+\end{description}
+
+The \field{virtio_video_resource_create} is followed by an array of
+\field{virtio_video_mem_entry} defined as follows:
+\begin{lstlisting}
+struct virtio_video_mem_entry {
+        le64 addr;
+        le32 length;
+        u8 padding[4];
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{addr}] is the physical guest address.
+\item[\field{length}] is the length of the resource.
+\end{description}
+The number of \field{virtio_video_mem_entry} MUST be equal to the sum
+of integers in the array \field{num_entries}.
+
+\item[VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL] Invalidate all the
+  resource descriptor created so far.
+\begin{lstlisting}
+struct virtio_video_resource_destroy_all {
+        struct virtio_video_cmd_hdr hdr;
+        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
+        u8 padding[4];
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{queue_type}] is a type of queue.
+\end{description}
+\end{description}
+
+\paragraph{Device Operation: Process buffers}
+
+\begin{itemize*}
+\item Use VIRTIO_VIDEO_CMD_RESOURCE_QUEUE to queue the resource for
+  processing in the device. The request completes asynchronously and
+  out-of-order when the device has finished with the buffer.
+\item Use VIRTIO_VIDEO_CMD_STREAM_DRAIN to ask the device to process
+  and return all of the already queued buffers.
+\item Use VIRTIO_VIDEO_CMD_QUEUE_CLEAR to ask the device to return
+  back already queued buffers from the input or the output queue. This
+  also includes input or output buffers that can be currently owned by
+  the device's processing pipeline.
+\end{itemize*}
+
+\begin{description}
+\item[VIRTIO_VIDEO_CMD_RESOURCE_QUEUE] Add a buffer to the device's
+queue.
+
+\begin{lstlisting}
+struct virtio_video_resource_queue {
+        struct virtio_video_cmd_hdr hdr;
+        le32 queue_type;
+        le32 resource_id;
+        le64 timestamp;
+        le32 num_data_sizes;
+        le32 data_sizes[VIRTIO_VIDEO_MAX_PLANES];
+        u8 padding[4];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{resource_id}] is the ID of the resource to be queued.
+\item[\field{timestamp}] is an abstract sequence counter that can be
+  used for synchronization. For an input buffer, the driver MUST set a
+  \field{timestamp}.
+\item[\field{num_data_sizes}] is the number of \field{data_sizes}
+  entries in use.
+\item[\field{data_sizes}] number of data bytes within a plane.
+\end{description}
+
+The device returns \field{virtio_video_resource_queue_resp} defined as
+follows:
+\begin{lstlisting}
+enum virtio_video_buffer_flag {
+        VIRTIO_VIDEO_BUFFER_FLAG_ERR = 0x0001,
+        VIRTIO_VIDEO_BUFFER_FLAG_EOS = 0x0002,
+
+        /* Encoder only */
+        VIRTIO_VIDEO_BUFFER_FLAG_IFRAME = 0x0004,
+        VIRTIO_VIDEO_BUFFER_FLAG_PFRAME = 0x0008,
+        VIRTIO_VIDEO_BUFFER_FLAG_BFRAME = 0x0010,
+};
+
+struct virtio_video_resource_queue_resp {
+        struct virtio_video_cmd_hdr hdr;
+        le64 timestamp;
+        le32 flags;
+        le32 size; /* Encoded size */
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{timestamp}] is an abstract sequence counter that can be
+  used for synchronization. For an output buffer, the device MUST copy
+  the \field{timestamp} of the input buffer this output buffer was
+  produced from.
+\item[\field{flags}] marks specific buffers in the sequence with
+  VIRTIO_VIDEO_BUFFER_FLAG_* flags.
+\item[\field{size}] is the data size in the buffer (encoder only).
+\end{description}
+
+\begin{itemize*}
+\item For each VIRTIO_VIDEO_CMD_RESOURCE_QUEUE request, the device
+  MUST send a response to the queue request with
+  VIRTIO_VIDEO_OK_NODATA when it has finished processing the buffer
+  successfully.
+\item The device MUST mark a buffer that triggered a processing error
+  with the VIRTIO_VIDEO_BUFFER_F_ERR flag.
+\item The device MUST mark the last buffer with the
+  VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain
+  sequence.
+\item In case of encoder, to denote a particular frame type the device
+  MUST mark the respective buffer with VIRTIO_VIDEO_BUFFER_IFRAME,
+  VIRTIO_VIDEO_BUFFER_PFRAME, or VIRTIO_VIDEO_BUFFER_BFRAME.
+\item If the processing was stopped due to
+  VIRTIO_VIDEO_CMD_QUEUE_CLEAR, the device MUST respond with
+  VIRTIO_VIDEO_RESP_OK_NODATA as a response type and
+  VIRTIO_VIDEO_BUFFER_FLAG_ERR in \field{flags}.
+\item The driver and device MUST follow requirements about buffer
+  ownership explained in \ref{sec:Device Types / Video Device / Device
+    Operation / Buffer lifecycle}.
+\end{itemize*}
+
+\item[VIRTIO_VIDEO_CMD_STREAM_DRAIN] Ask the device to push all the
+  queued buffers through the pipeline.
+
+\begin{lstlisting}
+struct virtio_video_stream_drain {
+        struct virtio_video_cmd_hdr hdr;
+};
+\end{lstlisting}
+
+\begin{itemize*}
+\item While the device is processing a VIRTIO_VIDEO_CMD_STREAM_DRAIN
+  command, the device MUST return
+  VIRTIO_VIDEO_RESP_ERR_INVALID_OPERATION for incoming commands of
+  VIRTIO_VIDEO_CMD_RESOURCE_QUEUE for input buffers and
+  VIRTIO_VIDEO_CMD_STREAM_DRAIN.
+\item If the processing was stopped due to
+  VIRTIO_VIDEO_CMD_QUEUE_CLEAR, the device MUST respond with
+  VIRTIO_VIDEO_RESP_OK_NODATA as a response type and
+  VIRTIO_VIDEO_BUFFER_FLAG_ERR in \field{flags}.
+\end{itemize*}
+
+\item[VIRTIO_VIDEO_CMD_QUEUE_CLEAR] Return already queued
+  buffers back from the input or the output queue of the device. The
+  device SHOULD return all of the buffers from the respective queue as
+  soon as possible without pushing the buffers through the processing
+  pipeline.
+
+\begin{lstlisting}
+struct virtio_video_resource_queue_clear {
+        struct virtio_video_cmd_hdr hdr;
+        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
+        u8 padding[4];
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{queue_type}] queue type.
+\end{description}
+
+While the device is processing a VIRTIO_VIDEO_CMD_QUEUE_CLEAR command,
+the device MUST return VIRTIO_VIDEO_RESP_ERR_INVALID_OPERATION for
+incoming commands of VIRTIO_VIDEO_CMD_RESOURCE_QUEUE,
+VIRTIO_VIDEO_CMD_STREAM_DRAIN, and VIRTIO_VIDEO_CMD_QUEUE_CLEAR.
+
+\end{description}
+
+\paragraph{Device Operation: Handle stream parameters}
+
+\begin{itemize*}
+\item Use VIRTIO_VIDEO_CMD_GET_PARAMS to get the current stream
+  parameters for input and output streams from the device.
+\item Use VIRTIO_VIDEO_CMD_SET_PARAMS to provide new stream parameters
+  to the device.
+\item After setting stream parameters, the driver may issue
+  VIRTIO_VIDEO_CMD_GET_PARAMS as some parameters of both input and
+  output can be changed implicitly by the device during the set
+  operation.
+\end{itemize*}
+
+\begin{description}
+\item[VIRTIO_VIDEO_CMD_GET_PARAMS] Get parameters of the input or the
+  output of a stream.
+
+\begin{lstlisting}
+struct virtio_video_plane_format {
+        le32 plane_size;
+        le32 stride;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{plane_size}] size of the plane in bytes.
+\item[\field{stride}] stride used for the plane in bytes.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_video_crop {
+        le32 left;
+        le32 top;
+        le32 width;
+        le32 height;
+};
+
+struct virtio_video_params {
+        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
+        le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
+        le32 frame_width;
+        le32 frame_height;
+        le32 min_buffers;
+        le32 max_buffers;
+        struct virtio_video_crop crop;
+        le32 frame_rate;
+        le32 num_planes;
+        struct virtio_video_plane_format plane_formats[VIRTIO_VIDEO_MAX_PLANES];
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{frame_width}] the value to get/set.
+\item[\field{frame_height}] the value to get/set.
+\item[\field{pixel_format}] the value to get/set.
+\item[\field{min_buffers}] minimum buffers required to handle the
+  format (r/o).
+\item[\field{max_buffers}] maximum buffers required to handle the
+  format (r/o).
+\item[\field{crop}] cropping (composing) rectangle.
+\item[\field{frame_rate}] the value to get/set.
+\item[\field{num_planes}] number of planes used to store pixel data
+(r/o).
+\item[\field{plane_formats}] description of each plane.
+\end{description}
+
+\begin{lstlisting}
+struct virtio_video_get_params {
+        struct virtio_video_cmd_hdr hdr;
+        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
+        u8 padding[4];
+};
+
+struct virtio_video_get_params_resp {
+        struct virtio_video_cmd_hdr hdr;
+        struct virtio_video_params params;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{queue_type}] queue type.
+\item[\field{params}] parameter values.
+\end{description}
+
+\item[VIRTIO_VIDEO_CMD_SET_PARAMS] Change parameters of a stream.
+
+\begin{lstlisting}
+struct virtio_video_set_params {
+        struct virtio_video_cmd_hdr hdr;
+        struct virtio_video_params params;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{params}] parameters to set.
+\end{description}
+
+Setting stream parameters might have side effects within the device.
+For example, the device MAY perform alignment of width and height,
+change the number of planes it uses for the format, or do whatever
+changes that are required to continue normal operation using the
+updated parameters. It is up to the driver to check the parameter set
+after the VIRTIO_VIDEO_CMD_SET_PARAMS request has been issued.
+\end{description}
+
+\paragraph{Device Operation: Handle control values}
+
+The driver can query, get and set control values. Though control
+values are associated with a video stream like stream parameters,
+supported values differ depending on the device capabilities and
+formats of videos being processed.
+
+\subparagraph{Commands for controls}
+
+\begin{description}
+\item[VIRTIO_VIDEO_CMD_QUERY_CONTROL] Query supported control values.
+
+\begin{lstlisting}
+struct virtio_video_query_control {
+        struct virtio_video_cmd_hdr hdr;
+        le32 control; /* One of VIRTIO_VIDEO_CONTROL_* types */
+        u8 padding[4];
+};
+
+struct virtio_video_query_control_resp {
+        struct virtio_video_cmd_hdr hdr;
+        /* Followed by one of struct virtio_video_query_control_resp_* */
+};
+\end{lstlisting}
+
+If the device supports a given control, the device MUST return a
+struct that indicates supported control values after sending
+\field{virtio_video_query_control_resp}. The struct used as the
+response differs depending on the value of requested \field{control}.
+Otherwise, the device MUST report an error with
+VIRTIO_VIDEO_RESP_UNSUPPORTED_CONTROL.
+
+\item[VIRTIO_VIDEO_CMD_GET_CONTROL] Get a control value.
+
+\begin{lstlisting}
+struct virtio_video_get_control {
+        struct virtio_video_cmd_hdr hdr;
+        le32 control; /* One of VIRTIO_VIDEO_CONTROL_* types */
+        u8 padding[4];
+};
+
+struct virtio_video_get_control_resp {
+        struct virtio_video_cmd_hdr hdr;
+        /* Followed by one of struct virtio_video_control_val_* */
+};
+\end{lstlisting}
+
+If the device supports a given control and a control value is
+available, the device MUST return a control value after
+\field{virtio_video_get_control_resp}.
+The struct used as the response differs depending on the value of
+requested as \field{control}. If the given control is unsupported, the
+device MUST report an error with
+VIRTIO_VIDEO_RESP_UNSUPPORTED_CONTROL.
+
+\item[VIRTIO_VIDEO_CMD_SET_CONTROL] Set a control value.
+
+\begin{lstlisting}
+struct virtio_video_set_control {
+        struct virtio_video_cmd_hdr hdr;
+        le32 control; /* One of VIRTIO_VIDEO_CONTROL_* types */
+        u8 padding[4];
+        /* Followed by one of struct virtio_video_control_val_* */
+};
+
+struct virtio_video_set_control_resp {
+        struct virtio_video_cmd_hdr hdr;
+};
+\end{lstlisting}
+
+The driver MUST set \field{control} in
+\field{virtio_video_set_control} and send a corresponding struct after
+it. If the given control is unsupported, the device MUST report an
+error with VIRTIO_VIDEO_RESP_UNSUPPORTED_CONTROL.
+\end{description}
+
+\subparagraph{Types of controls}
+
+The types of controls are defined as follows:
+\begin{lstlisting}
+enum virtio_video_control_type {
+        VIRTIO_VIDEO_CONTROL_BITRATE = 1,
+        VIRTIO_VIDEO_CONTROL_PROFILE,
+        VIRTIO_VIDEO_CONTROL_LEVEL,
+};
+\end{lstlisting}
+
+\begin{description}
+\item[VIRTIO_VIDEO_CONTROL_BITRATE] Bitrate of video. (Only for the
+  encoder device.)
+
+The driver can VIRTIO_VIDEO_CMD_GET_CONTROL and
+VIRTIO_VIDEO_CMD_SET_CONTROL to get and set a bitrate for encoding
+respectively.
+The following \field{virtio_video_control_val_bitrate} is used when
+the device returns a value as a response of
+VIRTIO_VIDEO_CMD_GET_CONTROL and when the driver specifies a value to
+be set by VIRTIO_VIDEO_CMD_SET_CONTROL.
+\begin{lstlisting}
+struct virtio_video_control_val_bitrate {
+        le32 bitrate;
+        u8 padding[4];
+};
+\end{lstlisting}
+
+\item[VIRTIO_VIDEO_CONTROL_PROFILE] Profile of a compressed video
+  stream.
+
+VIRTIO_VIDEO_CONTROL_PROFILE is used to handle profiles, which
+represent sets of cabalities for several compressed formats such as
+H.264 and VP9. The enum representing profiles are defined as follows:
+\begin{lstlisting}
+enum virtio_video_profile {
+        /* H.264 */
+        VIRTIO_VIDEO_PROFILE_H264_MIN = 0x100,
+        VIRTIO_VIDEO_PROFILE_H264_BASELINE = VIRTIO_VIDEO_PROFILE_H264_MIN,
+        VIRTIO_VIDEO_PROFILE_H264_MAIN,
+        VIRTIO_VIDEO_PROFILE_H264_EXTENDED,
+        VIRTIO_VIDEO_PROFILE_H264_HIGH,
+        VIRTIO_VIDEO_PROFILE_H264_HIGH10PROFILE,
+        VIRTIO_VIDEO_PROFILE_H264_HIGH422PROFILE,
+        VIRTIO_VIDEO_PROFILE_H264_HIGH444PREDICTIVEPROFILE,
+        VIRTIO_VIDEO_PROFILE_H264_SCALABLEBASELINE,
+        VIRTIO_VIDEO_PROFILE_H264_SCALABLEHIGH,
+        VIRTIO_VIDEO_PROFILE_H264_STEREOHIGH,
+        VIRTIO_VIDEO_PROFILE_H264_MULTIVIEWHIGH,
+        VIRTIO_VIDEO_PROFILE_H264_MAX = VIRTIO_VIDEO_PROFILE_H264_MULTIVIEWHIGH,
+
+        /* HEVC */
+        VIRTIO_VIDEO_PROFILE_HEVC_MIN = 0x200,
+        VIRTIO_VIDEO_PROFILE_HEVC_MAIN = VIRTIO_VIDEO_PROFILE_HEVC_MIN,
+        VIRTIO_VIDEO_PROFILE_HEVC_MAIN10,
+        VIRTIO_VIDEO_PROFILE_HEVC_MAIN_STILL_PICTURE,
+        VIRTIO_VIDEO_PROFILE_HEVC_MAX =
+                VIRTIO_VIDEO_PROFILE_HEVC_MAIN_STILL_PICTURE,
+
+        /* VP8 */
+        VIRTIO_VIDEO_PROFILE_VP8_MIN = 0x300,
+        VIRTIO_VIDEO_PROFILE_VP8_PROFILE0 = VIRTIO_VIDEO_PROFILE_VP8_MIN,
+        VIRTIO_VIDEO_PROFILE_VP8_PROFILE1,
+        VIRTIO_VIDEO_PROFILE_VP8_PROFILE2,
+        VIRTIO_VIDEO_PROFILE_VP8_PROFILE3,
+        VIRTIO_VIDEO_PROFILE_VP8_MAX = VIRTIO_VIDEO_PROFILE_VP8_PROFILE3,
+
+        /* VP9 */
+        VIRTIO_VIDEO_PROFILE_VP9_MIN = 0x400,
+        VIRTIO_VIDEO_PROFILE_VP9_PROFILE0 = VIRTIO_VIDEO_PROFILE_VP9_MIN,
+        VIRTIO_VIDEO_PROFILE_VP9_PROFILE1,
+        VIRTIO_VIDEO_PROFILE_VP9_PROFILE2,
+        VIRTIO_VIDEO_PROFILE_VP9_PROFILE3,
+        VIRTIO_VIDEO_PROFILE_VP9_MAX = VIRTIO_VIDEO_PROFILE_VP9_PROFILE3,
+};
+\end{lstlisting}
+
+The driver can query supported profiles for a compressed format by
+VIRTIO_VIDEO_CMD_QUERY_CONTROL command with
+\field{virtio_video_query_control_profile}. If the device supports the
+given format and the format's profiles are listed in \field{enum
+  virtio_video_profile}, the device MUST returns a list of supported
+profiles as a response
+\field{virtio_video_query_control_resp_profile}.
+\begin{lstlisting}
+struct virtio_video_query_control_profile {
+        le32 format; /* One of VIRTIO_VIDEO_FORMAT_* */
+        u8 padding[4];
+};
+
+struct virtio_video_query_control_resp_profile {
+        le32 num;
+        u8 padding[4];
+        /* Followed by an array le32 profiles[] */
+};
+\end{lstlisting}
+
+The device and the driver use \field{virtio_video_control_val_profile}
+for VIRTIO_VIDEO_CMD_GET_CONTROL and VIRTIO_VIDEO_CMD_SET_CONTROL.
+\begin{lstlisting}
+struct virtio_video_control_val_profile {
+        le32 profile;
+        u8 padding[4];
+};
+\end{lstlisting}
+
+\item[VIRTIO_VIDEO_CONTROL_LEVEL] Level of a compressed video stream.
+
+VIRTIO_VIDEO_CONTROL_LEVEL is used to handle levels of video stream.
+Levels are defined for some compressed video formats to specify sets
+of constraints that indicate a degree of required decoder performance.
+The enum representing levels are defined as follows:
+\begin{lstlisting}
+enum virtio_video_level {
+        /* H.264 */
+        VIRTIO_VIDEO_LEVEL_H264_MIN = 0x100,
+        VIRTIO_VIDEO_LEVEL_H264_1_0 = VIRTIO_VIDEO_LEVEL_H264_MIN,
+        VIRTIO_VIDEO_LEVEL_H264_1_1,
+        VIRTIO_VIDEO_LEVEL_H264_1_2,
+        VIRTIO_VIDEO_LEVEL_H264_1_3,
+        VIRTIO_VIDEO_LEVEL_H264_2_0,
+        VIRTIO_VIDEO_LEVEL_H264_2_1,
+        VIRTIO_VIDEO_LEVEL_H264_2_2,
+        VIRTIO_VIDEO_LEVEL_H264_3_0,
+        VIRTIO_VIDEO_LEVEL_H264_3_1,
+        VIRTIO_VIDEO_LEVEL_H264_3_2,
+        VIRTIO_VIDEO_LEVEL_H264_4_0,
+        VIRTIO_VIDEO_LEVEL_H264_4_1,
+        VIRTIO_VIDEO_LEVEL_H264_4_2,
+        VIRTIO_VIDEO_LEVEL_H264_5_0,
+        VIRTIO_VIDEO_LEVEL_H264_5_1,
+        VIRTIO_VIDEO_LEVEL_H264_MAX = VIRTIO_VIDEO_LEVEL_H264_5_1,
+};
+\end{lstlisting}
+
+The driver can query supported levels for a compressed format by
+VIRTIO_VIDEO_CMD_QUERY_CONTROL command with
+\field{virtio_video_query_control_level}. If the device supports the
+given format and the format's levels are listed in \field{enum
+  virtio_video_level}, the device MUST return a list of supported
+levels as a response \field{virtio_video_query_control_resp_level}.
+\begin{lstlisting}
+struct virtio_video_query_control_level {
+        le32 profile; /* One of VIRTIO_VIDEO_PROFILE_* */
+        u8 padding[4];
+};
+
+struct virtio_video_query_control_resp_level {
+        le32 num;
+        u8 padding[4];
+        /* Followed by an array le32 level[] */
+};
+\end{lstlisting}
+
+The device and the driver use \field{virtio_video_control_val_level}
+for VIRTIO_VIDEO_CMD_GET_CONTROL and VIRTIO_VIDEO_CMD_SET_CONTROL.
+\begin{lstlisting}
+struct virtio_video_control_val_level {
+        le32 level;
+        u8 padding[4];
+};
+\end{lstlisting}
+\end{description}
+
+\subsubsection{Buffer lifecycle}
+\label{sec:Device Types / Video Device / Device Operation / Buffer
+  lifecycle}
+
+The state machine in Figure~\ref{fig:buffer-lifecycle} shows the life
+cycle of a video buffer.
+
+\begin{figure}[h]
+  \centering
+  \includegraphics[width=\textwidth]{images/generated/video-buffer-lifecycle.png}
+  \caption{Lifecycle of a buffer}
+  \label{fig:buffer-lifecycle}
+\end{figure}
+
+\drivernormative{\subparagraph}{Buffer lifecycle}{Device Types / Video
+  Device / Device Operation / Device Operation: Buffer lifecycle}
+
+The following table shows whether the driver can read or write each
+buffer in each state in Figure~\ref{fig:buffer-lifecycle}. The driver
+MUST not read or write buffers in the state that doesn't permit.
+\begin{center}
+  \begin{tabular}{|c|c|c|}
+    \hline
+    State & Input buffers & Output buffers \\
+    \hline
+    Created  & Read / Write & Read \\
+    Queued   & -            & -    \\
+    Dequeued & Read / Write & Read \\
+    \hline
+  \end{tabular}
+\end{center}
+
+\devicenormative{\subparagraph}{Buffer lifecycle}{Device Types / Video
+  Device / Device Operation / Device Operation: Buffer lifecycle}
+
+The following table shows whether the device can read or write each
+buffer in each state in Figure~\ref{fig:buffer-lifecycle}. The device
+MUST not read or write buffers in the state that doesn't permit.
+\begin{center}
+  \begin{tabular}{ |c|c|c| }
+    \hline
+    State & Input buffers & Output buffers \\
+    \hline
+    Created  & -    & - \\
+    Queued   & Read & Read / Write \\
+    Dequeued & -    & Read \\
+    \hline
+  \end{tabular}
+\end{center}
+
+\subsubsection{Event Virtqueue}
+
+While processing buffers, the device can send asynchronous event
+notifications to the driver. The behaviour depends on the exact
+stream. For example, the decoder device sends a resolution change
+event when it encounters new resolution metadata in the stream.
+
+The device can report events on the event queue. The driver initially
+populates the queue with device-writeable buffers. When the device
+needs to report an event, it fills a buffer and notifies the driver.
+The driver consumes the report and adds a new buffer to the virtqueue.
+
+\begin{lstlisting}
+enum virtio_video_event_type {
+        /* For all devices */
+        VIRTIO_VIDEO_EVENT_ERROR = 0x0100,
+
+        /* For decoder only */
+        VIRTIO_VIDEO_EVENT_DECODER_RESOLUTION_CHANGED = 0x0200,
+};
+
+struct virtio_video_event {
+        le32 event_type; /* One of VIRTIO_VIDEO_EVENT_* types */
+        le32 stream_id;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{event_type}] type of the triggered event .
+\item[\field{stream_id}] id of the source stream.
+\end{description}
+
+The device MUST send VIRTIO_VIDEO_EVENT_DECODER_RESOLUTION_CHANGED
+whenever it encounters new resolution data in the stream. This
+includes the case of the initial device configuration after metadata
+has been parsed and the case of dynamic resolution change.
-- 
2.25.0.341.g760bfbb309-goog

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

[Gerd Hoffmann]

On Thu, Feb 06, 2020 at 07:20:57PM +0900, Keiichi Watanabe wrote:
> From: Dmitry Sepp <dmitry.sepp@opensynergy.com>
> 
> 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@opensynergy.com>
> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>

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

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

[Keiichi Watanabe]

Thanks for the review, Gerd.
Please see my replies inline below.

FYI, I'm implementing virtio-video device for ChromeOS that works with
Dmitry's virtio-video driver
https://patchwork.linuxtv.org/patch/61717/.
Once it becomes fully functional, I'll post a list of possible
improvements of protocol.

On Tue, Feb 25, 2020 at 7:00 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Feb 06, 2020 at 07:20:57PM +0900, Keiichi Watanabe wrote:
> > From: Dmitry Sepp <dmitry.sepp@opensynergy.com>
> >
> > 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@opensynergy.com>
> > Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
>
> 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?

While feature flags work well when we "extend" the protocol with an
optional feature, they don't when we want to "drop" or "modify"
features.
For example, I guess it'd be useful when we want:
* to abandon a non-optional command,
* to change a non-optional struct's layout,or
* to change the order of commands in which the device expects to be sent.

Though it might be possible to handle these changes by feature flags,
I suspect the version number allow us to transition protocols more
smoothly.
WDYT?

>
> > +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 ...

Ah, that's a good point. I agree that drivers don't need to loop over formats.
If they need, they can define such an alias locally.

Still, I guess it's worth defining the range for valid raw/coded formats.
This allows devices to report more detailed errors if a driver sent an
unexpected format.
i.e. "opposite format type" v.s. "unknown format"

So, I'd use your idea of RAW_MIN = 0x100 and RAW_MAX = 0x1ff.

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

Nope, it should be profile.
This "profile" field is specified by the driver to query supported
levels for a specific profile.

In my understanding, supported levels depend on profiles.
At least, the specification of H.264 [1] says that `"levels" are
specified within each profile.` at section "0.5 Profiles and levels".

[1] https://www.itu.int/rec/T-REC-H.264-201906-I/en

Best regards,
Keiichi

>
> cheers,
>   Gerd
>

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

[Gerd Hoffmann]

  Hi,

> Dmitry's virtio-video driver
> https://patchwork.linuxtv.org/patch/61717/.
> Once it becomes fully functional, I'll post a list of possible
> improvements of protocol.

Cool.  Actually implementing things can find design problems
in the protocol you didn't notice earlier.

> > > +\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?
> 
> While feature flags work well when we "extend" the protocol with an
> optional feature, they don't when we want to "drop" or "modify"
> features.
> For example, I guess it'd be useful when we want:
> * to abandon a non-optional command,
> * to change a non-optional struct's layout,or
> * to change the order of commands in which the device expects to be sent.
> 
> Though it might be possible to handle these changes by feature flags,
> I suspect the version number allow us to transition protocols more
> smoothly.

Feature flags can be mandatory, both device and driver can fail
initialization when a specific feature is not supported by the other
end.  So in case we did screw up things so badly that we have to
effectively start over (which I hope wouldn't be the case) we can add a
VERSION_2 feature flag for a new set of commands with new structs and
new semantics.

With a feature flag both driver and device can choose whenever they want
support v1 or v2 or both.  With a version config field this is more
limited, the device can't decide to support both.  So the bonus points
for a smooth transition go to the feature flags not the version field ;)

cheers,
  Gerd

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

[Alexandre Courbot]

On Thu, Feb 27, 2020 at 6:29 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > Dmitry's virtio-video driver
> > https://patchwork.linuxtv.org/patch/61717/.
> > Once it becomes fully functional, I'll post a list of possible
> > improvements of protocol.
>
> Cool.  Actually implementing things can find design problems
> in the protocol you didn't notice earlier.
>
> > > > +\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?
> >
> > While feature flags work well when we "extend" the protocol with an
> > optional feature, they don't when we want to "drop" or "modify"
> > features.
> > For example, I guess it'd be useful when we want:
> > * to abandon a non-optional command,
> > * to change a non-optional struct's layout,or
> > * to change the order of commands in which the device expects to be sent.
> >
> > Though it might be possible to handle these changes by feature flags,
> > I suspect the version number allow us to transition protocols more
> > smoothly.
>
> Feature flags can be mandatory, both device and driver can fail
> initialization when a specific feature is not supported by the other
> end.  So in case we did screw up things so badly that we have to
> effectively start over (which I hope wouldn't be the case) we can add a
> VERSION_2 feature flag for a new set of commands with new structs and
> new semantics.
>
> With a feature flag both driver and device can choose whenever they want
> support v1 or v2 or both.  With a version config field this is more
> limited, the device can't decide to support both.  So the bonus points
> for a smooth transition go to the feature flags not the version field ;)

I agree that feature flags would be preferable in general, but I'm
concerned by the fact that there is (IIUC) a limited number of them.
Video tends to change significantly over time, and to have optional
features that would also be presented as feature flags. After a while
we may run out of them, while a new protocol version would allow us to
extend the config struct with some new flags. Or am I missing
something?

I also wonder how "support v1 or v2 or both" would work with feature
flags. In order to make it possible to opt out of v1, I guess we would
need "v1 supported" flag to begin with?

Sorry for the newbie question about feature flags, I'm still in the
process of wrapping my head around virtio. :)

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

[Gerd Hoffmann]

  Hi,

> > With a feature flag both driver and device can choose whenever they want
> > support v1 or v2 or both.  With a version config field this is more
> > limited, the device can't decide to support both.  So the bonus points
> > for a smooth transition go to the feature flags not the version field ;)
> 
> I agree that feature flags would be preferable in general, but I'm
> concerned by the fact that there is (IIUC) a limited number of them.

We have 64 total, some reserved, 24 are available to devices right now,
see https://www.kraxel.org/virtio/virtio-v1.1-cs01-video-v3.html#x1-130002

> Video tends to change significantly over time, and to have optional
> features that would also be presented as feature flags. After a while
> we may run out of them, while a new protocol version would allow us to
> extend the config struct with some new flags. Or am I missing
> something?

Not everything needs a feature flag.  For example we have
VIRTIO_VIDEO_CMD_QUERY_CAPABILITY, and we can add new video formats
without needing a feature flag.  Maybe it is a good idea to explicitly
say in the specs that this can happen and that the driver should simply
ignore any unknown format returned by the device.

> I also wonder how "support v1 or v2 or both" would work with feature
> flags. In order to make it possible to opt out of v1, I guess we would
> need "v1 supported" flag to begin with?

The driver can ignore any device without v2 feature flag set.
The device can refuse to accept a driver without v2 support (don't allow
setting the FEATURES_OK bit).

A explicit v1 feature flag would allow the guest driver print a more
specific error message ("device doesn't support v1" instead of "feature
negotiation failed"), but that's it.

cheers,
  Gerd

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

[Alexandre Courbot]

On Wed, Mar 4, 2020 at 3:43 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > With a feature flag both driver and device can choose whenever they want
> > > support v1 or v2 or both.  With a version config field this is more
> > > limited, the device can't decide to support both.  So the bonus points
> > > for a smooth transition go to the feature flags not the version field ;)
> >
> > I agree that feature flags would be preferable in general, but I'm
> > concerned by the fact that there is (IIUC) a limited number of them.
>
> We have 64 total, some reserved, 24 are available to devices right now,
> see https://www.kraxel.org/virtio/virtio-v1.1-cs01-video-v3.html#x1-130002
>
> > Video tends to change significantly over time, and to have optional
> > features that would also be presented as feature flags. After a while
> > we may run out of them, while a new protocol version would allow us to
> > extend the config struct with some new flags. Or am I missing
> > something?
>
> Not everything needs a feature flag.  For example we have
> VIRTIO_VIDEO_CMD_QUERY_CAPABILITY, and we can add new video formats
> without needing a feature flag.  Maybe it is a good idea to explicitly
> say in the specs that this can happen and that the driver should simply
> ignore any unknown format returned by the device.
>
> > I also wonder how "support v1 or v2 or both" would work with feature
> > flags. In order to make it possible to opt out of v1, I guess we would
> > need "v1 supported" flag to begin with?
>
> The driver can ignore any device without v2 feature flag set.
> The device can refuse to accept a driver without v2 support (don't allow
> setting the FEATURES_OK bit).

That should work as long as we want to add features. I had deprecation
of old features in mind, but maybe the more reasonable answer to that
is "always remain backward compatible". :)

As for the 24 feature flags, I guess that should we run out of them we
can always use the last one to signal that more flags can be found at
the end of the device configuration space.

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

[Keiichi Watanabe]

Hi everyone,

I have implemented a virtio-video device following my v3 spec in
crosvm, which worked well together with Dmitry's driver [1]. I've
started preparing v4 proposal to address problems found while
implementing the driver and the devices.

Regarding v3 protocol, I'm thinking about how we can differentiate
'parameters' and 'controls' in the virtio-video spec?
In the previous discussion, we decided to have a profile, level and
bitrate as controls because we want to query supported values for each
field.
But, I don't think it's a good criteria because it'd be possible to
query other values in params.

So, I'm thinking about what should be the difference between params
and controls. If no difference, we should deprecate
virtio_video_params and have every field there as a control value
instead.
If we add a new command to get and set multiple controls at once, this
change won't cause overhead.

What do you think? Is there anything I missed?
If it sounds fine, I'll remove virtio_video_params from the v4 spec proposal.

Best regards,
Keiichi

[1]: https://patchwork.linuxtv.org/patch/61717/

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

[Dmitry Sepp]

Hi Keiichi,

On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> Hi everyone,
> 
> I have implemented a virtio-video device following my v3 spec in
> crosvm, which worked well together with Dmitry's driver [1]. I've
> started preparing v4 proposal to address problems found while
> implementing the driver and the devices.

Great news!

> 
> Regarding v3 protocol, I'm thinking about how we can differentiate
> 'parameters' and 'controls' in the virtio-video spec?
> In the previous discussion, we decided to have a profile, level and
> bitrate as controls because we want to query supported values for each
> field.
> But, I don't think it's a good criteria because it'd be possible to
> query other values in params.

Could you elaborate on this? Do you now how the design could look like or it 
is just an idea? AFAIR during the discussion of OpenSynergy's original v1 spec 
your point was to separate something that we call 'controls' now to reduce the 
command data size and make command handling less error prone.

On one hand if don't really see any difference in params vs controls it would 
for sure make sense to remove one of the two. On the other hand I'd of course 
like to avoid moving back in forth, especially when it comes to such a major 
driver rework.

> 
> So, I'm thinking about what should be the difference between params
> and controls. If no difference, we should deprecate
> virtio_video_params and have every field there as a control value
> instead.

I deem we should then deprecate controls instead. Params seem to be more 
abstract. Width and height don't sound like a control for me.

> If we add a new command to get and set multiple controls at once, this
> change won't cause overhead.
> 

How would we do this? Provide a flexible array member where each entry has a 
type field first?

What can also make sense is to potentially join set and get calls (probably 
provide 'get' stuff automatically within a response to 'set'). Anyway set and 
get are currently used in conjunction all the time.

Best regards,
Dmitry.

> What do you think? Is there anything I missed?
> If it sounds fine, I'll remove virtio_video_params from the v4 spec
> proposal.
> 
> Best regards,
> Keiichi
> 
> [1]: https://patchwork.linuxtv.org/patch/61717/

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

[Keiichi Watanabe]

Hi,

On Mon, Mar 23, 2020 at 10:28 PM Dmitry Sepp
<dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> > Hi everyone,
> >
> > I have implemented a virtio-video device following my v3 spec in
> > crosvm, which worked well together with Dmitry's driver [1]. I've
> > started preparing v4 proposal to address problems found while
> > implementing the driver and the devices.
>
> Great news!
>
> >
> > Regarding v3 protocol, I'm thinking about how we can differentiate
> > 'parameters' and 'controls' in the virtio-video spec?
> > In the previous discussion, we decided to have a profile, level and
> > bitrate as controls because we want to query supported values for each
> > field.
> > But, I don't think it's a good criteria because it'd be possible to
> > query other values in params.
>
> Could you elaborate on this? Do you now how the design could look like or it
> is just an idea? AFAIR during the discussion of OpenSynergy's original v1 spec
> your point was to separate something that we call 'controls' now to reduce the
> command data size and make command handling less error prone.

The problem in v3 is that if we want to add a new value to be set it'd
be unclear which params or controls is better to be extended.
One possible rule may be "if a value can be queried by the driver, it
should be a control". However, this rule doesn't explain why we have
"format" in params for example. So, I think we need a discussion and
may want to rearrange the structures.

Yeah, in the previous discussion, I suggested to have profile, level
and bitrate as control values instead of members of params. Now, I'm
wondering whether we can have every values as control values.
I don't think it's a perfect idea, but I haven't come up with any
better concrete design yet. So, I'd really appreciate if you could
share your thoughts.

>
> On one hand if don't really see any difference in params vs controls it would
> for sure make sense to remove one of the two. On the other hand I'd of course
> like to avoid moving back in forth, especially when it comes to such a major
> driver rework.

Yes, I understand that it may require a big change in the implementation.
I'm sorry for not being able to think of this point seriously in the
previous thread.

Of course, I'd also really like to avoid rework, but I believe we
shouldn't give up defining a clean and reasonable specification.
Let's find a clear definition in this cycle to avoid future rework.

>
> >
> > So, I'm thinking about what should be the difference between params
> > and controls. If no difference, we should deprecate
> > virtio_video_params and have every field there as a control value
> > instead.
>
> I deem we should then deprecate controls instead. Params seem to be more
> abstract. Width and height don't sound like a control for me.

Though this is actually one of options, how can we query profiles and
levels if they are in params?
This is why we decided them as controls.

Best regards,
Keiichi

>
> > If we add a new command to get and set multiple controls at once, this
> > change won't cause overhead.
> >
>
> How would we do this? Provide a flexible array member where each entry has a
> type field first?

Yeah, something like the idea. But, I haven't designed an actual structure yet.

>
> What can also make sense is to potentially join set and get calls (probably
> provide 'get' stuff automatically within a response to 'set'). Anyway set and
> get are currently used in conjunction all the time.

It'd make sense to return new input and output params when one of
params is updated.
But, if we choose this design, we need to assume one device has just
two params; input and output.

This is okay for video decoder and encoder, but it may become a
problem if we want to support other types of video device that has
only one direction. (e.g. video capture device)
Though we have no plan for supporting this, OpenSynergy's v1 proposal
contained this type IIRC.

Best regards,
Keiichi

>
> Best regards,
> Dmitry.
>
> > What do you think? Is there anything I missed?
> > If it sounds fine, I'll remove virtio_video_params from the v4 spec
> > proposal.
> >
> > Best regards,
> > Keiichi
> >
> > [1]: https://patchwork.linuxtv.org/patch/61717/
>
>

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

[Dmitry Sepp]

Hi Keiichi,

On Montag, 23. März 2020 16:48:14 CET Keiichi Watanabe wrote:
> Hi,
> 
> On Mon, Mar 23, 2020 at 10:28 PM Dmitry Sepp
> 
> <dmitry.sepp@opensynergy.com> wrote:
> > Hi Keiichi,
> > 
> > On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> > > Hi everyone,
> > > 
> > > I have implemented a virtio-video device following my v3 spec in
> > > crosvm, which worked well together with Dmitry's driver [1]. I've
> > > started preparing v4 proposal to address problems found while
> > > implementing the driver and the devices.
> > 
> > Great news!
> > 
> > > Regarding v3 protocol, I'm thinking about how we can differentiate
> > > 'parameters' and 'controls' in the virtio-video spec?
> > > In the previous discussion, we decided to have a profile, level and
> > > bitrate as controls because we want to query supported values for each
> > > field.
> > > But, I don't think it's a good criteria because it'd be possible to
> > > query other values in params.
> > 
> > Could you elaborate on this? Do you now how the design could look like or
> > it is just an idea? AFAIR during the discussion of OpenSynergy's original
> > v1 spec your point was to separate something that we call 'controls' now
> > to reduce the command data size and make command handling less error
> > prone.
> 
> The problem in v3 is that if we want to add a new value to be set it'd
> be unclear which params or controls is better to be extended.
> One possible rule may be "if a value can be queried by the driver, it
> should be a control". However, this rule doesn't explain why we have
> "format" in params for example. So, I think we need a discussion and
> may want to rearrange the structures.
> 
> Yeah, in the previous discussion, I suggested to have profile, level
> and bitrate as control values instead of members of params. Now, I'm
> wondering whether we can have every values as control values.
> I don't think it's a perfect idea, but I haven't come up with any
> better concrete design yet. So, I'd really appreciate if you could
> share your thoughts.
> 

Ok, we can for example add more precise definition to input and output. Let's 
say we have 'bitstream' format structure and a 'image' format structure. E.g. 
for decoder obviously bitstream is input and image is output.

Then instead of params and controls we can define some abstract 'properties'. 
And make some of the properties assigned/mapped/available to bitstream and 
some to image, depending on the current function. I think that could make 
sense as for example for decoder 'bitstream' probably requires very few basic 
'properties' like fourcc format, in contrast to 'image'. But for encoder 
'bitstream' will also have the bitrate 'property' set.

> > On one hand if don't really see any difference in params vs controls it
> > would for sure make sense to remove one of the two. On the other hand I'd
> > of course like to avoid moving back in forth, especially when it comes to
> > such a major driver rework.
> 
> Yes, I understand that it may require a big change in the implementation.
> I'm sorry for not being able to think of this point seriously in the
> previous thread.
> 
> Of course, I'd also really like to avoid rework, but I believe we
> shouldn't give up defining a clean and reasonable specification.
> Let's find a clear definition in this cycle to avoid future rework.
> 
> > > So, I'm thinking about what should be the difference between params
> > > and controls. If no difference, we should deprecate
> > > virtio_video_params and have every field there as a control value
> > > instead.
> > 
> > I deem we should then deprecate controls instead. Params seem to be more
> > abstract. Width and height don't sound like a control for me.
> 
> Though this is actually one of options, how can we query profiles and
> levels if they are in params?
> This is why we decided them as controls.
> 
> Best regards,
> Keiichi
> 
> > > If we add a new command to get and set multiple controls at once, this
> > > change won't cause overhead.
> > 
> > How would we do this? Provide a flexible array member where each entry has
> > a type field first?
> 
> Yeah, something like the idea. But, I haven't designed an actual structure
> yet.
> > What can also make sense is to potentially join set and get calls
> > (probably
> > provide 'get' stuff automatically within a response to 'set'). Anyway set
> > and get are currently used in conjunction all the time.
> 
> It'd make sense to return new input and output params when one of
> params is updated.
> But, if we choose this design, we need to assume one device has just
> two params; input and output.
> 
> This is okay for video decoder and encoder, but it may become a
> problem if we want to support other types of video device that has
> only one direction. (e.g. video capture device)
> Though we have no plan for supporting this, OpenSynergy's v1 proposal
> contained this type IIRC.

Honestly speaking, the idea is not completely abandoned. The spec and the 
driver has more than enough functionality to handle a simple Android EVS 
camera use-case. But I think let's discuss this separately later.

Best regards,
Dmitry.

> 
> Best regards,
> Keiichi
> 
> > Best regards,
> > Dmitry.
> > 
> > > What do you think? Is there anything I missed?
> > > If it sounds fine, I'll remove virtio_video_params from the v4 spec
> > > proposal.
> > > 
> > > Best regards,
> > > Keiichi
> > > 
> > > [1]: https://patchwork.linuxtv.org/patch/61717/

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

[Keiichi Watanabe]

Hi Dmitry,

On Wed, Mar 25, 2020 at 6:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Montag, 23. März 2020 16:48:14 CET Keiichi Watanabe wrote:
> > Hi,
> >
> > On Mon, Mar 23, 2020 at 10:28 PM Dmitry Sepp
> >
> > <dmitry.sepp@opensynergy.com> wrote:
> > > Hi Keiichi,
> > >
> > > On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> > > > Hi everyone,
> > > >
> > > > I have implemented a virtio-video device following my v3 spec in
> > > > crosvm, which worked well together with Dmitry's driver [1]. I've
> > > > started preparing v4 proposal to address problems found while
> > > > implementing the driver and the devices.
> > >
> > > Great news!
> > >
> > > > Regarding v3 protocol, I'm thinking about how we can differentiate
> > > > 'parameters' and 'controls' in the virtio-video spec?
> > > > In the previous discussion, we decided to have a profile, level and
> > > > bitrate as controls because we want to query supported values for each
> > > > field.
> > > > But, I don't think it's a good criteria because it'd be possible to
> > > > query other values in params.
> > >
> > > Could you elaborate on this? Do you now how the design could look like or
> > > it is just an idea? AFAIR during the discussion of OpenSynergy's original
> > > v1 spec your point was to separate something that we call 'controls' now
> > > to reduce the command data size and make command handling less error
> > > prone.
> >
> > The problem in v3 is that if we want to add a new value to be set it'd
> > be unclear which params or controls is better to be extended.
> > One possible rule may be "if a value can be queried by the driver, it
> > should be a control". However, this rule doesn't explain why we have
> > "format" in params for example. So, I think we need a discussion and
> > may want to rearrange the structures.
> >
> > Yeah, in the previous discussion, I suggested to have profile, level
> > and bitrate as control values instead of members of params. Now, I'm
> > wondering whether we can have every values as control values.
> > I don't think it's a perfect idea, but I haven't come up with any
> > better concrete design yet. So, I'd really appreciate if you could
> > share your thoughts.
> >
>
> Ok, we can for example add more precise definition to input and output. Let's
> say we have 'bitstream' format structure and a 'image' format structure. E.g.
> for decoder obviously bitstream is input and image is output.
>
> Then instead of params and controls we can define some abstract 'properties'.
> And make some of the properties assigned/mapped/available to bitstream and
> some to image, depending on the current function. I think that could make
> sense as for example for decoder 'bitstream' probably requires very few basic
> 'properties' like fourcc format, in contrast to 'image'. But for encoder
> 'bitstream' will also have the bitrate 'property' set.

Ah, it sounds like a good idea to have separate structs for bitstreams
and images.
Okay, let me sort out properties based on the idea.

# Bitstream
* format
* min/max number of buffers
* bitrate (encoder only)
* profile (depending on format)
* level (depending on format/profile)

# Image
* format
* min/max number of buffers
* width/height
* crop information
* number of planes
* plane format
* plane layout

Then, we have three categories here:
(a) Mandatory properties for bitstreams for both functions
(b) Mandatory properties for images for both functions
(c) Optional properties for bitstream (e.g. bitrate, profile, level)

So, how about defining structs for each (a), (b) and (c)?

(a) and (b) can be similar to virtio_video_params in v3 spec draft:
e.g.
struct virtio_video_{bitstream, image}_info {
  int format;
  int min_buffers;
  int max_buffers;
  ...
}

(c) would be very similar to struct virtio_video_*_control in v3.
Renaming them to 'properties' would be a nice idea as Dmitry said.

While the designs of structs are not changed from 'params' and
'controls', we now have rules for differentiation at least.
What do you think?

Best regards,
Keiichi



>
> > > On one hand if don't really see any difference in params vs controls it
> > > would for sure make sense to remove one of the two. On the other hand I'd
> > > of course like to avoid moving back in forth, especially when it comes to
> > > such a major driver rework.
> >
> > Yes, I understand that it may require a big change in the implementation.
> > I'm sorry for not being able to think of this point seriously in the
> > previous thread.
> >
> > Of course, I'd also really like to avoid rework, but I believe we
> > shouldn't give up defining a clean and reasonable specification.
> > Let's find a clear definition in this cycle to avoid future rework.
> >
> > > > So, I'm thinking about what should be the difference between params
> > > > and controls. If no difference, we should deprecate
> > > > virtio_video_params and have every field there as a control value
> > > > instead.
> > >
> > > I deem we should then deprecate controls instead. Params seem to be more
> > > abstract. Width and height don't sound like a control for me.
> >
> > Though this is actually one of options, how can we query profiles and
> > levels if they are in params?
> > This is why we decided them as controls.
> >
> > Best regards,
> > Keiichi
> >
> > > > If we add a new command to get and set multiple controls at once, this
> > > > change won't cause overhead.
> > >
> > > How would we do this? Provide a flexible array member where each entry has
> > > a type field first?
> >
> > Yeah, something like the idea. But, I haven't designed an actual structure
> > yet.
> > > What can also make sense is to potentially join set and get calls
> > > (probably
> > > provide 'get' stuff automatically within a response to 'set'). Anyway set
> > > and get are currently used in conjunction all the time.
> >
> > It'd make sense to return new input and output params when one of
> > params is updated.
> > But, if we choose this design, we need to assume one device has just
> > two params; input and output.
> >
> > This is okay for video decoder and encoder, but it may become a
> > problem if we want to support other types of video device that has
> > only one direction. (e.g. video capture device)
> > Though we have no plan for supporting this, OpenSynergy's v1 proposal
> > contained this type IIRC.
>
> Honestly speaking, the idea is not completely abandoned. The spec and the
> driver has more than enough functionality to handle a simple Android EVS
> camera use-case. But I think let's discuss this separately later.
>
> Best regards,
> Dmitry.
>
> >
> > Best regards,
> > Keiichi
> >
> > > Best regards,
> > > Dmitry.
> > >
> > > > What do you think? Is there anything I missed?
> > > > If it sounds fine, I'll remove virtio_video_params from the v4 spec
> > > > proposal.
> > > >
> > > > Best regards,
> > > > Keiichi
> > > >
> > > > [1]: https://patchwork.linuxtv.org/patch/61717/
>
>

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

[Dmitry Sepp]

Hi Keiichi,

On Freitag, 27. März 2020 04:35:13 CEST Keiichi Watanabe wrote:
> Hi Dmitry,
> 
> On Wed, Mar 25, 2020 at 6:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 
wrote:
> > Hi Keiichi,
> > 
> > On Montag, 23. März 2020 16:48:14 CET Keiichi Watanabe wrote:
> > > Hi,
> > > 
> > > On Mon, Mar 23, 2020 at 10:28 PM Dmitry Sepp
> > > 
> > > <dmitry.sepp@opensynergy.com> wrote:
> > > > Hi Keiichi,
> > > > 
> > > > On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> > > > > Hi everyone,
> > > > > 
> > > > > I have implemented a virtio-video device following my v3 spec in
> > > > > crosvm, which worked well together with Dmitry's driver [1]. I've
> > > > > started preparing v4 proposal to address problems found while
> > > > > implementing the driver and the devices.
> > > > 
> > > > Great news!
> > > > 
> > > > > Regarding v3 protocol, I'm thinking about how we can differentiate
> > > > > 'parameters' and 'controls' in the virtio-video spec?
> > > > > In the previous discussion, we decided to have a profile, level and
> > > > > bitrate as controls because we want to query supported values for
> > > > > each
> > > > > field.
> > > > > But, I don't think it's a good criteria because it'd be possible to
> > > > > query other values in params.
> > > > 
> > > > Could you elaborate on this? Do you now how the design could look like
> > > > or
> > > > it is just an idea? AFAIR during the discussion of OpenSynergy's
> > > > original
> > > > v1 spec your point was to separate something that we call 'controls'
> > > > now
> > > > to reduce the command data size and make command handling less error
> > > > prone.
> > > 
> > > The problem in v3 is that if we want to add a new value to be set it'd
> > > be unclear which params or controls is better to be extended.
> > > One possible rule may be "if a value can be queried by the driver, it
> > > should be a control". However, this rule doesn't explain why we have
> > > "format" in params for example. So, I think we need a discussion and
> > > may want to rearrange the structures.
> > > 
> > > Yeah, in the previous discussion, I suggested to have profile, level
> > > and bitrate as control values instead of members of params. Now, I'm
> > > wondering whether we can have every values as control values.
> > > I don't think it's a perfect idea, but I haven't come up with any
> > > better concrete design yet. So, I'd really appreciate if you could
> > > share your thoughts.
> > 
> > Ok, we can for example add more precise definition to input and output.
> > Let's say we have 'bitstream' format structure and a 'image' format
> > structure. E.g. for decoder obviously bitstream is input and image is
> > output.
> > 
> > Then instead of params and controls we can define some abstract
> > 'properties'. And make some of the properties assigned/mapped/available
> > to bitstream and some to image, depending on the current function. I
> > think that could make sense as for example for decoder 'bitstream'
> > probably requires very few basic 'properties' like fourcc format, in
> > contrast to 'image'. But for encoder 'bitstream' will also have the
> > bitrate 'property' set.
> 
> Ah, it sounds like a good idea to have separate structs for bitstreams
> and images.
> Okay, let me sort out properties based on the idea.
> 
> # Bitstream
> * format
> * min/max number of buffers
> * bitrate (encoder only)
Also depending on format?

> * profile (depending on format)
> * level (depending on format/profile)
> 
> # Image
> * format
> * min/max number of buffers
> * width/height
> * crop information
> * number of planes
> * plane format
> * plane layout
* frame rate?

> 
> Then, we have three categories here:
> (a) Mandatory properties for bitstreams for both functions
> (b) Mandatory properties for images for both functions
> (c) Optional properties for bitstream (e.g. bitrate, profile, level)
> 
> So, how about defining structs for each (a), (b) and (c)?
> 
> (a) and (b) can be similar to virtio_video_params in v3 spec draft:
> e.g.
> struct virtio_video_{bitstream, image}_info {
>   int format;
>   int min_buffers;
>   int max_buffers;
>   ...
> }
Yep, this could make sense. Not sure to which of the two the frame rate should 
belong. I'd guess to the image.

We should also thing of a way to set this. Would we have a set/
get_bistream_info() and set/get_image_info()? Plus one generic query/get/
set_properties (can use a flexible array to provide several in one step).

Or probably event better if properties would apply to the whole stream, not to 
bitstream or image in particular. WDYT?

Best regards,
Dmitry.

> 
> (c) would be very similar to struct virtio_video_*_control in v3.
> Renaming them to 'properties' would be a nice idea as Dmitry said.
> 
> While the designs of structs are not changed from 'params' and
> 'controls', we now have rules for differentiation at least.
> What do you think?

> 
> Best regards,
> Keiichi
> 
> > > > On one hand if don't really see any difference in params vs controls
> > > > it
> > > > would for sure make sense to remove one of the two. On the other hand
> > > > I'd
> > > > of course like to avoid moving back in forth, especially when it comes
> > > > to
> > > > such a major driver rework.
> > > 
> > > Yes, I understand that it may require a big change in the
> > > implementation.
> > > I'm sorry for not being able to think of this point seriously in the
> > > previous thread.
> > > 
> > > Of course, I'd also really like to avoid rework, but I believe we
> > > shouldn't give up defining a clean and reasonable specification.
> > > Let's find a clear definition in this cycle to avoid future rework.
> > > 
> > > > > So, I'm thinking about what should be the difference between params
> > > > > and controls. If no difference, we should deprecate
> > > > > virtio_video_params and have every field there as a control value
> > > > > instead.
> > > > 
> > > > I deem we should then deprecate controls instead. Params seem to be
> > > > more
> > > > abstract. Width and height don't sound like a control for me.
> > > 
> > > Though this is actually one of options, how can we query profiles and
> > > levels if they are in params?
> > > This is why we decided them as controls.
> > > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > > If we add a new command to get and set multiple controls at once,
> > > > > this
> > > > > change won't cause overhead.
> > > > 
> > > > How would we do this? Provide a flexible array member where each entry
> > > > has
> > > > a type field first?
> > > 
> > > Yeah, something like the idea. But, I haven't designed an actual
> > > structure
> > > yet.
> > > 
> > > > What can also make sense is to potentially join set and get calls
> > > > (probably
> > > > provide 'get' stuff automatically within a response to 'set'). Anyway
> > > > set
> > > > and get are currently used in conjunction all the time.
> > > 
> > > It'd make sense to return new input and output params when one of
> > > params is updated.
> > > But, if we choose this design, we need to assume one device has just
> > > two params; input and output.
> > > 
> > > This is okay for video decoder and encoder, but it may become a
> > > problem if we want to support other types of video device that has
> > > only one direction. (e.g. video capture device)
> > > Though we have no plan for supporting this, OpenSynergy's v1 proposal
> > > contained this type IIRC.
> > 
> > Honestly speaking, the idea is not completely abandoned. The spec and the
> > driver has more than enough functionality to handle a simple Android EVS
> > camera use-case. But I think let's discuss this separately later.
> > 
> > Best regards,
> > Dmitry.
> > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > Best regards,
> > > > Dmitry.
> > > > 
> > > > > What do you think? Is there anything I missed?
> > > > > If it sounds fine, I'll remove virtio_video_params from the v4 spec
> > > > > proposal.
> > > > > 
> > > > > Best regards,
> > > > > Keiichi
> > > > > 
> > > > > [1]: https://patchwork.linuxtv.org/patch/61717/

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

[Alexandre Courbot]

On Mon, Mar 30, 2020 at 6:54 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Freitag, 27. März 2020 04:35:13 CEST Keiichi Watanabe wrote:
> > Hi Dmitry,
> >
> > On Wed, Mar 25, 2020 at 6:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> wrote:
> > > Hi Keiichi,
> > >
> > > On Montag, 23. März 2020 16:48:14 CET Keiichi Watanabe wrote:
> > > > Hi,
> > > >
> > > > On Mon, Mar 23, 2020 at 10:28 PM Dmitry Sepp
> > > >
> > > > <dmitry.sepp@opensynergy.com> wrote:
> > > > > Hi Keiichi,
> > > > >
> > > > > On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> > > > > > Hi everyone,
> > > > > >
> > > > > > I have implemented a virtio-video device following my v3 spec in
> > > > > > crosvm, which worked well together with Dmitry's driver [1]. I've
> > > > > > started preparing v4 proposal to address problems found while
> > > > > > implementing the driver and the devices.
> > > > >
> > > > > Great news!
> > > > >
> > > > > > Regarding v3 protocol, I'm thinking about how we can differentiate
> > > > > > 'parameters' and 'controls' in the virtio-video spec?
> > > > > > In the previous discussion, we decided to have a profile, level and
> > > > > > bitrate as controls because we want to query supported values for
> > > > > > each
> > > > > > field.
> > > > > > But, I don't think it's a good criteria because it'd be possible to
> > > > > > query other values in params.
> > > > >
> > > > > Could you elaborate on this? Do you now how the design could look like
> > > > > or
> > > > > it is just an idea? AFAIR during the discussion of OpenSynergy's
> > > > > original
> > > > > v1 spec your point was to separate something that we call 'controls'
> > > > > now
> > > > > to reduce the command data size and make command handling less error
> > > > > prone.
> > > >
> > > > The problem in v3 is that if we want to add a new value to be set it'd
> > > > be unclear which params or controls is better to be extended.
> > > > One possible rule may be "if a value can be queried by the driver, it
> > > > should be a control". However, this rule doesn't explain why we have
> > > > "format" in params for example. So, I think we need a discussion and
> > > > may want to rearrange the structures.
> > > >
> > > > Yeah, in the previous discussion, I suggested to have profile, level
> > > > and bitrate as control values instead of members of params. Now, I'm
> > > > wondering whether we can have every values as control values.
> > > > I don't think it's a perfect idea, but I haven't come up with any
> > > > better concrete design yet. So, I'd really appreciate if you could
> > > > share your thoughts.
> > >
> > > Ok, we can for example add more precise definition to input and output.
> > > Let's say we have 'bitstream' format structure and a 'image' format
> > > structure. E.g. for decoder obviously bitstream is input and image is
> > > output.
> > >
> > > Then instead of params and controls we can define some abstract
> > > 'properties'. And make some of the properties assigned/mapped/available
> > > to bitstream and some to image, depending on the current function. I
> > > think that could make sense as for example for decoder 'bitstream'
> > > probably requires very few basic 'properties' like fourcc format, in
> > > contrast to 'image'. But for encoder 'bitstream' will also have the
> > > bitrate 'property' set.
> >
> > Ah, it sounds like a good idea to have separate structs for bitstreams
> > and images.
> > Okay, let me sort out properties based on the idea.
> >
> > # Bitstream
> > * format
> > * min/max number of buffers
> > * bitrate (encoder only)
> Also depending on format?
>
> > * profile (depending on format)
> > * level (depending on format/profile)
> >
> > # Image
> > * format
> > * min/max number of buffers
> > * width/height
> > * crop information
> > * number of planes
> > * plane format
> > * plane layout
> * frame rate?
>
> >
> > Then, we have three categories here:
> > (a) Mandatory properties for bitstreams for both functions
> > (b) Mandatory properties for images for both functions
> > (c) Optional properties for bitstream (e.g. bitrate, profile, level)
> >
> > So, how about defining structs for each (a), (b) and (c)?
> >
> > (a) and (b) can be similar to virtio_video_params in v3 spec draft:
> > e.g.
> > struct virtio_video_{bitstream, image}_info {
> >   int format;
> >   int min_buffers;
> >   int max_buffers;
> >   ...
> > }
> Yep, this could make sense. Not sure to which of the two the frame rate should
> belong. I'd guess to the image.
>
> We should also thing of a way to set this. Would we have a set/
> get_bistream_info() and set/get_image_info()? Plus one generic query/get/
> set_properties (can use a flexible array to provide several in one step).
>
> Or probably event better if properties would apply to the whole stream, not to
> bitstream or image in particular. WDYT?

Sorry for chiming in late.

Merging the controls and parameters into a single class makes sense to
me. Both controls and parameters are values that can be updated by
either the driver or the device, have a default value, and for some of
them are only relevant depending on the value of other controls (e.g.
profile depends on the active codec).

V4L2 needs to differentiate between the two because on top of codecs
it has to be able to manage other kinds of media devices. Since we are
focused on codecs, we don't have this constraint and thus can probably
keep things a bit more unified.

How about merging all the properties/controls into a single structure
which semantics would morph depending on the values of some of its
members? E.g.

struct codec_params {
    u32 frame_width;
    u32 frame_height;
    /* H264, VP8, VP9, etc */
    enum virtio_codec format;
    union {
        struct {
            enum h264_profile profile;
            enum h264_level level;
        } h264;
        struct {
            enum vp8_profile profile;
        } vp8;
        ....
    };
    u32 bitrate;
    ....
};

The idea being that depending on the value of "format", only the
relevant member of the union becomes meaningful. This ensures that the
device/driver does not need to check for every control whether it is
valid in the current context ; it just needs to check what "format" is
and take the values from the relevant members.

I don't think we even need to have a different queue for both structs
(which is what V4L2 does, but again for its own reasons). Just a
single one per coding or decoding context could be enough AFAICT.

Depending on whether we are decoding or encoding, access to some
members would be restricted to read/only for the device or driver. For
instance, when encoding the driver can set "bitrate" to the desired
encoding rate. When decoding, the decoder can report the video's
bitrate there.

Now I'm not sure what would be the best way to share this structure.
Either a memory area shared between the driver and device, with
commands/messages sent to notify that something has changed, or
sending the whole structure with each command/message.

Now the parameters I have listed above are not subject to changing a
lot, but there are also parameters that we may want to specify/be
notified on with each frame. For instance, whether we want a frame to
be forcibly encoded as a keyframe. V4L2 uses a control for this, but
we could probably do better if we can pass this information with each
frame to be encoded. Maybe we can implement that by using different
QUEUE commands for encoder and decoder, or again by using a union.

>
> Best regards,
> Dmitry.
>
> >
> > (c) would be very similar to struct virtio_video_*_control in v3.
> > Renaming them to 'properties' would be a nice idea as Dmitry said.
> >
> > While the designs of structs are not changed from 'params' and
> > 'controls', we now have rules for differentiation at least.
> > What do you think?
>
> >
> > Best regards,
> > Keiichi
> >
> > > > > On one hand if don't really see any difference in params vs controls
> > > > > it
> > > > > would for sure make sense to remove one of the two. On the other hand
> > > > > I'd
> > > > > of course like to avoid moving back in forth, especially when it comes
> > > > > to
> > > > > such a major driver rework.
> > > >
> > > > Yes, I understand that it may require a big change in the
> > > > implementation.
> > > > I'm sorry for not being able to think of this point seriously in the
> > > > previous thread.
> > > >
> > > > Of course, I'd also really like to avoid rework, but I believe we
> > > > shouldn't give up defining a clean and reasonable specification.
> > > > Let's find a clear definition in this cycle to avoid future rework.
> > > >
> > > > > > So, I'm thinking about what should be the difference between params
> > > > > > and controls. If no difference, we should deprecate
> > > > > > virtio_video_params and have every field there as a control value
> > > > > > instead.
> > > > >
> > > > > I deem we should then deprecate controls instead. Params seem to be
> > > > > more
> > > > > abstract. Width and height don't sound like a control for me.
> > > >
> > > > Though this is actually one of options, how can we query profiles and
> > > > levels if they are in params?
> > > > This is why we decided them as controls.
> > > >
> > > > Best regards,
> > > > Keiichi
> > > >
> > > > > > If we add a new command to get and set multiple controls at once,
> > > > > > this
> > > > > > change won't cause overhead.
> > > > >
> > > > > How would we do this? Provide a flexible array member where each entry
> > > > > has
> > > > > a type field first?
> > > >
> > > > Yeah, something like the idea. But, I haven't designed an actual
> > > > structure
> > > > yet.
> > > >
> > > > > What can also make sense is to potentially join set and get calls
> > > > > (probably
> > > > > provide 'get' stuff automatically within a response to 'set'). Anyway
> > > > > set
> > > > > and get are currently used in conjunction all the time.
> > > >
> > > > It'd make sense to return new input and output params when one of
> > > > params is updated.
> > > > But, if we choose this design, we need to assume one device has just
> > > > two params; input and output.
> > > >
> > > > This is okay for video decoder and encoder, but it may become a
> > > > problem if we want to support other types of video device that has
> > > > only one direction. (e.g. video capture device)
> > > > Though we have no plan for supporting this, OpenSynergy's v1 proposal
> > > > contained this type IIRC.
> > >
> > > Honestly speaking, the idea is not completely abandoned. The spec and the
> > > driver has more than enough functionality to handle a simple Android EVS
> > > camera use-case. But I think let's discuss this separately later.
> > >
> > > Best regards,
> > > Dmitry.
> > >
> > > > Best regards,
> > > > Keiichi
> > > >
> > > > > Best regards,
> > > > > Dmitry.
> > > > >
> > > > > > What do you think? Is there anything I missed?
> > > > > > If it sounds fine, I'll remove virtio_video_params from the v4 spec
> > > > > > proposal.
> > > > > >
> > > > > > Best regards,
> > > > > > Keiichi
> > > > > >
> > > > > > [1]: https://patchwork.linuxtv.org/patch/61717/
>
>

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

[Keiichi Watanabe]

Hi,
Thanks Alex for sharing your idea.

On Mon, Apr 6, 2020 at 6:32 PM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Mon, Mar 30, 2020 at 6:54 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> >
> > Hi Keiichi,
> >
> > On Freitag, 27. März 2020 04:35:13 CEST Keiichi Watanabe wrote:
> > > Hi Dmitry,
> > >
> > > On Wed, Mar 25, 2020 at 6:47 PM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> > wrote:
> > > > Hi Keiichi,
> > > >
> > > > On Montag, 23. März 2020 16:48:14 CET Keiichi Watanabe wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, Mar 23, 2020 at 10:28 PM Dmitry Sepp
> > > > >
> > > > > <dmitry.sepp@opensynergy.com> wrote:
> > > > > > Hi Keiichi,
> > > > > >
> > > > > > On Montag, 23. März 2020 13:07:54 CET Keiichi Watanabe wrote:
> > > > > > > Hi everyone,
> > > > > > >
> > > > > > > I have implemented a virtio-video device following my v3 spec in
> > > > > > > crosvm, which worked well together with Dmitry's driver [1]. I've
> > > > > > > started preparing v4 proposal to address problems found while
> > > > > > > implementing the driver and the devices.
> > > > > >
> > > > > > Great news!
> > > > > >
> > > > > > > Regarding v3 protocol, I'm thinking about how we can differentiate
> > > > > > > 'parameters' and 'controls' in the virtio-video spec?
> > > > > > > In the previous discussion, we decided to have a profile, level and
> > > > > > > bitrate as controls because we want to query supported values for
> > > > > > > each
> > > > > > > field.
> > > > > > > But, I don't think it's a good criteria because it'd be possible to
> > > > > > > query other values in params.
> > > > > >
> > > > > > Could you elaborate on this? Do you now how the design could look like
> > > > > > or
> > > > > > it is just an idea? AFAIR during the discussion of OpenSynergy's
> > > > > > original
> > > > > > v1 spec your point was to separate something that we call 'controls'
> > > > > > now
> > > > > > to reduce the command data size and make command handling less error
> > > > > > prone.
> > > > >
> > > > > The problem in v3 is that if we want to add a new value to be set it'd
> > > > > be unclear which params or controls is better to be extended.
> > > > > One possible rule may be "if a value can be queried by the driver, it
> > > > > should be a control". However, this rule doesn't explain why we have
> > > > > "format" in params for example. So, I think we need a discussion and
> > > > > may want to rearrange the structures.
> > > > >
> > > > > Yeah, in the previous discussion, I suggested to have profile, level
> > > > > and bitrate as control values instead of members of params. Now, I'm
> > > > > wondering whether we can have every values as control values.
> > > > > I don't think it's a perfect idea, but I haven't come up with any
> > > > > better concrete design yet. So, I'd really appreciate if you could
> > > > > share your thoughts.
> > > >
> > > > Ok, we can for example add more precise definition to input and output.
> > > > Let's say we have 'bitstream' format structure and a 'image' format
> > > > structure. E.g. for decoder obviously bitstream is input and image is
> > > > output.
> > > >
> > > > Then instead of params and controls we can define some abstract
> > > > 'properties'. And make some of the properties assigned/mapped/available
> > > > to bitstream and some to image, depending on the current function. I
> > > > think that could make sense as for example for decoder 'bitstream'
> > > > probably requires very few basic 'properties' like fourcc format, in
> > > > contrast to 'image'. But for encoder 'bitstream' will also have the
> > > > bitrate 'property' set.
> > >
> > > Ah, it sounds like a good idea to have separate structs for bitstreams
> > > and images.
> > > Okay, let me sort out properties based on the idea.
> > >
> > > # Bitstream
> > > * format
> > > * min/max number of buffers
> > > * bitrate (encoder only)
> > Also depending on format?
> >
> > > * profile (depending on format)
> > > * level (depending on format/profile)
> > >
> > > # Image
> > > * format
> > > * min/max number of buffers
> > > * width/height
> > > * crop information
> > > * number of planes
> > > * plane format
> > > * plane layout
> > * frame rate?
> >
> > >
> > > Then, we have three categories here:
> > > (a) Mandatory properties for bitstreams for both functions
> > > (b) Mandatory properties for images for both functions
> > > (c) Optional properties for bitstream (e.g. bitrate, profile, level)
> > >
> > > So, how about defining structs for each (a), (b) and (c)?
> > >
> > > (a) and (b) can be similar to virtio_video_params in v3 spec draft:
> > > e.g.
> > > struct virtio_video_{bitstream, image}_info {
> > >   int format;
> > >   int min_buffers;
> > >   int max_buffers;
> > >   ...
> > > }
> > Yep, this could make sense. Not sure to which of the two the frame rate should
> > belong. I'd guess to the image.
> >
> > We should also thing of a way to set this. Would we have a set/
> > get_bistream_info() and set/get_image_info()? Plus one generic query/get/
> > set_properties (can use a flexible array to provide several in one step).
> >
> > Or probably event better if properties would apply to the whole stream, not to
> > bitstream or image in particular. WDYT?
>
> Sorry for chiming in late.
>
> Merging the controls and parameters into a single class makes sense to
> me. Both controls and parameters are values that can be updated by
> either the driver or the device, have a default value, and for some of
> them are only relevant depending on the value of other controls (e.g.
> profile depends on the active codec).
>
> V4L2 needs to differentiate between the two because on top of codecs
> it has to be able to manage other kinds of media devices. Since we are
> focused on codecs, we don't have this constraint and thus can probably
> keep things a bit more unified.
>
> How about merging all the properties/controls into a single structure
> which semantics would morph depending on the values of some of its
> members? E.g.

It seems that you're suggesting a big struct where bitstream params,
image params and all controls are merged into, right?
After rethinking, it makes sense to me.
Originally, we had two motivations to have input params and output
params separately:
(1) We could reuse virtio_video_params for both queues.
(2) We have considered extending virtio-video for video capture like V4L2.
However, (1) doesn't seem to be a good idea as per the recent discussion.
Regarding (2), the virtio-video design shouldn't be affected by other
possible use cases. They should be discussed after we finalize
virtio-video design.
Hence, I guess we have no reason to stick to separate params into two.
I'm supportive of this idea now.
Also, having only one struct would simplify updating parameters.

>
> struct codec_params {
>     u32 frame_width;
>     u32 frame_height;
>     /* H264, VP8, VP9, etc */
>     enum virtio_codec format;
>     union {
>         struct {
>             enum h264_profile profile;
>             enum h264_level level;
>         } h264;
>         struct {
>             enum vp8_profile profile;
>         } vp8;
>         ....
>     };
>     u32 bitrate;
>     ....
> };

More specifically, this struct would be:

// the word "codec" might not be needed in the struct name.
struct virtio_video_codec_params {
    // Image format
    enum virtio_video_frame_format frame_format;
    le32 frame_width;
    le32 frame_height;
    le32 min_frame_buffers;
    le32 max_frame_buffers;
    le32 cur_frame_buffers; // It's needed for REQBUFS's "count"
    struct virtio_video_crop crop;
    le32 frame_rate;
    le32 num_planes;
    struct virtio_video_plane_format \
        plane_formats[VIRTIO_VIDEO_MAX_PLANES];

    // Bitstream format
    enum virtio_video_coded_format coded_format;
    le32 min_coded_buffers;
    le32 max_coded_buffers;
    le32 cur_coded_buffers;
    le32 bitrate;
    union {
        struct {
          enum h264_profile profile;
          enum h264_level level;
        } h264;
        struct {
          enum vp8_profile profile;
        } vp8;
        ...
    } codec;
}


>
> The idea being that depending on the value of "format", only the
> relevant member of the union becomes meaningful. This ensures that the
> device/driver does not need to check for every control whether it is
> valid in the current context ; it just needs to check what "format" is
> and take the values from the relevant members.

I like this idea to use union to make it more structured.

>
> I don't think we even need to have a different queue for both structs
> (which is what V4L2 does, but again for its own reasons). Just a
> single one per coding or decoding context could be enough AFAICT.
>
> Depending on whether we are decoding or encoding, access to some
> members would be restricted to read/only for the device or driver. For
> instance, when encoding the driver can set "bitrate" to the desired
> encoding rate. When decoding, the decoder can report the video's
> bitrate there.
>
> Now I'm not sure what would be the best way to share this structure.
> Either a memory area shared between the driver and device, with
> commands/messages sent to notify that something has changed, or
> sending the whole structure with each command/message.

I don't think the first idea is feasible in the virtio mechanism. So,
we can utilize the same way as the previous proposal; SET_PARAMS and
GET_PARAMS.

We also need to think of how to advertise supported profiles and
levels. Probably, we might want to extend struct
virtio_video_format_desc to include supported profiles/levels in a
response of QUERY_CAPABLITY.

>
> Now the parameters I have listed above are not subject to changing a
> lot, but there are also parameters that we may want to specify/be
> notified on with each frame. For instance, whether we want a frame to
> be forcibly encoded as a keyframe. V4L2 uses a control for this, but
> we could probably do better if we can pass this information with each
> frame to be encoded. Maybe we can implement that by using different
> QUEUE commands for encoder and decoder, or again by using a union.

Ah, I haven't come up with such a kind of parameter. Perhaps, we can
extend struct virtio_video_resource_queue to have this flag.

Best regards,
Keiichi

>
> >
> > Best regards,
> > Dmitry.
> >
> > >
> > > (c) would be very similar to struct virtio_video_*_control in v3.
> > > Renaming them to 'properties' would be a nice idea as Dmitry said.
> > >
> > > While the designs of structs are not changed from 'params' and
> > > 'controls', we now have rules for differentiation at least.
> > > What do you think?
> >
> > >
> > > Best regards,
> > > Keiichi
> > >
> > > > > > On one hand if don't really see any difference in params vs controls
> > > > > > it
> > > > > > would for sure make sense to remove one of the two. On the other hand
> > > > > > I'd
> > > > > > of course like to avoid moving back in forth, especially when it comes
> > > > > > to
> > > > > > such a major driver rework.
> > > > >
> > > > > Yes, I understand that it may require a big change in the
> > > > > implementation.
> > > > > I'm sorry for not being able to think of this point seriously in the
> > > > > previous thread.
> > > > >
> > > > > Of course, I'd also really like to avoid rework, but I believe we
> > > > > shouldn't give up defining a clean and reasonable specification.
> > > > > Let's find a clear definition in this cycle to avoid future rework.
> > > > >
> > > > > > > So, I'm thinking about what should be the difference between params
> > > > > > > and controls. If no difference, we should deprecate
> > > > > > > virtio_video_params and have every field there as a control value
> > > > > > > instead.
> > > > > >
> > > > > > I deem we should then deprecate controls instead. Params seem to be
> > > > > > more
> > > > > > abstract. Width and height don't sound like a control for me.
> > > > >
> > > > > Though this is actually one of options, how can we query profiles and
> > > > > levels if they are in params?
> > > > > This is why we decided them as controls.
> > > > >
> > > > > Best regards,
> > > > > Keiichi
> > > > >
> > > > > > > If we add a new command to get and set multiple controls at once,
> > > > > > > this
> > > > > > > change won't cause overhead.
> > > > > >
> > > > > > How would we do this? Provide a flexible array member where each entry
> > > > > > has
> > > > > > a type field first?
> > > > >
> > > > > Yeah, something like the idea. But, I haven't designed an actual
> > > > > structure
> > > > > yet.
> > > > >
> > > > > > What can also make sense is to potentially join set and get calls
> > > > > > (probably
> > > > > > provide 'get' stuff automatically within a response to 'set'). Anyway
> > > > > > set
> > > > > > and get are currently used in conjunction all the time.
> > > > >
> > > > > It'd make sense to return new input and output params when one of
> > > > > params is updated.
> > > > > But, if we choose this design, we need to assume one device has just
> > > > > two params; input and output.
> > > > >
> > > > > This is okay for video decoder and encoder, but it may become a
> > > > > problem if we want to support other types of video device that has
> > > > > only one direction. (e.g. video capture device)
> > > > > Though we have no plan for supporting this, OpenSynergy's v1 proposal
> > > > > contained this type IIRC.
> > > >
> > > > Honestly speaking, the idea is not completely abandoned. The spec and the
> > > > driver has more than enough functionality to handle a simple Android EVS
> > > > camera use-case. But I think let's discuss this separately later.
> > > >
> > > > Best regards,
> > > > Dmitry.
> > > >
> > > > > Best regards,
> > > > > Keiichi
> > > > >
> > > > > > Best regards,
> > > > > > Dmitry.
> > > > > >
> > > > > > > What do you think? Is there anything I missed?
> > > > > > > If it sounds fine, I'll remove virtio_video_params from the v4 spec
> > > > > > > proposal.
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Keiichi
> > > > > > >
> > > > > > > [1]: https://patchwork.linuxtv.org/patch/61717/
> >
> >

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

[Dmitry Sepp]

Hi Alexandre, Keiichi,

Thanks for the updates,

On Montag, 6. April 2020 13:46:33 CEST Keiichi Watanabe wrote:
> 
> It seems that you're suggesting a big struct where bitstream params,
> image params and all controls are merged into, right?
> After rethinking, it makes sense to me.
> Originally, we had two motivations to have input params and output
> params separately:
> (1) We could reuse virtio_video_params for both queues.
> (2) We have considered extending virtio-video for video capture like V4L2.
> However, (1) doesn't seem to be a good idea as per the recent discussion.
> Regarding (2), the virtio-video design shouldn't be affected by other
> possible use cases. They should be discussed after we finalize
> virtio-video design.
> Hence, I guess we have no reason to stick to separate params into two.
> I'm supportive of this idea now.
> Also, having only one struct would simplify updating parameters.
> 
> > struct codec_params {
> > 
> >     u32 frame_width;
> >     u32 frame_height;
> >     /* H264, VP8, VP9, etc */
> >     enum virtio_codec format;
> >     union {
> >     
> >         struct {
> >         
> >             enum h264_profile profile;
> >             enum h264_level level;
> >         
> >         } h264;
> >         struct {
> >         
> >             enum vp8_profile profile;
> >         
> >         } vp8;
> >         ....
> >     
> >     };
> >     u32 bitrate;
> >     ....
> > 
> > };
> 
> More specifically, this struct would be:
> 
> // the word "codec" might not be needed in the struct name.
> struct virtio_video_codec_params {
>     // Image format
>     enum virtio_video_frame_format frame_format;
>     le32 frame_width;
>     le32 frame_height;
>     le32 min_frame_buffers;
>     le32 max_frame_buffers;
>     le32 cur_frame_buffers; // It's needed for REQBUFS's "count"
>     struct virtio_video_crop crop;
>     le32 frame_rate;
>     le32 num_planes;
>     struct virtio_video_plane_format \
>         plane_formats[VIRTIO_VIDEO_MAX_PLANES];
> 
>     // Bitstream format
>     enum virtio_video_coded_format coded_format;
>     le32 min_coded_buffers;
>     le32 max_coded_buffers;
>     le32 cur_coded_buffers;
>     le32 bitrate;
>     union {
>         struct {
>           enum h264_profile profile;
>           enum h264_level level;
>         } h264;
>         struct {
>           enum vp8_profile profile;
>         } vp8;
>         ...
>     } codec;
> }
> 

I would strongly disagree with this approach as it kills the flexibility and 
any possibility of having a uni-directional stream for seemingly no reason.

I could be useful if it was possible to store the structure in the config 
space, but that won't fly as we have multiple streams with different settings. 
Also one device can support multiple formats, so we won't be able to handle 
the unions.

> > The idea being that depending on the value of "format", only the
> > relevant member of the union becomes meaningful. This ensures that the
> > device/driver does not need to check for every control whether it is
> > valid in the current context ; it just needs to check what "format" is
> > and take the values from the relevant members.
> 
> I like this idea to use union to make it more structured.
> 

I don't really have any strong objections agains unions per se, but I deem we 
need to keep the structure flexible. At the very beginning of the development 
there was a discussion about stream priority. If I add a 'prio' field to this 
structure it will break the binary compatibility with older versions.

> > I don't think we even need to have a different queue for both structs
> > (which is what V4L2 does, but again for its own reasons). Just a
> > single one per coding or decoding context could be enough AFAICT.
> > 
> > Depending on whether we are decoding or encoding, access to some
> > members would be restricted to read/only for the device or driver. For
> > instance, when encoding the driver can set "bitrate" to the desired
> > encoding rate. When decoding, the decoder can report the video's
> > bitrate there.
> > 
> > Now I'm not sure what would be the best way to share this structure.
> > Either a memory area shared between the driver and device, with
> > commands/messages sent to notify that something has changed, or
> > sending the whole structure with each command/message.
> 
> I don't think the first idea is feasible in the virtio mechanism. So,
> we can utilize the same way as the previous proposal; SET_PARAMS and
> GET_PARAMS.
> 

For similar thing the virtio config space exists, but as I mentioned above, it 
won't fit the current virtio-video design (or we probably can pre-define the max 
number of streams on the device side and have params for each stream in the 
config space, but this looks clumsy).

> We also need to think of how to advertise supported profiles and
> levels. Probably, we might want to extend struct
> virtio_video_format_desc to include supported profiles/levels in a
> response of QUERY_CAPABLITY.
> 

It would mean back to spec v1 AFAIR. We probably need to recall why we got rid 
of that.

> > Now the parameters I have listed above are not subject to changing a
> > lot, but there are also parameters that we may want to specify/be
> > notified on with each frame. For instance, whether we want a frame to
> > be forcibly encoded as a keyframe. V4L2 uses a control for this, but
> > we could probably do better if we can pass this information with each
> > frame to be encoded. Maybe we can implement that by using different
> > QUEUE commands for encoder and decoder, or again by using a union.
> 
> Ah, I haven't come up with such a kind of parameter. Perhaps, we can
> extend struct virtio_video_resource_queue to have this flag.
> 

This looks sane for me.

Best regards,
Dmitry.

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

[Keiichi Watanabe]

Hi Dmitry,

On Tue, Apr 7, 2020 at 6:21 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Alexandre, Keiichi,
>
> Thanks for the updates,
>
> On Montag, 6. April 2020 13:46:33 CEST Keiichi Watanabe wrote:
> >
> > It seems that you're suggesting a big struct where bitstream params,
> > image params and all controls are merged into, right?
> > After rethinking, it makes sense to me.
> > Originally, we had two motivations to have input params and output
> > params separately:
> > (1) We could reuse virtio_video_params for both queues.
> > (2) We have considered extending virtio-video for video capture like V4L2.
> > However, (1) doesn't seem to be a good idea as per the recent discussion.
> > Regarding (2), the virtio-video design shouldn't be affected by other
> > possible use cases. They should be discussed after we finalize
> > virtio-video design.
> > Hence, I guess we have no reason to stick to separate params into two.
> > I'm supportive of this idea now.
> > Also, having only one struct would simplify updating parameters.
> >
> > > struct codec_params {
> > >
> > >     u32 frame_width;
> > >     u32 frame_height;
> > >     /* H264, VP8, VP9, etc */
> > >     enum virtio_codec format;
> > >     union {
> > >
> > >         struct {
> > >
> > >             enum h264_profile profile;
> > >             enum h264_level level;
> > >
> > >         } h264;
> > >         struct {
> > >
> > >             enum vp8_profile profile;
> > >
> > >         } vp8;
> > >         ....
> > >
> > >     };
> > >     u32 bitrate;
> > >     ....
> > >
> > > };
> >
> > More specifically, this struct would be:
> >
> > // the word "codec" might not be needed in the struct name.
> > struct virtio_video_codec_params {
> >     // Image format
> >     enum virtio_video_frame_format frame_format;
> >     le32 frame_width;
> >     le32 frame_height;
> >     le32 min_frame_buffers;
> >     le32 max_frame_buffers;
> >     le32 cur_frame_buffers; // It's needed for REQBUFS's "count"
> >     struct virtio_video_crop crop;
> >     le32 frame_rate;
> >     le32 num_planes;
> >     struct virtio_video_plane_format \
> >         plane_formats[VIRTIO_VIDEO_MAX_PLANES];
> >
> >     // Bitstream format
> >     enum virtio_video_coded_format coded_format;
> >     le32 min_coded_buffers;
> >     le32 max_coded_buffers;
> >     le32 cur_coded_buffers;
> >     le32 bitrate;
> >     union {
> >         struct {
> >           enum h264_profile profile;
> >           enum h264_level level;
> >         } h264;
> >         struct {
> >           enum vp8_profile profile;
> >         } vp8;
> >         ...
> >     } codec;
> > }
> >
>
> I would strongly disagree with this approach as it kills the flexibility and
> any possibility of having a uni-directional stream for seemingly no reason.
>

I believe it won't be less flexible. For uni-directional stream (if we
want to support it as a part of virtio-video), we can say that the
driver and the device should not use fields for bitstream formats in
the spec.
We already have some encoder-only fields. So, it should be acceptable.
But, I'm not sure if this is the best design.

Currently, we have three options of the design of per-stream properties:

1. Have two structs for image format and bitstream format.
Pros:
Well structured. Easy to support uni-directional stream.
Cons:
Not all properties may not be classified well. For example, bitrate in
encoder is about "how we encode it" rather than "what we encode it
into". So, it may be a bit strange to have it in the bitstream
information.

2. Have one struct that contains all properties.
Pros:
Well structured. Since updating one format affects the other format,
it may make more sense to have everything in one struct.
Also, we can have any per-stream properties there that may be tied to
neither image format nor bitstream format.
Cons:
If we want to support uni-directional streams in the virtio-video
protocol, we may be going to have many unused fields in that struct.

3. Have every property as a separate subcommand like v3's controls
Pros:
Easy to add more properties in the future.
Cons:
Less structured. So, we need to be careful not to overlook mandatory
properties when we implement the driver and device.


IMHO, I'm relatively supportive of 2, but we might need to rethink
whether we really want to support uni-directional streams in
virtio-video.
I guess it's worthwhile to create a separate protocol like
virtio-camera even if it somehow overlaps with virtio-video.
Only for a simple video capture scenario, extending the virtio-video
protocol can be one of simple solutions. However, if we want to extend
it for more features like MIPI cameras, it's not hard to imagine the
protocol becoming very complicated.
I wonder if we can keep virtio protocols simple and clean rather than
making an all-in-one protocol for media devices like V4L2.


> I could be useful if it was possible to store the structure in the config
> space, but that won't fly as we have multiple streams with different settings.
> Also one device can support multiple formats, so we won't be able to handle
> the unions.

Yeah, this structure should be per-stream properties but virtio's
config is for per-device properties.
So, it doesn't work as you said.

>
> > > The idea being that depending on the value of "format", only the
> > > relevant member of the union becomes meaningful. This ensures that the
> > > device/driver does not need to check for every control whether it is
> > > valid in the current context ; it just needs to check what "format" is
> > > and take the values from the relevant members.
> >
> > I like this idea to use union to make it more structured.
> >
>
> I don't really have any strong objections agains unions per se, but I deem we
> need to keep the structure flexible. At the very beginning of the development
> there was a discussion about stream priority. If I add a 'prio' field to this
> structure it will break the binary compatibility with older versions.

Hmm, I don't think unions can incur any extra binary compatibility
issues compared with designs using only structs.
Since alignment rules are well-defined for unions as well as structs,
it'd be okay.
We might want to have an explicit field to show the size of a union like:

union {
  struct {
    ...
  } h264;
  struct {
    ...
  } vp8;
  ...
  u8 _align[MAX_SIZE_OF_FIELDS_IN_THIS_UNION];
}

>
> > > I don't think we even need to have a different queue for both structs
> > > (which is what V4L2 does, but again for its own reasons). Just a
> > > single one per coding or decoding context could be enough AFAICT.
> > >
> > > Depending on whether we are decoding or encoding, access to some
> > > members would be restricted to read/only for the device or driver. For
> > > instance, when encoding the driver can set "bitrate" to the desired
> > > encoding rate. When decoding, the decoder can report the video's
> > > bitrate there.
> > >
> > > Now I'm not sure what would be the best way to share this structure.
> > > Either a memory area shared between the driver and device, with
> > > commands/messages sent to notify that something has changed, or
> > > sending the whole structure with each command/message.
> >
> > I don't think the first idea is feasible in the virtio mechanism. So,
> > we can utilize the same way as the previous proposal; SET_PARAMS and
> > GET_PARAMS.
> >
>
> For similar thing the virtio config space exists, but as I mentioned above, it
> won't fit the current virtio-video design (or we probably can pre-define the max
> number of streams on the device side and have params for each stream in the
> config space, but this looks clumsy).
>
> > We also need to think of how to advertise supported profiles and
> > levels. Probably, we might want to extend struct
> > virtio_video_format_desc to include supported profiles/levels in a
> > response of QUERY_CAPABLITY.
> >
>
> It would mean back to spec v1 AFAIR. We probably need to recall why we got rid
> of that.

Probably, this reply:
https://markmail.org/thread/zr3ycvxixnwi5agt
At that time, I assumed that we'll have profiles/levels/bitrates as
controls, aparting from other per-stream properties. In that
situation, it made sense to have a separate mechanism to get/set/query
these properties.
However, we are likely not to end up distinguishing these properties
from other properties. If so, we don't need any other querying
mechanism other than QUERY_CAPABILITY.

To support profiles, we can extend virtio_video_format_desc to
(a) add fields like "profiles" and "levels" that shows supported
values as bit mask, or
(b) add fields like "num_profiles" and "num_levels" that describes the
lengths of arrays that follows.

My personal preference is (a).

Best regards,
Keiichi

>
> > > Now the parameters I have listed above are not subject to changing a
> > > lot, but there are also parameters that we may want to specify/be
> > > notified on with each frame. For instance, whether we want a frame to
> > > be forcibly encoded as a keyframe. V4L2 uses a control for this, but
> > > we could probably do better if we can pass this information with each
> > > frame to be encoded. Maybe we can implement that by using different
> > > QUEUE commands for encoder and decoder, or again by using a union.
> >
> > Ah, I haven't come up with such a kind of parameter. Perhaps, we can
> > extend struct virtio_video_resource_queue to have this flag.
> >
>
> This looks sane for me.
>
> Best regards,
> Dmitry.
>
>
>

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

[Dmitry Sepp]

Hi,

On Donnerstag, 9. April 2020 12:46:27 CEST Keiichi Watanabe wrote:
> Currently, we have three options of the design of per-stream properties:
> 
> 1. Have two structs for image format and bitstream format.
> Pros:
> Well structured. Easy to support uni-directional stream.
> Cons:
> Not all properties may not be classified well. For example, bitrate in
> encoder is about "how we encode it" rather than "what we encode it
> into". So, it may be a bit strange to have it in the bitstream
> information.
> 
> 2. Have one struct that contains all properties.
> Pros:
> Well structured. Since updating one format affects the other format,
> it may make more sense to have everything in one struct.
> Also, we can have any per-stream properties there that may be tied to
> neither image format nor bitstream format.
> Cons:
> If we want to support uni-directional streams in the virtio-video
> protocol, we may be going to have many unused fields in that struct.
> 
> 3. Have every property as a separate subcommand like v3's controls
> Pros:
> Easy to add more properties in the future.
> Cons:
> Less structured. So, we need to be careful not to overlook mandatory
> properties when we implement the driver and device.
> 
> 
> IMHO, I'm relatively supportive of 2, but we might need to rethink
> whether we really want to support uni-directional streams in
> virtio-video.

Ok, let's assume we keep it is one struct. Anyway, we indeed can just ignore 
some of the fields if we want so. We need to define some conventions for the 
struct. Like whether we should fill all the fields all the time when sending 
set_params() and so on.

I want to ask you about the frame-level bitrate control here [1]. Is it 
planned to support it? If yes, we also need a control to enable that and a way 
to pass minimum and maximum value for the quantization parameter.

> I guess it's worthwhile to create a separate protocol like
> virtio-camera even if it somehow overlaps with virtio-video.
> Only for a simple video capture scenario, extending the virtio-video
> protocol can be one of simple solutions. However, if we want to extend
> it for more features like MIPI cameras, it's not hard to imagine the
> protocol becoming very complicated.
> I wonder if we can keep virtio protocols simple and clean rather than
> making an all-in-one protocol for media devices like V4L2.
> 
> > I could be useful if it was possible to store the structure in the config
> > space, but that won't fly as we have multiple streams with different
> > settings. Also one device can support multiple formats, so we won't be
> > able to handle the unions.
> 
> Yeah, this structure should be per-stream properties but virtio's
> config is for per-device properties.
> So, it doesn't work as you said.
> 
> > > > The idea being that depending on the value of "format", only the
> > > > relevant member of the union becomes meaningful. This ensures that the
> > > > device/driver does not need to check for every control whether it is
> > > > valid in the current context ; it just needs to check what "format" is
> > > > and take the values from the relevant members.
> > > 
> > > I like this idea to use union to make it more structured.
> > 
> > I don't really have any strong objections agains unions per se, but I deem
> > we need to keep the structure flexible. At the very beginning of the
> > development there was a discussion about stream priority. If I add a
> > 'prio' field to this structure it will break the binary compatibility
> > with older versions.
> Hmm, I don't think unions can incur any extra binary compatibility
> issues compared with designs using only structs.
> Since alignment rules are well-defined for unions as well as structs,
> it'd be okay.
> We might want to have an explicit field to show the size of a union like:
> 
> union {
>   struct {
>     ...
>   } h264;
>   struct {
>     ...
>   } vp8;
>   ...
>   u8 _align[MAX_SIZE_OF_FIELDS_IN_THIS_UNION];
> }
> 
> > > > I don't think we even need to have a different queue for both structs
> > > > (which is what V4L2 does, but again for its own reasons). Just a
> > > > single one per coding or decoding context could be enough AFAICT.
> > > > 
> > > > Depending on whether we are decoding or encoding, access to some
> > > > members would be restricted to read/only for the device or driver. For
> > > > instance, when encoding the driver can set "bitrate" to the desired
> > > > encoding rate. When decoding, the decoder can report the video's
> > > > bitrate there.
> > > > 
> > > > Now I'm not sure what would be the best way to share this structure.
> > > > Either a memory area shared between the driver and device, with
> > > > commands/messages sent to notify that something has changed, or
> > > > sending the whole structure with each command/message.
> > > 
> > > I don't think the first idea is feasible in the virtio mechanism. So,
> > > we can utilize the same way as the previous proposal; SET_PARAMS and
> > > GET_PARAMS.
> > 
> > For similar thing the virtio config space exists, but as I mentioned
> > above, it won't fit the current virtio-video design (or we probably can
> > pre-define the max number of streams on the device side and have params
> > for each stream in the config space, but this looks clumsy).
> > 
> > > We also need to think of how to advertise supported profiles and
> > > levels. Probably, we might want to extend struct
> > > virtio_video_format_desc to include supported profiles/levels in a
> > > response of QUERY_CAPABLITY.
> > 
> > It would mean back to spec v1 AFAIR. We probably need to recall why we got
> > rid of that.
> 
> Probably, this reply:
> https://markmail.org/thread/zr3ycvxixnwi5agt
> At that time, I assumed that we'll have profiles/levels/bitrates as
> controls, aparting from other per-stream properties. In that
> situation, it made sense to have a separate mechanism to get/set/query
> these properties.
> However, we are likely not to end up distinguishing these properties
> from other properties. If so, we don't need any other querying
> mechanism other than QUERY_CAPABILITY.
> 
> To support profiles, we can extend virtio_video_format_desc to
> (a) add fields like "profiles" and "levels" that shows supported
> values as bit mask, or
> (b) add fields like "num_profiles" and "num_levels" that describes the
> lengths of arrays that follows.
> 
> My personal preference is (a).
> 

Yes, this should be ok. We had arrays in the spec v1, but as we have now 
bitmasks for formats, we can do so for profiles and levels.

We currently have two problems with capabilities when it comes to the real 
implementation:

1. If we want to avoid calling stream_create in open(), we need to have 
bitrates already on per-format basis in capabilities.
2. We also need to know min input and output buffer count in advance, i.e. it 
should not be in params, especially for encoder that won't report it after 
metadata parsing like decoder, please see [2] (thanks Nicolas Dufresne for 
helping with that issue).

[1] https://github.com/chromium/chromium/blob/master/media/gpu/v4l2/
v4l2_video_encode_accelerator.cc#L1579
[2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/672

Best regards,
Dmitry.

> Best regards,
> Keiichi
> 
> > > > Now the parameters I have listed above are not subject to changing a
> > > > lot, but there are also parameters that we may want to specify/be
> > > > notified on with each frame. For instance, whether we want a frame to
> > > > be forcibly encoded as a keyframe. V4L2 uses a control for this, but
> > > > we could probably do better if we can pass this information with each
> > > > frame to be encoded. Maybe we can implement that by using different
> > > > QUEUE commands for encoder and decoder, or again by using a union.
> > > 
> > > Ah, I haven't come up with such a kind of parameter. Perhaps, we can
> > > extend struct virtio_video_resource_queue to have this flag.
> > 
> > This looks sane for me.
> > 
> > Best regards,
> > Dmitry.

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

[Keiichi Watanabe]

Hi Dmitry,

On Fri, Apr 17, 2020 at 5:09 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi,
>
> On Donnerstag, 9. April 2020 12:46:27 CEST Keiichi Watanabe wrote:
> > Currently, we have three options of the design of per-stream properties:
> >
> > 1. Have two structs for image format and bitstream format.
> > Pros:
> > Well structured. Easy to support uni-directional stream.
> > Cons:
> > Not all properties may not be classified well. For example, bitrate in
> > encoder is about "how we encode it" rather than "what we encode it
> > into". So, it may be a bit strange to have it in the bitstream
> > information.
> >
> > 2. Have one struct that contains all properties.
> > Pros:
> > Well structured. Since updating one format affects the other format,
> > it may make more sense to have everything in one struct.
> > Also, we can have any per-stream properties there that may be tied to
> > neither image format nor bitstream format.
> > Cons:
> > If we want to support uni-directional streams in the virtio-video
> > protocol, we may be going to have many unused fields in that struct.
> >
> > 3. Have every property as a separate subcommand like v3's controls
> > Pros:
> > Easy to add more properties in the future.
> > Cons:
> > Less structured. So, we need to be careful not to overlook mandatory
> > properties when we implement the driver and device.
> >
> >
> > IMHO, I'm relatively supportive of 2, but we might need to rethink
> > whether we really want to support uni-directional streams in
> > virtio-video.
>
> Ok, let's assume we keep it is one struct. Anyway, we indeed can just ignore
> some of the fields if we want so. We need to define some conventions for the
> struct. Like whether we should fill all the fields all the time when sending
> set_params() and so on.

Right. We need to define whether each field in params is (i) either
mandatory, (2) applicable for encoder and decoder and (3) the driver
can modify it.

>
> I want to ask you about the frame-level bitrate control here [1]. Is it
> planned to support it? If yes, we also need a control to enable that and a way
> to pass minimum and maximum value for the quantization parameter.

I have no plan to support this kind of controls as I explained at
https://markmail.org/message/orbtthzxcg3qyzxo.

Even if we want to support max of bitrates doesn't make much sense
because a user can specify any big value as bitrate and encoder will
do its best to make better bitstream in the specified bitrate.
I think this is the reason why we didn't have a QUERY_CONTROL value
for bitrates in v3 spec.

>
> > I guess it's worthwhile to create a separate protocol like
> > virtio-camera even if it somehow overlaps with virtio-video.
> > Only for a simple video capture scenario, extending the virtio-video
> > protocol can be one of simple solutions. However, if we want to extend
> > it for more features like MIPI cameras, it's not hard to imagine the
> > protocol becoming very complicated.
> > I wonder if we can keep virtio protocols simple and clean rather than
> > making an all-in-one protocol for media devices like V4L2.
> >
> > > I could be useful if it was possible to store the structure in the config
> > > space, but that won't fly as we have multiple streams with different
> > > settings. Also one device can support multiple formats, so we won't be
> > > able to handle the unions.
> >
> > Yeah, this structure should be per-stream properties but virtio's
> > config is for per-device properties.
> > So, it doesn't work as you said.
> >
> > > > > The idea being that depending on the value of "format", only the
> > > > > relevant member of the union becomes meaningful. This ensures that the
> > > > > device/driver does not need to check for every control whether it is
> > > > > valid in the current context ; it just needs to check what "format" is
> > > > > and take the values from the relevant members.
> > > >
> > > > I like this idea to use union to make it more structured.
> > >
> > > I don't really have any strong objections agains unions per se, but I deem
> > > we need to keep the structure flexible. At the very beginning of the
> > > development there was a discussion about stream priority. If I add a
> > > 'prio' field to this structure it will break the binary compatibility
> > > with older versions.
> > Hmm, I don't think unions can incur any extra binary compatibility
> > issues compared with designs using only structs.
> > Since alignment rules are well-defined for unions as well as structs,
> > it'd be okay.
> > We might want to have an explicit field to show the size of a union like:
> >
> > union {
> >   struct {
> >     ...
> >   } h264;
> >   struct {
> >     ...
> >   } vp8;
> >   ...
> >   u8 _align[MAX_SIZE_OF_FIELDS_IN_THIS_UNION];
> > }
> >
> > > > > I don't think we even need to have a different queue for both structs
> > > > > (which is what V4L2 does, but again for its own reasons). Just a
> > > > > single one per coding or decoding context could be enough AFAICT.
> > > > >
> > > > > Depending on whether we are decoding or encoding, access to some
> > > > > members would be restricted to read/only for the device or driver. For
> > > > > instance, when encoding the driver can set "bitrate" to the desired
> > > > > encoding rate. When decoding, the decoder can report the video's
> > > > > bitrate there.
> > > > >
> > > > > Now I'm not sure what would be the best way to share this structure.
> > > > > Either a memory area shared between the driver and device, with
> > > > > commands/messages sent to notify that something has changed, or
> > > > > sending the whole structure with each command/message.
> > > >
> > > > I don't think the first idea is feasible in the virtio mechanism. So,
> > > > we can utilize the same way as the previous proposal; SET_PARAMS and
> > > > GET_PARAMS.
> > >
> > > For similar thing the virtio config space exists, but as I mentioned
> > > above, it won't fit the current virtio-video design (or we probably can
> > > pre-define the max number of streams on the device side and have params
> > > for each stream in the config space, but this looks clumsy).
> > >
> > > > We also need to think of how to advertise supported profiles and
> > > > levels. Probably, we might want to extend struct
> > > > virtio_video_format_desc to include supported profiles/levels in a
> > > > response of QUERY_CAPABLITY.
> > >
> > > It would mean back to spec v1 AFAIR. We probably need to recall why we got
> > > rid of that.
> >
> > Probably, this reply:
> > https://markmail.org/thread/zr3ycvxixnwi5agt
> > At that time, I assumed that we'll have profiles/levels/bitrates as
> > controls, aparting from other per-stream properties. In that
> > situation, it made sense to have a separate mechanism to get/set/query
> > these properties.
> > However, we are likely not to end up distinguishing these properties
> > from other properties. If so, we don't need any other querying
> > mechanism other than QUERY_CAPABILITY.
> >
> > To support profiles, we can extend virtio_video_format_desc to
> > (a) add fields like "profiles" and "levels" that shows supported
> > values as bit mask, or
> > (b) add fields like "num_profiles" and "num_levels" that describes the
> > lengths of arrays that follows.
> >
> > My personal preference is (a).
> >
>
> Yes, this should be ok. We had arrays in the spec v1, but as we have now
> bitmasks for formats, we can do so for profiles and levels.
>
> We currently have two problems with capabilities when it comes to the real
> implementation:
>
> 1. If we want to avoid calling stream_create in open(), we need to have
> bitrates already on per-format basis in capabilities.
> 2. We also need to know min input and output buffer count in advance, i.e. it
> should not be in params, especially for encoder that won't report it after
> metadata parsing like decoder, please see [2] (thanks Nicolas Dufresne for
> helping with that issue).

I think these problems wouldn't be problems if we distinguish "guest
driver's internal session" and "virtio-video stream'.
The open() is an operation to create a guest driver's internal
session, not virtio-video stream.
Specifically, in your driver patch, we can define a struct like the
following example for the driver's internal context.

// Represents an internal state of a driver session.
struct virtio_video_context {
  enum virtio_video_context_state;
  struct video-_device *video_dev;
  struct v4l2_fh fh;
  ...

  // The following pointers are NULL at the initialization.
  virtio_video_stream *stream;
  virtio_video_params *params;
};

This struct will be allocated in open(), as it represents a driver's
internal state.
Its fields associated with a virtio-video stream will be initialized
at the time when it's really needed via an accessor function like
this:

// Gets virtio-video stream information from a context.
// If a given context is associated to no virtio-video stream,
// it creates one by the STREAM_CREATE command.
virtio_video_stream* ctx2stream(struct virtio_video_context *ctx)
{
  if (!ctx->stream)
    ctx->stream = virtio_video_cmd_stream_create(...);

  return ctx->stream;
}

In this implementation, we can delay creation of a virtio-video stream
until it's actually needed without considering where it is.


Regarding the second problem you mentioned, I think the min/max number
of buffers _should_ be in params because they're per-stream parameters
and can be changed at the middle of a stream because of resolution
changes, for example.

Best regards,
Keiichi

>
> [1] https://github.com/chromium/chromium/blob/master/media/gpu/v4l2/
> v4l2_video_encode_accelerator.cc#L1579
> [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/672
>
> Best regards,
> Dmitry.
>
> > Best regards,
> > Keiichi
> >
> > > > > Now the parameters I have listed above are not subject to changing a
> > > > > lot, but there are also parameters that we may want to specify/be
> > > > > notified on with each frame. For instance, whether we want a frame to
> > > > > be forcibly encoded as a keyframe. V4L2 uses a control for this, but
> > > > > we could probably do better if we can pass this information with each
> > > > > frame to be encoded. Maybe we can implement that by using different
> > > > > QUEUE commands for encoder and decoder, or again by using a union.
> > > >
> > > > Ah, I haven't come up with such a kind of parameter. Perhaps, we can
> > > > extend struct virtio_video_resource_queue to have this flag.
> > >
> > > This looks sane for me.
> > >
> > > Best regards,
> > > Dmitry.
>
>

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

[Dmitry Sepp]

Hi Keiichi,

On Montag, 20. April 2020 11:57:52 CEST Keiichi Watanabe wrote:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 5:09 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 
wrote:
> > Hi,
> > 
> > On Donnerstag, 9. April 2020 12:46:27 CEST Keiichi Watanabe wrote:
> > > Currently, we have three options of the design of per-stream properties:
> > > 
> > > 1. Have two structs for image format and bitstream format.
> > > Pros:
> > > Well structured. Easy to support uni-directional stream.
> > > Cons:
> > > Not all properties may not be classified well. For example, bitrate in
> > > encoder is about "how we encode it" rather than "what we encode it
> > > into". So, it may be a bit strange to have it in the bitstream
> > > information.
> > > 
> > > 2. Have one struct that contains all properties.
> > > Pros:
> > > Well structured. Since updating one format affects the other format,
> > > it may make more sense to have everything in one struct.
> > > Also, we can have any per-stream properties there that may be tied to
> > > neither image format nor bitstream format.
> > > Cons:
> > > If we want to support uni-directional streams in the virtio-video
> > > protocol, we may be going to have many unused fields in that struct.
> > > 
> > > 3. Have every property as a separate subcommand like v3's controls
> > > Pros:
> > > Easy to add more properties in the future.
> > > Cons:
> > > Less structured. So, we need to be careful not to overlook mandatory
> > > properties when we implement the driver and device.
> > > 
> > > 
> > > IMHO, I'm relatively supportive of 2, but we might need to rethink
> > > whether we really want to support uni-directional streams in
> > > virtio-video.
> > 
> > Ok, let's assume we keep it is one struct. Anyway, we indeed can just
> > ignore some of the fields if we want so. We need to define some
> > conventions for the struct. Like whether we should fill all the fields
> > all the time when sending set_params() and so on.
> 
> Right. We need to define whether each field in params is (i) either
> mandatory, (2) applicable for encoder and decoder and (3) the driver
> can modify it.
> 
> > I want to ask you about the frame-level bitrate control here [1]. Is it
> > planned to support it? If yes, we also need a control to enable that and a
> > way to pass minimum and maximum value for the quantization parameter.
> 
> I have no plan to support this kind of controls as I explained at
> https://markmail.org/message/orbtthzxcg3qyzxo.
> 
> Even if we want to support max of bitrates doesn't make much sense
> because a user can specify any big value as bitrate and encoder will
> do its best to make better bitstream in the specified bitrate.
> I think this is the reason why we didn't have a QUERY_CONTROL value
> for bitrates in v3 spec.
> 

Yes, we had already that discussion, but it does not answer the question. On 
open we need to init controls, bitrate is also there. So according to your 
proposal we should set max bitrate limit to 0xffffffff. Is it correct?

> > > I guess it's worthwhile to create a separate protocol like
> > > virtio-camera even if it somehow overlaps with virtio-video.
> > > Only for a simple video capture scenario, extending the virtio-video
> > > protocol can be one of simple solutions. However, if we want to extend
> > > it for more features like MIPI cameras, it's not hard to imagine the
> > > protocol becoming very complicated.
> > > I wonder if we can keep virtio protocols simple and clean rather than
> > > making an all-in-one protocol for media devices like V4L2.
> > > 
> > > > I could be useful if it was possible to store the structure in the
> > > > config
> > > > space, but that won't fly as we have multiple streams with different
> > > > settings. Also one device can support multiple formats, so we won't be
> > > > able to handle the unions.
> > > 
> > > Yeah, this structure should be per-stream properties but virtio's
> > > config is for per-device properties.
> > > So, it doesn't work as you said.
> > > 
> > > > > > The idea being that depending on the value of "format", only the
> > > > > > relevant member of the union becomes meaningful. This ensures that
> > > > > > the
> > > > > > device/driver does not need to check for every control whether it
> > > > > > is
> > > > > > valid in the current context ; it just needs to check what
> > > > > > "format" is
> > > > > > and take the values from the relevant members.
> > > > > 
> > > > > I like this idea to use union to make it more structured.
> > > > 
> > > > I don't really have any strong objections agains unions per se, but I
> > > > deem
> > > > we need to keep the structure flexible. At the very beginning of the
> > > > development there was a discussion about stream priority. If I add a
> > > > 'prio' field to this structure it will break the binary compatibility
> > > > with older versions.
> > > 
> > > Hmm, I don't think unions can incur any extra binary compatibility
> > > issues compared with designs using only structs.
> > > Since alignment rules are well-defined for unions as well as structs,
> > > it'd be okay.
> > > We might want to have an explicit field to show the size of a union
> > > like:
> > > 
> > > union {
> > > 
> > >   struct {
> > >   
> > >     ...
> > >   
> > >   } h264;
> > >   struct {
> > >   
> > >     ...
> > >   
> > >   } vp8;
> > >   ...
> > >   u8 _align[MAX_SIZE_OF_FIELDS_IN_THIS_UNION];
> > > 
> > > }
> > > 
> > > > > > I don't think we even need to have a different queue for both
> > > > > > structs
> > > > > > (which is what V4L2 does, but again for its own reasons). Just a
> > > > > > single one per coding or decoding context could be enough AFAICT.
> > > > > > 
> > > > > > Depending on whether we are decoding or encoding, access to some
> > > > > > members would be restricted to read/only for the device or driver.
> > > > > > For
> > > > > > instance, when encoding the driver can set "bitrate" to the
> > > > > > desired
> > > > > > encoding rate. When decoding, the decoder can report the video's
> > > > > > bitrate there.
> > > > > > 
> > > > > > Now I'm not sure what would be the best way to share this
> > > > > > structure.
> > > > > > Either a memory area shared between the driver and device, with
> > > > > > commands/messages sent to notify that something has changed, or
> > > > > > sending the whole structure with each command/message.
> > > > > 
> > > > > I don't think the first idea is feasible in the virtio mechanism.
> > > > > So,
> > > > > we can utilize the same way as the previous proposal; SET_PARAMS and
> > > > > GET_PARAMS.
> > > > 
> > > > For similar thing the virtio config space exists, but as I mentioned
> > > > above, it won't fit the current virtio-video design (or we probably
> > > > can
> > > > pre-define the max number of streams on the device side and have
> > > > params
> > > > for each stream in the config space, but this looks clumsy).
> > > > 
> > > > > We also need to think of how to advertise supported profiles and
> > > > > levels. Probably, we might want to extend struct
> > > > > virtio_video_format_desc to include supported profiles/levels in a
> > > > > response of QUERY_CAPABLITY.
> > > > 
> > > > It would mean back to spec v1 AFAIR. We probably need to recall why we
> > > > got
> > > > rid of that.
> > > 
> > > Probably, this reply:
> > > https://markmail.org/thread/zr3ycvxixnwi5agt
> > > At that time, I assumed that we'll have profiles/levels/bitrates as
> > > controls, aparting from other per-stream properties. In that
> > > situation, it made sense to have a separate mechanism to get/set/query
> > > these properties.
> > > However, we are likely not to end up distinguishing these properties
> > > from other properties. If so, we don't need any other querying
> > > mechanism other than QUERY_CAPABILITY.
> > > 
> > > To support profiles, we can extend virtio_video_format_desc to
> > > (a) add fields like "profiles" and "levels" that shows supported
> > > values as bit mask, or
> > > (b) add fields like "num_profiles" and "num_levels" that describes the
> > > lengths of arrays that follows.
> > > 
> > > My personal preference is (a).
> > 
> > Yes, this should be ok. We had arrays in the spec v1, but as we have now
> > bitmasks for formats, we can do so for profiles and levels.
> > 
> > We currently have two problems with capabilities when it comes to the real
> > implementation:
> > 
> > 1. If we want to avoid calling stream_create in open(), we need to have
> > bitrates already on per-format basis in capabilities.
> > 2. We also need to know min input and output buffer count in advance, i.e.
> > it should not be in params, especially for encoder that won't report it
> > after metadata parsing like decoder, please see [2] (thanks Nicolas
> > Dufresne for helping with that issue).
> 
> I think these problems wouldn't be problems if we distinguish "guest
> driver's internal session" and "virtio-video stream'.
> The open() is an operation to create a guest driver's internal
> session, not virtio-video stream.
> Specifically, in your driver patch, we can define a struct like the
> following example for the driver's internal context.
> 
> // Represents an internal state of a driver session.
> struct virtio_video_context {
>   enum virtio_video_context_state;
>   struct video-_device *video_dev;
>   struct v4l2_fh fh;
>   ...
> 
>   // The following pointers are NULL at the initialization.
>   virtio_video_stream *stream;
>   virtio_video_params *params;
> };
> 
> This struct will be allocated in open(), as it represents a driver's
> internal state.
> Its fields associated with a virtio-video stream will be initialized
> at the time when it's really needed via an accessor function like
> this:
> 
> // Gets virtio-video stream information from a context.
> // If a given context is associated to no virtio-video stream,
> // it creates one by the STREAM_CREATE command.
> virtio_video_stream* ctx2stream(struct virtio_video_context *ctx)
> {
>   if (!ctx->stream)
>     ctx->stream = virtio_video_cmd_stream_create(...);
> 
>   return ctx->stream;
> }
> 
> In this implementation, we can delay creation of a virtio-video stream
> until it's actually needed without considering where it is.
> 
> 
> Regarding the second problem you mentioned, I think the min/max number
> of buffers _should_ be in params because they're per-stream parameters
> and can be changed at the middle of a stream because of resolution
> changes, for example.
> 

Thanks for providing this detailed overview. But again, we have already 
discussed this in a similar way and it does not answer the questions. Ok, 
suppose we set bitrate to 0xffffffff as I assumed above. Then the decoder code 
should ideally wait until metadata has been parsed, then query parameters and 
get min buffers (using get_params). Encoder does not have such logic. What 
values should we set for encoder to make sure that the pipelines does not 
stall? Probably people from the Chromium team has better knowledge, if they 
can provide some sane value it'd be just great.

Best regards,
Dmitry.

> Best regards,
> Keiichi
> 
> > [1] https://github.com/chromium/chromium/blob/master/media/gpu/v4l2/
> > v4l2_video_encode_accelerator.cc#L1579
> > [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/672
> > 
> > Best regards,
> > Dmitry.
> > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > > > Now the parameters I have listed above are not subject to changing
> > > > > > a
> > > > > > lot, but there are also parameters that we may want to specify/be
> > > > > > notified on with each frame. For instance, whether we want a frame
> > > > > > to
> > > > > > be forcibly encoded as a keyframe. V4L2 uses a control for this,
> > > > > > but
> > > > > > we could probably do better if we can pass this information with
> > > > > > each
> > > > > > frame to be encoded. Maybe we can implement that by using
> > > > > > different
> > > > > > QUEUE commands for encoder and decoder, or again by using a union.
> > > > > 
> > > > > Ah, I haven't come up with such a kind of parameter. Perhaps, we can
> > > > > extend struct virtio_video_resource_queue to have this flag.
> > > > 
> > > > This looks sane for me.
> > > > 
> > > > Best regards,
> > > > Dmitry.

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

[Keiichi Watanabe]

Hi Dmitry,

On Tue, Apr 21, 2020 at 5:39 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Montag, 20. April 2020 11:57:52 CEST Keiichi Watanabe wrote:
> > Hi Dmitry,
> >
> > On Fri, Apr 17, 2020 at 5:09 PM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> wrote:
> > > Hi,
> > >
> > > On Donnerstag, 9. April 2020 12:46:27 CEST Keiichi Watanabe wrote:
> > > > Currently, we have three options of the design of per-stream properties:
> > > >
> > > > 1. Have two structs for image format and bitstream format.
> > > > Pros:
> > > > Well structured. Easy to support uni-directional stream.
> > > > Cons:
> > > > Not all properties may not be classified well. For example, bitrate in
> > > > encoder is about "how we encode it" rather than "what we encode it
> > > > into". So, it may be a bit strange to have it in the bitstream
> > > > information.
> > > >
> > > > 2. Have one struct that contains all properties.
> > > > Pros:
> > > > Well structured. Since updating one format affects the other format,
> > > > it may make more sense to have everything in one struct.
> > > > Also, we can have any per-stream properties there that may be tied to
> > > > neither image format nor bitstream format.
> > > > Cons:
> > > > If we want to support uni-directional streams in the virtio-video
> > > > protocol, we may be going to have many unused fields in that struct.
> > > >
> > > > 3. Have every property as a separate subcommand like v3's controls
> > > > Pros:
> > > > Easy to add more properties in the future.
> > > > Cons:
> > > > Less structured. So, we need to be careful not to overlook mandatory
> > > > properties when we implement the driver and device.
> > > >
> > > >
> > > > IMHO, I'm relatively supportive of 2, but we might need to rethink
> > > > whether we really want to support uni-directional streams in
> > > > virtio-video.
> > >
> > > Ok, let's assume we keep it is one struct. Anyway, we indeed can just
> > > ignore some of the fields if we want so. We need to define some
> > > conventions for the struct. Like whether we should fill all the fields
> > > all the time when sending set_params() and so on.
> >
> > Right. We need to define whether each field in params is (i) either
> > mandatory, (2) applicable for encoder and decoder and (3) the driver
> > can modify it.
> >
> > > I want to ask you about the frame-level bitrate control here [1]. Is it
> > > planned to support it? If yes, we also need a control to enable that and a
> > > way to pass minimum and maximum value for the quantization parameter.
> >
> > I have no plan to support this kind of controls as I explained at
> > https://markmail.org/message/orbtthzxcg3qyzxo.
> >
> > Even if we want to support max of bitrates doesn't make much sense
> > because a user can specify any big value as bitrate and encoder will
> > do its best to make better bitstream in the specified bitrate.
> > I think this is the reason why we didn't have a QUERY_CONTROL value
> > for bitrates in v3 spec.
> >
>
> Yes, we had already that discussion, but it does not answer the question. On
> open we need to init controls, bitrate is also there. So according to your
> proposal we should set max bitrate limit to 0xffffffff. Is it correct?

Ah, you were talking about the actual placeholder value that we should
pass to v4l2_ctrl_new_std as "max" for V4L2_CID_MPEG_VIDEO_BITRATE.
Yeah, 0xffffffff sounds reasonable.

Or, we may be able to have a field of the "global" maximum bitrate;
max of "max bitrate for a format" for each format.
But, I don't think it doesn't make much sense even though it matches
with V4L2's design.

>
> > > > I guess it's worthwhile to create a separate protocol like
> > > > virtio-camera even if it somehow overlaps with virtio-video.
> > > > Only for a simple video capture scenario, extending the virtio-video
> > > > protocol can be one of simple solutions. However, if we want to extend
> > > > it for more features like MIPI cameras, it's not hard to imagine the
> > > > protocol becoming very complicated.
> > > > I wonder if we can keep virtio protocols simple and clean rather than
> > > > making an all-in-one protocol for media devices like V4L2.
> > > >
> > > > > I could be useful if it was possible to store the structure in the
> > > > > config
> > > > > space, but that won't fly as we have multiple streams with different
> > > > > settings. Also one device can support multiple formats, so we won't be
> > > > > able to handle the unions.
> > > >
> > > > Yeah, this structure should be per-stream properties but virtio's
> > > > config is for per-device properties.
> > > > So, it doesn't work as you said.
> > > >
> > > > > > > The idea being that depending on the value of "format", only the
> > > > > > > relevant member of the union becomes meaningful. This ensures that
> > > > > > > the
> > > > > > > device/driver does not need to check for every control whether it
> > > > > > > is
> > > > > > > valid in the current context ; it just needs to check what
> > > > > > > "format" is
> > > > > > > and take the values from the relevant members.
> > > > > >
> > > > > > I like this idea to use union to make it more structured.
> > > > >
> > > > > I don't really have any strong objections agains unions per se, but I
> > > > > deem
> > > > > we need to keep the structure flexible. At the very beginning of the
> > > > > development there was a discussion about stream priority. If I add a
> > > > > 'prio' field to this structure it will break the binary compatibility
> > > > > with older versions.
> > > >
> > > > Hmm, I don't think unions can incur any extra binary compatibility
> > > > issues compared with designs using only structs.
> > > > Since alignment rules are well-defined for unions as well as structs,
> > > > it'd be okay.
> > > > We might want to have an explicit field to show the size of a union
> > > > like:
> > > >
> > > > union {
> > > >
> > > >   struct {
> > > >
> > > >     ...
> > > >
> > > >   } h264;
> > > >   struct {
> > > >
> > > >     ...
> > > >
> > > >   } vp8;
> > > >   ...
> > > >   u8 _align[MAX_SIZE_OF_FIELDS_IN_THIS_UNION];
> > > >
> > > > }
> > > >
> > > > > > > I don't think we even need to have a different queue for both
> > > > > > > structs
> > > > > > > (which is what V4L2 does, but again for its own reasons). Just a
> > > > > > > single one per coding or decoding context could be enough AFAICT.
> > > > > > >
> > > > > > > Depending on whether we are decoding or encoding, access to some
> > > > > > > members would be restricted to read/only for the device or driver.
> > > > > > > For
> > > > > > > instance, when encoding the driver can set "bitrate" to the
> > > > > > > desired
> > > > > > > encoding rate. When decoding, the decoder can report the video's
> > > > > > > bitrate there.
> > > > > > >
> > > > > > > Now I'm not sure what would be the best way to share this
> > > > > > > structure.
> > > > > > > Either a memory area shared between the driver and device, with
> > > > > > > commands/messages sent to notify that something has changed, or
> > > > > > > sending the whole structure with each command/message.
> > > > > >
> > > > > > I don't think the first idea is feasible in the virtio mechanism.
> > > > > > So,
> > > > > > we can utilize the same way as the previous proposal; SET_PARAMS and
> > > > > > GET_PARAMS.
> > > > >
> > > > > For similar thing the virtio config space exists, but as I mentioned
> > > > > above, it won't fit the current virtio-video design (or we probably
> > > > > can
> > > > > pre-define the max number of streams on the device side and have
> > > > > params
> > > > > for each stream in the config space, but this looks clumsy).
> > > > >
> > > > > > We also need to think of how to advertise supported profiles and
> > > > > > levels. Probably, we might want to extend struct
> > > > > > virtio_video_format_desc to include supported profiles/levels in a
> > > > > > response of QUERY_CAPABLITY.
> > > > >
> > > > > It would mean back to spec v1 AFAIR. We probably need to recall why we
> > > > > got
> > > > > rid of that.
> > > >
> > > > Probably, this reply:
> > > > https://markmail.org/thread/zr3ycvxixnwi5agt
> > > > At that time, I assumed that we'll have profiles/levels/bitrates as
> > > > controls, aparting from other per-stream properties. In that
> > > > situation, it made sense to have a separate mechanism to get/set/query
> > > > these properties.
> > > > However, we are likely not to end up distinguishing these properties
> > > > from other properties. If so, we don't need any other querying
> > > > mechanism other than QUERY_CAPABILITY.
> > > >
> > > > To support profiles, we can extend virtio_video_format_desc to
> > > > (a) add fields like "profiles" and "levels" that shows supported
> > > > values as bit mask, or
> > > > (b) add fields like "num_profiles" and "num_levels" that describes the
> > > > lengths of arrays that follows.
> > > >
> > > > My personal preference is (a).
> > >
> > > Yes, this should be ok. We had arrays in the spec v1, but as we have now
> > > bitmasks for formats, we can do so for profiles and levels.
> > >
> > > We currently have two problems with capabilities when it comes to the real
> > > implementation:
> > >
> > > 1. If we want to avoid calling stream_create in open(), we need to have
> > > bitrates already on per-format basis in capabilities.
> > > 2. We also need to know min input and output buffer count in advance, i.e.
> > > it should not be in params, especially for encoder that won't report it
> > > after metadata parsing like decoder, please see [2] (thanks Nicolas
> > > Dufresne for helping with that issue).
> >
> > I think these problems wouldn't be problems if we distinguish "guest
> > driver's internal session" and "virtio-video stream'.
> > The open() is an operation to create a guest driver's internal
> > session, not virtio-video stream.
> > Specifically, in your driver patch, we can define a struct like the
> > following example for the driver's internal context.
> >
> > // Represents an internal state of a driver session.
> > struct virtio_video_context {
> >   enum virtio_video_context_state;
> >   struct video-_device *video_dev;
> >   struct v4l2_fh fh;
> >   ...
> >
> >   // The following pointers are NULL at the initialization.
> >   virtio_video_stream *stream;
> >   virtio_video_params *params;
> > };
> >
> > This struct will be allocated in open(), as it represents a driver's
> > internal state.
> > Its fields associated with a virtio-video stream will be initialized
> > at the time when it's really needed via an accessor function like
> > this:
> >
> > // Gets virtio-video stream information from a context.
> > // If a given context is associated to no virtio-video stream,
> > // it creates one by the STREAM_CREATE command.
> > virtio_video_stream* ctx2stream(struct virtio_video_context *ctx)
> > {
> >   if (!ctx->stream)
> >     ctx->stream = virtio_video_cmd_stream_create(...);
> >
> >   return ctx->stream;
> > }
> >
> > In this implementation, we can delay creation of a virtio-video stream
> > until it's actually needed without considering where it is.
> >
> >
> > Regarding the second problem you mentioned, I think the min/max number
> > of buffers _should_ be in params because they're per-stream parameters
> > and can be changed at the middle of a stream because of resolution
> > changes, for example.
> >
>
> Thanks for providing this detailed overview. But again, we have already
> discussed this in a similar way and it does not answer the questions. Ok,
> suppose we set bitrate to 0xffffffff as I assumed above. Then the decoder code
> should ideally wait until metadata has been parsed, then query parameters and
> get min buffers (using get_params). Encoder does not have such logic. What
> values should we set for encoder to make sure that the pipelines does not
> stall? Probably people from the Chromium team has better knowledge, if they
> can provide some sane value it'd be just great.

Sorry that my explanation was unclear to you.  Let me try to explain
my idea in another way.

The design I'm proposing can be seen as a kind of "Copy-on-Write"
strategy. The resource creation can be deferred to the first write.
In other words, the call of STREAM_CREATE can be delayed to the first
place where a user makes a change to the stream, instead of open().
The code snippet I wrote above explains a general way of achieving
CoW. In this design, ctx2stream() will be called when modifying a
stream.

In the V4L2 stateful encoder API, the first place in which a user
writes values is the first call of VIDIOC_S_FMT().
So, we can have a virtio_video_cmd_stream_cmd() call in the callback
for VIDIOC_S_FMT. I guess it's even better than my previous proposal
of ctx2stream, as the driver can raise an error when a user calls
ioctls in a wrong order.
(I think I said that the place would be REQBUFS callback in a previous
review comment to your patch. It was my misunderstanding. My
apologies.)

Does it make sense? I hope this answers your question.

Best regards,
Keiichi

>
> Best regards,
> Dmitry.
>
> > Best regards,
> > Keiichi
> >
> > > [1] https://github.com/chromium/chromium/blob/master/media/gpu/v4l2/
> > > v4l2_video_encode_accelerator.cc#L1579
> > > [2] https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/672
> > >
> > > Best regards,
> > > Dmitry.
> > >
> > > > Best regards,
> > > > Keiichi
> > > >
> > > > > > > Now the parameters I have listed above are not subject to changing
> > > > > > > a
> > > > > > > lot, but there are also parameters that we may want to specify/be
> > > > > > > notified on with each frame. For instance, whether we want a frame
> > > > > > > to
> > > > > > > be forcibly encoded as a keyframe. V4L2 uses a control for this,
> > > > > > > but
> > > > > > > we could probably do better if we can pass this information with
> > > > > > > each
> > > > > > > frame to be encoded. Maybe we can implement that by using
> > > > > > > different
> > > > > > > QUEUE commands for encoder and decoder, or again by using a union.
> > > > > >
> > > > > > Ah, I haven't come up with such a kind of parameter. Perhaps, we can
> > > > > > extend struct virtio_video_resource_queue to have this flag.
> > > > >
> > > > > This looks sane for me.
> > > > >
> > > > > Best regards,
> > > > > Dmitry.
>
>

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

[Dmitry Sepp]

Hi Keiichi,

Thanks for the update.

> > Thanks for providing this detailed overview. But again, we have already
> > discussed this in a similar way and it does not answer the questions. Ok,
> > suppose we set bitrate to 0xffffffff as I assumed above. Then the decoder
> > code should ideally wait until metadata has been parsed, then query
> > parameters and get min buffers (using get_params). Encoder does not have
> > such logic. What values should we set for encoder to make sure that the
> > pipelines does not stall? Probably people from the Chromium team has
> > better knowledge, if they can provide some sane value it'd be just great.
> 
> Sorry that my explanation was unclear to you.  Let me try to explain
> my idea in another way.
> 
> The design I'm proposing can be seen as a kind of "Copy-on-Write"
> strategy. The resource creation can be deferred to the first write.
> In other words, the call of STREAM_CREATE can be delayed to the first
> place where a user makes a change to the stream, instead of open().
> The code snippet I wrote above explains a general way of achieving
> CoW. In this design, ctx2stream() will be called when modifying a
> stream.
> 
> In the V4L2 stateful encoder API, the first place in which a user
> writes values is the first call of VIDIOC_S_FMT().
> So, we can have a virtio_video_cmd_stream_cmd() call in the callback
> for VIDIOC_S_FMT. I guess it's even better than my previous proposal
> of ctx2stream, as the driver can raise an error when a user calls
> ioctls in a wrong order.
> (I think I said that the place would be REQBUFS callback in a previous
> review comment to your patch. It was my misunderstanding. My
> apologies.)
> 
> Does it make sense? I hope this answers your question.

I had understood the idea behind the proposal. But I didn't see a correct way 
to implement that. But now it looks different of course. The decoder should 
also be fine if we create a stream on S_FMT.

Best regards,
Dmitry.

> 
> Best regards,
> Keiichi
> 

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

[Dmitry Sepp]

Hi,

> +\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
> +  (context) within the device.
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_destroy {
> +        struct virtio_video_cmd_hdr hdr;
> +};
> +\end{lstlisting}

Let's add more strict description to stream_destroy, like as follows:
Device MUST NOT generate any events for the stream in question after receiving 
the command. Before completing the command, Device MUST ensure that all 
asynchronous commands that are related to the stream have been completed and 
all memory objects are unreferenced.

Best regards,
Dmitry.

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

[Keiichi Watanabe]

Hi,

On Tue, Apr 7, 2020 at 11:49 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi,
>
> > +\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
> > +  (context) within the device.
> > +
> > +\begin{lstlisting}
> > +struct virtio_video_stream_destroy {
> > +        struct virtio_video_cmd_hdr hdr;
> > +};
> > +\end{lstlisting}
>
> Let's add more strict description to stream_destroy, like as follows:
> Device MUST NOT generate any events for the stream in question after receiving
> the command. Before completing the command, Device MUST ensure that all
> asynchronous commands that are related to the stream have been completed and
> all memory objects are unreferenced.

Sounds good. But, the device should probably be able to generate
VIRTIO_VIDEO_EVENT_ERROR for a device-wide error?
Or, should VIRTIO_VIDEO_EVENT_ERROR always be a per-stream error? (I
haven't documented it in v3)

Best regards,
Keiichi

>
> Best regards,
> Dmitry.
>
>
>

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

[Dmitry Sepp]

Hi Keiichi,

On Donnerstag, 9. April 2020 12:46:56 CEST Keiichi Watanabe wrote:
> Hi,
> 
> On Tue, Apr 7, 2020 at 11:49 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 
wrote:
> > Hi,
> > 
> > > +\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
> > > +  (context) within the device.
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_video_stream_destroy {
> > > +        struct virtio_video_cmd_hdr hdr;
> > > +};
> > > +\end{lstlisting}
> > 
> > Let's add more strict description to stream_destroy, like as follows:
> > Device MUST NOT generate any events for the stream in question after
> > receiving the command. Before completing the command, Device MUST ensure
> > that all asynchronous commands that are related to the stream have been
> > completed and all memory objects are unreferenced.
> 
> Sounds good. But, the device should probably be able to generate
> VIRTIO_VIDEO_EVENT_ERROR for a device-wide error?
> Or, should VIRTIO_VIDEO_EVENT_ERROR always be a per-stream error? (I
> haven't documented it in v3)
> 

In the current version of the driver  I have we interpret it a stream error. I 
think it makes sense as several stream formats might be backed by different 
hardware devices on the host side. So it would be an overkill to mark the 
whole virtio device as broken on the guest side.

BTW, I think we should add some hard limit to the max_cap_length and 
max_resp_length in the spec, so buggy device does not make us allocate all 
memory for a response on the host side by providing a garbage value. I think 
4k might be a good value.

Best regards,
Dmitry.

> Best regards,
> Keiichi
> 
> > Best regards,
> > Dmitry.

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

[Keiichi Watanabe]

Hi Dmitry,

On Thu, Apr 9, 2020 at 10:23 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Donnerstag, 9. April 2020 12:46:56 CEST Keiichi Watanabe wrote:
> > Hi,
> >
> > On Tue, Apr 7, 2020 at 11:49 PM Dmitry Sepp <dmitry.sepp@opensynergy.com>
> wrote:
> > > Hi,
> > >
> > > > +\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
> > > > +  (context) within the device.
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_video_stream_destroy {
> > > > +        struct virtio_video_cmd_hdr hdr;
> > > > +};
> > > > +\end{lstlisting}
> > >
> > > Let's add more strict description to stream_destroy, like as follows:
> > > Device MUST NOT generate any events for the stream in question after
> > > receiving the command. Before completing the command, Device MUST ensure
> > > that all asynchronous commands that are related to the stream have been
> > > completed and all memory objects are unreferenced.
> >
> > Sounds good. But, the device should probably be able to generate
> > VIRTIO_VIDEO_EVENT_ERROR for a device-wide error?
> > Or, should VIRTIO_VIDEO_EVENT_ERROR always be a per-stream error? (I
> > haven't documented it in v3)
> >
>
> In the current version of the driver  I have we interpret it a stream error. I
> think it makes sense as several stream formats might be backed by different
> hardware devices on the host side. So it would be an overkill to mark the
> whole virtio device as broken on the guest side.

It makes sense. I'll explicitly document that it's a per-stream error.

>
> BTW, I think we should add some hard limit to the max_cap_length and
> max_resp_length in the spec, so buggy device does not make us allocate all
> memory for a response on the host side by providing a garbage value. I think
> 4k might be a good value.

Sounds good. Thank you for the suggestion.

Best regards,
Keiichi


>
> Best regards,
> Dmitry.
>
> > Best regards,
> > Keiichi
> >
> > > Best regards,
> > > Dmitry.
>
>

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

[Dmitry Sepp]

Hi Keiichi,

One more addition to the comments below. Currently spec does not define units 
for bitrate (are units needed for anything else except that?). Let's 
explicitly state that bitrate is provided in bits per sec, so whatever 
implementation can do proper conversion if needed.

Best regards,
Dmity.

On Freitag, 24. April 2020 13:45:38 CEST Keiichi Watanabe wrote:
> Hi Dmitry,
> 
> On Thu, Apr 9, 2020 at 10:23 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> 
wrote:
> > Hi Keiichi,
> > 
> > On Donnerstag, 9. April 2020 12:46:56 CEST Keiichi Watanabe wrote:
> > > Hi,
> > > 
> > > On Tue, Apr 7, 2020 at 11:49 PM Dmitry Sepp
> > > <dmitry.sepp@opensynergy.com>
> > 
> > wrote:
> > > > Hi,
> > > > 
> > > > > +\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
> > > > > +  (context) within the device.
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_video_stream_destroy {
> > > > > +        struct virtio_video_cmd_hdr hdr;
> > > > > +};
> > > > > +\end{lstlisting}
> > > > 
> > > > Let's add more strict description to stream_destroy, like as follows:
> > > > Device MUST NOT generate any events for the stream in question after
> > > > receiving the command. Before completing the command, Device MUST
> > > > ensure
> > > > that all asynchronous commands that are related to the stream have
> > > > been
> > > > completed and all memory objects are unreferenced.
> > > 
> > > Sounds good. But, the device should probably be able to generate
> > > VIRTIO_VIDEO_EVENT_ERROR for a device-wide error?
> > > Or, should VIRTIO_VIDEO_EVENT_ERROR always be a per-stream error? (I
> > > haven't documented it in v3)
> > 
> > In the current version of the driver  I have we interpret it a stream
> > error. I think it makes sense as several stream formats might be backed
> > by different hardware devices on the host side. So it would be an
> > overkill to mark the whole virtio device as broken on the guest side.
> 
> It makes sense. I'll explicitly document that it's a per-stream error.
> 
> > BTW, I think we should add some hard limit to the max_cap_length and
> > max_resp_length in the spec, so buggy device does not make us allocate all
> > memory for a response on the host side by providing a garbage value. I
> > think 4k might be a good value.
> 
> Sounds good. Thank you for the suggestion.
> 
> Best regards,
> Keiichi
> 
> > Best regards,
> > Dmitry.
> > 
> > > Best regards,
> > > Keiichi
> > > 
> > > > Best regards,
> > > > Dmitry.

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

[Keiichi Watanabe]

Hi,
While I'm writing the next version of this proposal, I came up with
some changes on the protocol. Let me share my ideas in advance. I'd
like to hear your idea if you have another thought.

On Thu, Feb 6, 2020 at 7:21 PM Keiichi Watanabe <keiichiw@chromium.org> wrote:
>
> From: Dmitry Sepp <dmitry.sepp@opensynergy.com>
>
> 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@opensynergy.com>
> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> ---
>  .gitignore                        |   1 +
>  content.tex                       |   1 +
>  images/video-buffer-lifecycle.dot |  18 +
>  make-setup-generated.sh           |   8 +
>  virtio-video.tex                  | 988 ++++++++++++++++++++++++++++++
>  5 files changed, 1016 insertions(+)
>  create mode 100644 .gitignore
>  create mode 100644 images/video-buffer-lifecycle.dot
>  create mode 100644 virtio-video.tex
>
> diff --git a/.gitignore b/.gitignore
> new file mode 100644
> index 0000000..31272c2
> --- /dev/null
> +++ b/.gitignore
> @@ -0,0 +1 @@
> +/images/generated/
> diff --git a/content.tex b/content.tex
> index b91a132..b75a40f 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -6062,6 +6062,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  \input{virtio-fs.tex}
>  \input{virtio-rpmb.tex}
>  \input{virtio-iommu.tex}
> +\input{virtio-video.tex}
>
>  \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
> diff --git a/images/video-buffer-lifecycle.dot b/images/video-buffer-lifecycle.dot
> new file mode 100644
> index 0000000..98f379b
> --- /dev/null
> +++ b/images/video-buffer-lifecycle.dot
> @@ -0,0 +1,18 @@
> +digraph {
> +  graph [ rankdir = LR, layout = dot ];
> +
> +  init [style = invis]
> +  destroyed  [style = invis]
> +  created [label="Created", shape=circle]
> +  dequeued [label="Dequeued", shape=circle]
> +  queued [label="Queued", shape=circle]
> +
> +  init -> created [label="RESOURCE_CREATE"]
> +
> +  created -> queued [label="RESOURCE_QUEUE is sent"]
> +  dequeued -> queued [label="RESOURCE_QUEUE\n is sent"]
> +  queued -> dequeued [label="RESOURCE_QUEUE\n has returned"]
> +
> +  created -> destroyed [label="RESOURCE_DESTROY_ALL"]
> +  dequeued -> destroyed [label="RESOURCE_DESTROY_ALL"]
> +}
> diff --git a/make-setup-generated.sh b/make-setup-generated.sh
> index f15d148..4caff72 100755
> --- a/make-setup-generated.sh
> +++ b/make-setup-generated.sh
> @@ -61,3 +61,11 @@ cat > setup-generated.tex <<EOF
>  \newcommand{\virtiodraftstagename}{$STAGENAME}
>  \newcommand{\virtiodraftoasisstagename}{$OASISSTAGENAME}
>  EOF
> +
> +# Generate PNG from DOT
> +mkdir -p images/generated
> +for file in images/*.dot
> +do
> +    BASENAME=`basename "$file" .dot`
> +    dot -Tpng -o images/generated/${BASENAME}.png ${file}
> +done
> diff --git a/virtio-video.tex b/virtio-video.tex
> new file mode 100644
> index 0000000..2eeee53
> --- /dev/null
> +++ b/virtio-video.tex
> @@ -0,0 +1,988 @@
> +\section{Video Device}\label{sec:Device Types / Video Device}
> +
> +The virtio video encoder device and decoder device are virtual devices
> +that supports encoding and decoding respectively. While the encoder
> +and the decoder are different devices, they use the same protocol.
> +
> +\subsection{Device ID}
> +\label{sec:Device Types / Video Device / Device ID}
> +
> +\begin{description}
> +\item[30] encoder device
> +\item[31] decoder device
> +\end{description}
> +
> +\subsection{Virtqueues}
> +\label{sec:Device Types / Video Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] commandq - queue for sending commands.
> +\item[1] eventq - queue for sending events happened in the device.
> +\end{description}
> +
> +\subsection{Feature bits}
> +\label{sec:Device Types / Video Device / Feature bits}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)] Guest pages can be used
> +  for video buffers.
> +\item[VIRTIO_VIDEO_F_RESOURCE_NON_CONTIG (1)] The device can use
> +  non-contiguous memories for video buffers. Without this flag, the
> +  driver and device MUST use video buffers that are contiguous in the
> +  device-side.
> +\end{description}
> +
> +\devicenormative{\subsubsection}{Feature bits}{Device Types / Video
> +  Device / Feature bits}
> +
> +The device MUST offer VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES.
> +
> +\subsection{Device configuration layout}
> +\label{sec:Device Types / Video Device / Device configuration layout}
> +
> +Video device configuration uses the following layout structure:
> +
> +\begin{lstlisting}
> +struct virtio_video_config {
> +        le32 version;
> +        le32 max_caps_length;
> +        le32 max_resp_length;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{version}] is the protocol version that the device talks.
> +  The device MUST set this to 0.
> +\item[\field{max_caps_length}] defines the maximum length of a
> +  descriptor required to call VIRTIO_VIDEO_CMD_QUERY_CAPABILITY in
> +  bytes. The device MUST set this value.
> +\item[\field{max_resp_length}] defines the maximum length of a
> +  descriptor required to call a command other than
> +  VIRTIO_VIDEO_CMD_QUERY_CAPABILITY in bytes. The device MUST set this
> +  value.
> +\end{description}
> +
> +\subsection{Device Initialization}
> +\label{sec:Device Types / Video Device / Device Initialization}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types /
> +  Video Device / Device Initialization}
> +
> +The driver SHOULD query device capability by using the
> +VIRTIO_VIDEO_CMD_QUERY_CAPABILITY and use that information for the
> +initial setup.
> +
> +\subsection{Device Operation}
> +\label{sec:Device Types / Video Device / Device Operation}
> +
> +The driver allocates input and output buffers and queues the buffers
> +to the device. The device performs operations on the buffers according
> +to the function in question.
> +
> +\subsubsection{Command Virtqueue}
> +
> +\paragraph{Device Operation: Command header}
> +
> +All commands and responses on the command virtqueue have a fixed
> +header using the following layout structure and definitions:
> +
> +\begin{lstlisting}
> +  enum virtio_video_cmd_type {
> +        /* Command */
> +        VIRTIO_VIDEO_CMD_QUERY_CAPABILITY = 0x0100,
> +        VIRTIO_VIDEO_CMD_STREAM_CREATE,
> +        VIRTIO_VIDEO_CMD_STREAM_DESTROY,
> +        VIRTIO_VIDEO_CMD_STREAM_DRAIN,
> +        VIRTIO_VIDEO_CMD_RESOURCE_CREATE,
> +        VIRTIO_VIDEO_CMD_RESOURCE_QUEUE,
> +        VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL,
> +        VIRTIO_VIDEO_CMD_QUEUE_CLEAR,
> +        VIRTIO_VIDEO_CMD_GET_PARAMS,
> +        VIRTIO_VIDEO_CMD_SET_PARAMS,
> +        VIRTIO_VIDEO_CMD_QUERY_CONTROL,
> +        VIRTIO_VIDEO_CMD_GET_CONTROL,
> +        VIRTIO_VIDEO_CMD_SET_CONTROL,
> +
> +        /* Response */
> +        VIRTIO_VIDEO_RESP_OK_NODATA = 0x0200,
> +        VIRTIO_VIDEO_RESP_OK_QUERY_CAPABILITY,
> +        VIRTIO_VIDEO_RESP_OK_RESOURCE_QUEUE,
> +        VIRTIO_VIDEO_RESP_OK_GET_PARAMS,
> +        VIRTIO_VIDEO_RESP_OK_QUERY_CONTROL,
> +        VIRTIO_VIDEO_RESP_OK_GET_CONTROL,
> +
> +        VIRTIO_VIDEO_RESP_ERR_INVALID_OPERATION = 0x0300,
> +        VIRTIO_VIDEO_RESP_ERR_OUT_OF_MEMORY,
> +        VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID,
> +        VIRTIO_VIDEO_RESP_ERR_INVALID_RESOURCE_ID,
> +        VIRTIO_VIDEO_RESP_ERR_INVALID_PARAMETER,
> +        VIRTIO_VIDEO_RESP_ERR_UNSUPPORTED_CONTROL,
> +};
> +
> +struct virtio_video_cmd_hdr {
> +        le32 type;
> +        le32 stream_id;
> +};
> +\end{lstlisting}
> +
> +The fixed header \field{virtio_video_cmd_hdr} in each message includes
> +the following field:
> +\begin{description}
> +\item[\field{type}] specifies the type of the driver command
> +  (VIRTIO_VIDEO_CMD_*) or the device response (VIRTIO_VIDEO_RESP_*).
> +\item[\field{stream_id}] specifies a target stream if needed.
> +\end{description}
> +
> +On success the device will return VIRTIO_VIDEO_RESP_OK_NODATA in case
> +there is no payload. Otherwise the \field{type} field will indicate
> +the kind of payload.
> +
> +On error the device will return one of the VIRTIO_VIDEO_RESP_ERR_*
> +error codes.
> +
> +\paragraph{Device Operation: Query device capability}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CMD_QUERY_CAPABILITY] Retrieve information about
> +supported formats.
> +
> +The driver uses \field{virtio_video_query_capability} to send a
> +query request.
> +
> +\begin{lstlisting}
> +enum virtio_video_queue_type {
> +        VIRTIO_VIDEO_QUEUE_TYPE_INPUT = 0x100,
> +        VIRTIO_VIDEO_QUEUE_TYPE_OUTPUT,
> +};
> +
> +struct virtio_video_query_capability {
> +        struct virtio_video_ctrl_hdr hdr;
> +        le32 queue_type;
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{queue_type}] is the queue type that the driver asks
> +information about. The driver MUST set either
> +\field{VIRTIO_VIDEO_QUEUE_TYPE_INPUT} or
> +\field{VIRTIO_VIDEO_QUEUE_TYPE_OUTPUT}.
> +\end{description}
> +
> +The device responds to VIRTIO_VIDEO_CMD_QUERY_CAPABILITY with
> +\field{virtio_video_query_capability_resp}.
> +\begin{lstlisting}
> +struct virtio_video_query_capability_resp {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 num_descs;
> +        u8 padding[4];
> +        /* Followed by struct virtio_video_format_desc descs[] */
> +};
> +\end{lstlisting}
> +
> +The device MUST return its capability with \field{
> +  virtio_video_capability_resp} that includes the following fields:
> +\begin{description}
> +\item[\field{num_descs}] is a number of \field{virtio_video_format_desc}
> +  that follow. The value MUST not exceed 64.
> +\end{description}
> +
> +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,
> +
> +        /* Coded formats */
> +        VIRTIO_VIDEO_FORMAT_CODED_MIN = 0x1000,
> +        VIRTIO_VIDEO_FORMAT_MPEG2 = VIRTIO_VIDEO_FORMAT_CODED_MIN, /* MPEG-2 Part 2 */
> +        VIRTIO_VIDEO_FORMAT_MPEG4, /* MPEG-4 Part 2 */
> +        VIRTIO_VIDEO_FORMAT_H264, /* H.264 */
> +        VIRTIO_VIDEO_FORMAT_HEVC, /* HEVC aka H.265*/
> +        VIRTIO_VIDEO_FORMAT_VP8, /* VP8 */
> +        VIRTIO_VIDEO_FORMAT_VP9, /* VP9 */
> +        VIRTIO_VIDEO_FORMAT_CODED_MAX = VIRTIO_VIDEO_FORMAT_VP9,
> +};
> +
> +enum virtio_video_planes_layout_flag {
> +        VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER = 1 << 0,
> +        VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE = 1 << 1,
> +};
> +
> +struct virtio_video_format_desc {
> +        le64 mask;
> +        le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> +        le32 planes_layout; /* Bitmask with VIRTIO_VIDEO_PLANES_LAYOUT_* */
> +        le32 plane_align;
> +        le32 num_frames;
> +        /* Followed by struct virtio_video_format_frame frames[] */
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{mask}] is a bitset that represents the supported
> +  combination of input and output format. If \textit{i}-th bit is set
> +  in \field{mask} of \textit{j}-th \field{virtio_video_format_desc}
> +  for input, the device supports encoding or decoding from the
> +  \textit{j}-th input format to \textit{i}-th output format.
> +\item[\field{format}] specifies an image format. The device MUST set
> +  one of \field{enum virtio_video_format}.
> +\item[\field{planes_layout}] is a bitmask representing a set of plane
> +  layout types the device supports. This driver MUST ignore this field
> +  for encoded formats.
> +  \begin{description}
> +  \item[\field{VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER}] The device
> +    expects planes in one buffer laid out one after another.
> +  \item[\field{VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE}] The device
> +    expects planes to be located in separate buffers.
> +  \end{description}
> +\item[\field{plane_align}] is a plane alignment the device require
> +  when multiple planes are located in one buffer. The driver MUST
> +  ignore this field if \field{format} is a bitstream format or
> +  \field{planes_layout} doesn't have
> +  \field{VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER} bit.
> +\item[\field{num_frames}] is the number of
> +  \field{virtio_video_format_frame} that follows.
> +\end{description}
> +
> +The frame information \field{virtio_video_format_frame} is defined as
> +follows:
> +\begin{lstlisting}
> +struct virtio_video_format_range {
> +        le32 min;
> +        le32 max;
> +        le32 step;
> +        u8 padding[4];
> +};
> +
> +struct virtio_video_format_frame {
> +        struct virtio_video_format_range width;
> +        struct virtio_video_format_range height;
> +        le32 num_rates;
> +        u8 padding[4];
> +        /* Followed by struct virtio_video_format_range frame_rates[] */
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{width, height}] represents a range of resolutions
> +  supported by the device. If its \field{step} is not applicable, its
> +  \field{min} is equal to its \field{max}.
> +\item[\field{num_rates}] is the number of
> +  \field{virtio_video_format_ranges} that follows to represent
> +  supported frame rates.
> +\end{description}
> +\end{description}
> +
> +\devicenormative{\subparagraph}{Device Operation: Query device
> +  capability}{Device Types / Video Device / Device Operation / Device
> +  Operation: Query device capability}
> +
> +The total size of the device response MUST not exceed
> +\field{max_cap_length} in bytes reported in the device configuration.
> +
> +\paragraph{Device Operation: Create streams}
> +
> +To process buffers, the device needs to associate them with a certain
> +video stream (essentially, a context). Streams are created by
> +VIRTIO_VIDEO_CMD_STREAM_CREATE with a default set of parameters
> +determined by the device.
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CMD_STREAM_CREATE] Create a video stream (context)
> +  within the device.
> +
> +\begin{lstlisting}
> +enum virtio_video_mem_type {
> +        VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES,
> +};
> +
> +struct virtio_video_stream_create {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> +        le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> +        le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> +        u8 padding[4];
> +        u8 tag[64];
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{in_mem_type, out_mem_type}] is a type of buffer
> +  management for input /output buffers. The driver MUST set a value in
> +  \field{enum virtio_video_mem_type} that the device reported a
> +  corresponding feature bit.
> +\begin{description}
> +\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
> +\end{description}
> +\item[\field{coded_format}] is the encoded format that will be
> +  processed.
> +\item[\field{tag}] is the name associated with this stream. The tag
> +  MUST be encoded in UTF-8 and NUL-terminated.

I wonder why we need this "tag" field. I have kept this field from
Dmitry's first proposal, where this was called "char debug_name[64]".
However, on second thought, I have no idea what is the necessity to
have this field. Our VMM implementation in ChromeOS simply ignores
this field.
If OpenSynergy's implementation relies on this field, I'm curious
about the usage. We might want to have an enum value instead of this
field where arbitrary values can be stored.

> +\end{description}
> +
> +The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
> +to an integer that is not used before. If a used value is passed as
> +\field{stream_id}, the device MUST reports an error with
> +VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.

I'm wondering if we can't generate stream_id in the host side so that
we will have less error control code. In the current design, both the
device and the driver have error checks; the device must check that a
given ID is available and the driver must check if the device didn't
return the INVALID_STREAM_ID error. Instead, by generating IDs in the
device, we will be free from this type of failure. Same for
resource_id in RESOURCE_CREATE.

I guess this design originally came from the virtio-gpu protocol.
However, I couldn't find a benefit of adopting the same design here.

Any feedback is welcome.

Best regards,
Keiichi


> +
> +\item[VIRTIO_VIDEO_CMD_STREAM_DESTROY] Destroy a video stream
> +  (context) within the device.
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_destroy {
> +        struct virtio_video_cmd_hdr hdr;
> +};
> +\end{lstlisting}
> +\end{description}
> +
> +\paragraph{Device Operation: Import resources}
> +
> +\begin{itemize*}
> +\item Use VIRTIO_VIDEO_CMD_RESOURCE_CREATE to import a resource to use
> +  it as a video buffer.
> +\item Use VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL to invalidate all the
> +  resources imported by VIRTIO_VIDEO_CMD_RESOURCE_CREATE so far.
> +\end{itemize*}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CMD_RESOURCE_CREATE] Create a resource descriptor
> +  within the device.
> +
> +The driver sends \field{virtio_video_resource_create} defined as
> +follows:
> +\begin{lstlisting}
> +#define VIRTIO_VIDEO_MAX_PLANES 8
> +
> +struct virtio_video_resource_create {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
> +        le32 resource_id;
> +        le32 planes_layout;
> +        le32 num_planes;
> +        le32 plane_offsets[VIRTIO_VIDEO_MAX_PLANES];
> +        le32 num_entries[VIRTIO_VIDEO_MAX_PLANES];
> +        /* Followed by struct virtio_video_mem_entry entries[] */
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{queue_type}] specifies a direction of queue which the
> +  resource will be used for.
> +\item[\field{resource_id}] is an identifier of the resource. The
> +  driver MUST set it to an integer that is not used before for the
> +  given \field{queue_type}. If the specified \field{resource_id} is
> +  already in use, the device MUST report an error with
> +  VIRTIO_VIDEO_RESP_ERR_INVALID_RESOURCE_ID.
> +\item[\field{planes_layout}] specifies a plane layout. The driver MUST
> +  set only one bit of \field{virtio_video_planes_layout_flag} that is
> +  supported for a current raw format.
> +\item[\field{num_planes}] specifies a number of planes.
> +\item[\field{plane_offsets}] specifies offsets for each plane.
> +\item[\field{num_entries}] is an array of numbers of \field{entries}
> +  memory entries for each plane. If \field{planes_layout} is
> +  VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER, only the first element is
> +  used.
> +\end{description}
> +
> +The \field{virtio_video_resource_create} is followed by an array of
> +\field{virtio_video_mem_entry} defined as follows:
> +\begin{lstlisting}
> +struct virtio_video_mem_entry {
> +        le64 addr;
> +        le32 length;
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{addr}] is the physical guest address.
> +\item[\field{length}] is the length of the resource.
> +\end{description}
> +The number of \field{virtio_video_mem_entry} MUST be equal to the sum
> +of integers in the array \field{num_entries}.
> +
> +\item[VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL] Invalidate all the
> +  resource descriptor created so far.
> +\begin{lstlisting}
> +struct virtio_video_resource_destroy_all {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{queue_type}] is a type of queue.
> +\end{description}
> +\end{description}
> +
> +\paragraph{Device Operation: Process buffers}
> +
> +\begin{itemize*}
> +\item Use VIRTIO_VIDEO_CMD_RESOURCE_QUEUE to queue the resource for
> +  processing in the device. The request completes asynchronously and
> +  out-of-order when the device has finished with the buffer.
> +\item Use VIRTIO_VIDEO_CMD_STREAM_DRAIN to ask the device to process
> +  and return all of the already queued buffers.
> +\item Use VIRTIO_VIDEO_CMD_QUEUE_CLEAR to ask the device to return
> +  back already queued buffers from the input or the output queue. This
> +  also includes input or output buffers that can be currently owned by
> +  the device's processing pipeline.
> +\end{itemize*}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CMD_RESOURCE_QUEUE] Add a buffer to the device's
> +queue.
> +
> +\begin{lstlisting}
> +struct virtio_video_resource_queue {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 queue_type;
> +        le32 resource_id;
> +        le64 timestamp;
> +        le32 num_data_sizes;
> +        le32 data_sizes[VIRTIO_VIDEO_MAX_PLANES];
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{resource_id}] is the ID of the resource to be queued.
> +\item[\field{timestamp}] is an abstract sequence counter that can be
> +  used for synchronization. For an input buffer, the driver MUST set a
> +  \field{timestamp}.
> +\item[\field{num_data_sizes}] is the number of \field{data_sizes}
> +  entries in use.
> +\item[\field{data_sizes}] number of data bytes within a plane.
> +\end{description}
> +
> +The device returns \field{virtio_video_resource_queue_resp} defined as
> +follows:
> +\begin{lstlisting}
> +enum virtio_video_buffer_flag {
> +        VIRTIO_VIDEO_BUFFER_FLAG_ERR = 0x0001,
> +        VIRTIO_VIDEO_BUFFER_FLAG_EOS = 0x0002,
> +
> +        /* Encoder only */
> +        VIRTIO_VIDEO_BUFFER_FLAG_IFRAME = 0x0004,
> +        VIRTIO_VIDEO_BUFFER_FLAG_PFRAME = 0x0008,
> +        VIRTIO_VIDEO_BUFFER_FLAG_BFRAME = 0x0010,
> +};
> +
> +struct virtio_video_resource_queue_resp {
> +        struct virtio_video_cmd_hdr hdr;
> +        le64 timestamp;
> +        le32 flags;
> +        le32 size; /* Encoded size */
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{timestamp}] is an abstract sequence counter that can be
> +  used for synchronization. For an output buffer, the device MUST copy
> +  the \field{timestamp} of the input buffer this output buffer was
> +  produced from.
> +\item[\field{flags}] marks specific buffers in the sequence with
> +  VIRTIO_VIDEO_BUFFER_FLAG_* flags.
> +\item[\field{size}] is the data size in the buffer (encoder only).
> +\end{description}
> +
> +\begin{itemize*}
> +\item For each VIRTIO_VIDEO_CMD_RESOURCE_QUEUE request, the device
> +  MUST send a response to the queue request with
> +  VIRTIO_VIDEO_OK_NODATA when it has finished processing the buffer
> +  successfully.
> +\item The device MUST mark a buffer that triggered a processing error
> +  with the VIRTIO_VIDEO_BUFFER_F_ERR flag.
> +\item The device MUST mark the last buffer with the
> +  VIRTIO_VIDEO_BUFFER_F_EOS flag to denote completion of the drain
> +  sequence.
> +\item In case of encoder, to denote a particular frame type the device
> +  MUST mark the respective buffer with VIRTIO_VIDEO_BUFFER_IFRAME,
> +  VIRTIO_VIDEO_BUFFER_PFRAME, or VIRTIO_VIDEO_BUFFER_BFRAME.
> +\item If the processing was stopped due to
> +  VIRTIO_VIDEO_CMD_QUEUE_CLEAR, the device MUST respond with
> +  VIRTIO_VIDEO_RESP_OK_NODATA as a response type and
> +  VIRTIO_VIDEO_BUFFER_FLAG_ERR in \field{flags}.
> +\item The driver and device MUST follow requirements about buffer
> +  ownership explained in \ref{sec:Device Types / Video Device / Device
> +    Operation / Buffer lifecycle}.
> +\end{itemize*}
> +
> +\item[VIRTIO_VIDEO_CMD_STREAM_DRAIN] Ask the device to push all the
> +  queued buffers through the pipeline.
> +
> +\begin{lstlisting}
> +struct virtio_video_stream_drain {
> +        struct virtio_video_cmd_hdr hdr;
> +};
> +\end{lstlisting}
> +
> +\begin{itemize*}
> +\item While the device is processing a VIRTIO_VIDEO_CMD_STREAM_DRAIN
> +  command, the device MUST return
> +  VIRTIO_VIDEO_RESP_ERR_INVALID_OPERATION for incoming commands of
> +  VIRTIO_VIDEO_CMD_RESOURCE_QUEUE for input buffers and
> +  VIRTIO_VIDEO_CMD_STREAM_DRAIN.
> +\item If the processing was stopped due to
> +  VIRTIO_VIDEO_CMD_QUEUE_CLEAR, the device MUST respond with
> +  VIRTIO_VIDEO_RESP_OK_NODATA as a response type and
> +  VIRTIO_VIDEO_BUFFER_FLAG_ERR in \field{flags}.
> +\end{itemize*}
> +
> +\item[VIRTIO_VIDEO_CMD_QUEUE_CLEAR] Return already queued
> +  buffers back from the input or the output queue of the device. The
> +  device SHOULD return all of the buffers from the respective queue as
> +  soon as possible without pushing the buffers through the processing
> +  pipeline.
> +
> +\begin{lstlisting}
> +struct virtio_video_resource_queue_clear {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +\begin{description}
> +\item[\field{queue_type}] queue type.
> +\end{description}
> +
> +While the device is processing a VIRTIO_VIDEO_CMD_QUEUE_CLEAR command,
> +the device MUST return VIRTIO_VIDEO_RESP_ERR_INVALID_OPERATION for
> +incoming commands of VIRTIO_VIDEO_CMD_RESOURCE_QUEUE,
> +VIRTIO_VIDEO_CMD_STREAM_DRAIN, and VIRTIO_VIDEO_CMD_QUEUE_CLEAR.
> +
> +\end{description}
> +
> +\paragraph{Device Operation: Handle stream parameters}
> +
> +\begin{itemize*}
> +\item Use VIRTIO_VIDEO_CMD_GET_PARAMS to get the current stream
> +  parameters for input and output streams from the device.
> +\item Use VIRTIO_VIDEO_CMD_SET_PARAMS to provide new stream parameters
> +  to the device.
> +\item After setting stream parameters, the driver may issue
> +  VIRTIO_VIDEO_CMD_GET_PARAMS as some parameters of both input and
> +  output can be changed implicitly by the device during the set
> +  operation.
> +\end{itemize*}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CMD_GET_PARAMS] Get parameters of the input or the
> +  output of a stream.
> +
> +\begin{lstlisting}
> +struct virtio_video_plane_format {
> +        le32 plane_size;
> +        le32 stride;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{plane_size}] size of the plane in bytes.
> +\item[\field{stride}] stride used for the plane in bytes.
> +\end{description}
> +
> +\begin{lstlisting}
> +struct virtio_video_crop {
> +        le32 left;
> +        le32 top;
> +        le32 width;
> +        le32 height;
> +};
> +
> +struct virtio_video_params {
> +        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
> +        le32 format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> +        le32 frame_width;
> +        le32 frame_height;
> +        le32 min_buffers;
> +        le32 max_buffers;
> +        struct virtio_video_crop crop;
> +        le32 frame_rate;
> +        le32 num_planes;
> +        struct virtio_video_plane_format plane_formats[VIRTIO_VIDEO_MAX_PLANES];
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{frame_width}] the value to get/set.
> +\item[\field{frame_height}] the value to get/set.
> +\item[\field{pixel_format}] the value to get/set.
> +\item[\field{min_buffers}] minimum buffers required to handle the
> +  format (r/o).
> +\item[\field{max_buffers}] maximum buffers required to handle the
> +  format (r/o).
> +\item[\field{crop}] cropping (composing) rectangle.
> +\item[\field{frame_rate}] the value to get/set.
> +\item[\field{num_planes}] number of planes used to store pixel data
> +(r/o).
> +\item[\field{plane_formats}] description of each plane.
> +\end{description}
> +
> +\begin{lstlisting}
> +struct virtio_video_get_params {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 queue_type; /* One of VIRTIO_VIDEO_QUEUE_TYPE_* types */
> +        u8 padding[4];
> +};
> +
> +struct virtio_video_get_params_resp {
> +        struct virtio_video_cmd_hdr hdr;
> +        struct virtio_video_params params;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{queue_type}] queue type.
> +\item[\field{params}] parameter values.
> +\end{description}
> +
> +\item[VIRTIO_VIDEO_CMD_SET_PARAMS] Change parameters of a stream.
> +
> +\begin{lstlisting}
> +struct virtio_video_set_params {
> +        struct virtio_video_cmd_hdr hdr;
> +        struct virtio_video_params params;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{params}] parameters to set.
> +\end{description}
> +
> +Setting stream parameters might have side effects within the device.
> +For example, the device MAY perform alignment of width and height,
> +change the number of planes it uses for the format, or do whatever
> +changes that are required to continue normal operation using the
> +updated parameters. It is up to the driver to check the parameter set
> +after the VIRTIO_VIDEO_CMD_SET_PARAMS request has been issued.
> +\end{description}
> +
> +\paragraph{Device Operation: Handle control values}
> +
> +The driver can query, get and set control values. Though control
> +values are associated with a video stream like stream parameters,
> +supported values differ depending on the device capabilities and
> +formats of videos being processed.
> +
> +\subparagraph{Commands for controls}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CMD_QUERY_CONTROL] Query supported control values.
> +
> +\begin{lstlisting}
> +struct virtio_video_query_control {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 control; /* One of VIRTIO_VIDEO_CONTROL_* types */
> +        u8 padding[4];
> +};
> +
> +struct virtio_video_query_control_resp {
> +        struct virtio_video_cmd_hdr hdr;
> +        /* Followed by one of struct virtio_video_query_control_resp_* */
> +};
> +\end{lstlisting}
> +
> +If the device supports a given control, the device MUST return a
> +struct that indicates supported control values after sending
> +\field{virtio_video_query_control_resp}. The struct used as the
> +response differs depending on the value of requested \field{control}.
> +Otherwise, the device MUST report an error with
> +VIRTIO_VIDEO_RESP_UNSUPPORTED_CONTROL.
> +
> +\item[VIRTIO_VIDEO_CMD_GET_CONTROL] Get a control value.
> +
> +\begin{lstlisting}
> +struct virtio_video_get_control {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 control; /* One of VIRTIO_VIDEO_CONTROL_* types */
> +        u8 padding[4];
> +};
> +
> +struct virtio_video_get_control_resp {
> +        struct virtio_video_cmd_hdr hdr;
> +        /* Followed by one of struct virtio_video_control_val_* */
> +};
> +\end{lstlisting}
> +
> +If the device supports a given control and a control value is
> +available, the device MUST return a control value after
> +\field{virtio_video_get_control_resp}.
> +The struct used as the response differs depending on the value of
> +requested as \field{control}. If the given control is unsupported, the
> +device MUST report an error with
> +VIRTIO_VIDEO_RESP_UNSUPPORTED_CONTROL.
> +
> +\item[VIRTIO_VIDEO_CMD_SET_CONTROL] Set a control value.
> +
> +\begin{lstlisting}
> +struct virtio_video_set_control {
> +        struct virtio_video_cmd_hdr hdr;
> +        le32 control; /* One of VIRTIO_VIDEO_CONTROL_* types */
> +        u8 padding[4];
> +        /* Followed by one of struct virtio_video_control_val_* */
> +};
> +
> +struct virtio_video_set_control_resp {
> +        struct virtio_video_cmd_hdr hdr;
> +};
> +\end{lstlisting}
> +
> +The driver MUST set \field{control} in
> +\field{virtio_video_set_control} and send a corresponding struct after
> +it. If the given control is unsupported, the device MUST report an
> +error with VIRTIO_VIDEO_RESP_UNSUPPORTED_CONTROL.
> +\end{description}
> +
> +\subparagraph{Types of controls}
> +
> +The types of controls are defined as follows:
> +\begin{lstlisting}
> +enum virtio_video_control_type {
> +        VIRTIO_VIDEO_CONTROL_BITRATE = 1,
> +        VIRTIO_VIDEO_CONTROL_PROFILE,
> +        VIRTIO_VIDEO_CONTROL_LEVEL,
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[VIRTIO_VIDEO_CONTROL_BITRATE] Bitrate of video. (Only for the
> +  encoder device.)
> +
> +The driver can VIRTIO_VIDEO_CMD_GET_CONTROL and
> +VIRTIO_VIDEO_CMD_SET_CONTROL to get and set a bitrate for encoding
> +respectively.
> +The following \field{virtio_video_control_val_bitrate} is used when
> +the device returns a value as a response of
> +VIRTIO_VIDEO_CMD_GET_CONTROL and when the driver specifies a value to
> +be set by VIRTIO_VIDEO_CMD_SET_CONTROL.
> +\begin{lstlisting}
> +struct virtio_video_control_val_bitrate {
> +        le32 bitrate;
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\item[VIRTIO_VIDEO_CONTROL_PROFILE] Profile of a compressed video
> +  stream.
> +
> +VIRTIO_VIDEO_CONTROL_PROFILE is used to handle profiles, which
> +represent sets of cabalities for several compressed formats such as
> +H.264 and VP9. The enum representing profiles are defined as follows:
> +\begin{lstlisting}
> +enum virtio_video_profile {
> +        /* H.264 */
> +        VIRTIO_VIDEO_PROFILE_H264_MIN = 0x100,
> +        VIRTIO_VIDEO_PROFILE_H264_BASELINE = VIRTIO_VIDEO_PROFILE_H264_MIN,
> +        VIRTIO_VIDEO_PROFILE_H264_MAIN,
> +        VIRTIO_VIDEO_PROFILE_H264_EXTENDED,
> +        VIRTIO_VIDEO_PROFILE_H264_HIGH,
> +        VIRTIO_VIDEO_PROFILE_H264_HIGH10PROFILE,
> +        VIRTIO_VIDEO_PROFILE_H264_HIGH422PROFILE,
> +        VIRTIO_VIDEO_PROFILE_H264_HIGH444PREDICTIVEPROFILE,
> +        VIRTIO_VIDEO_PROFILE_H264_SCALABLEBASELINE,
> +        VIRTIO_VIDEO_PROFILE_H264_SCALABLEHIGH,
> +        VIRTIO_VIDEO_PROFILE_H264_STEREOHIGH,
> +        VIRTIO_VIDEO_PROFILE_H264_MULTIVIEWHIGH,
> +        VIRTIO_VIDEO_PROFILE_H264_MAX = VIRTIO_VIDEO_PROFILE_H264_MULTIVIEWHIGH,
> +
> +        /* HEVC */
> +        VIRTIO_VIDEO_PROFILE_HEVC_MIN = 0x200,
> +        VIRTIO_VIDEO_PROFILE_HEVC_MAIN = VIRTIO_VIDEO_PROFILE_HEVC_MIN,
> +        VIRTIO_VIDEO_PROFILE_HEVC_MAIN10,
> +        VIRTIO_VIDEO_PROFILE_HEVC_MAIN_STILL_PICTURE,
> +        VIRTIO_VIDEO_PROFILE_HEVC_MAX =
> +                VIRTIO_VIDEO_PROFILE_HEVC_MAIN_STILL_PICTURE,
> +
> +        /* VP8 */
> +        VIRTIO_VIDEO_PROFILE_VP8_MIN = 0x300,
> +        VIRTIO_VIDEO_PROFILE_VP8_PROFILE0 = VIRTIO_VIDEO_PROFILE_VP8_MIN,
> +        VIRTIO_VIDEO_PROFILE_VP8_PROFILE1,
> +        VIRTIO_VIDEO_PROFILE_VP8_PROFILE2,
> +        VIRTIO_VIDEO_PROFILE_VP8_PROFILE3,
> +        VIRTIO_VIDEO_PROFILE_VP8_MAX = VIRTIO_VIDEO_PROFILE_VP8_PROFILE3,
> +
> +        /* VP9 */
> +        VIRTIO_VIDEO_PROFILE_VP9_MIN = 0x400,
> +        VIRTIO_VIDEO_PROFILE_VP9_PROFILE0 = VIRTIO_VIDEO_PROFILE_VP9_MIN,
> +        VIRTIO_VIDEO_PROFILE_VP9_PROFILE1,
> +        VIRTIO_VIDEO_PROFILE_VP9_PROFILE2,
> +        VIRTIO_VIDEO_PROFILE_VP9_PROFILE3,
> +        VIRTIO_VIDEO_PROFILE_VP9_MAX = VIRTIO_VIDEO_PROFILE_VP9_PROFILE3,
> +};
> +\end{lstlisting}
> +
> +The driver can query supported profiles for a compressed format by
> +VIRTIO_VIDEO_CMD_QUERY_CONTROL command with
> +\field{virtio_video_query_control_profile}. If the device supports the
> +given format and the format's profiles are listed in \field{enum
> +  virtio_video_profile}, the device MUST returns a list of supported
> +profiles as a response
> +\field{virtio_video_query_control_resp_profile}.
> +\begin{lstlisting}
> +struct virtio_video_query_control_profile {
> +        le32 format; /* One of VIRTIO_VIDEO_FORMAT_* */
> +        u8 padding[4];
> +};
> +
> +struct virtio_video_query_control_resp_profile {
> +        le32 num;
> +        u8 padding[4];
> +        /* Followed by an array le32 profiles[] */
> +};
> +\end{lstlisting}
> +
> +The device and the driver use \field{virtio_video_control_val_profile}
> +for VIRTIO_VIDEO_CMD_GET_CONTROL and VIRTIO_VIDEO_CMD_SET_CONTROL.
> +\begin{lstlisting}
> +struct virtio_video_control_val_profile {
> +        le32 profile;
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +
> +\item[VIRTIO_VIDEO_CONTROL_LEVEL] Level of a compressed video stream.
> +
> +VIRTIO_VIDEO_CONTROL_LEVEL is used to handle levels of video stream.
> +Levels are defined for some compressed video formats to specify sets
> +of constraints that indicate a degree of required decoder performance.
> +The enum representing levels are defined as follows:
> +\begin{lstlisting}
> +enum virtio_video_level {
> +        /* H.264 */
> +        VIRTIO_VIDEO_LEVEL_H264_MIN = 0x100,
> +        VIRTIO_VIDEO_LEVEL_H264_1_0 = VIRTIO_VIDEO_LEVEL_H264_MIN,
> +        VIRTIO_VIDEO_LEVEL_H264_1_1,
> +        VIRTIO_VIDEO_LEVEL_H264_1_2,
> +        VIRTIO_VIDEO_LEVEL_H264_1_3,
> +        VIRTIO_VIDEO_LEVEL_H264_2_0,
> +        VIRTIO_VIDEO_LEVEL_H264_2_1,
> +        VIRTIO_VIDEO_LEVEL_H264_2_2,
> +        VIRTIO_VIDEO_LEVEL_H264_3_0,
> +        VIRTIO_VIDEO_LEVEL_H264_3_1,
> +        VIRTIO_VIDEO_LEVEL_H264_3_2,
> +        VIRTIO_VIDEO_LEVEL_H264_4_0,
> +        VIRTIO_VIDEO_LEVEL_H264_4_1,
> +        VIRTIO_VIDEO_LEVEL_H264_4_2,
> +        VIRTIO_VIDEO_LEVEL_H264_5_0,
> +        VIRTIO_VIDEO_LEVEL_H264_5_1,
> +        VIRTIO_VIDEO_LEVEL_H264_MAX = VIRTIO_VIDEO_LEVEL_H264_5_1,
> +};
> +\end{lstlisting}
> +
> +The driver can query supported levels for a compressed format by
> +VIRTIO_VIDEO_CMD_QUERY_CONTROL command with
> +\field{virtio_video_query_control_level}. If the device supports the
> +given format and the format's levels are listed in \field{enum
> +  virtio_video_level}, the device MUST return a list of supported
> +levels as a response \field{virtio_video_query_control_resp_level}.
> +\begin{lstlisting}
> +struct virtio_video_query_control_level {
> +        le32 profile; /* One of VIRTIO_VIDEO_PROFILE_* */
> +        u8 padding[4];
> +};
> +
> +struct virtio_video_query_control_resp_level {
> +        le32 num;
> +        u8 padding[4];
> +        /* Followed by an array le32 level[] */
> +};
> +\end{lstlisting}
> +
> +The device and the driver use \field{virtio_video_control_val_level}
> +for VIRTIO_VIDEO_CMD_GET_CONTROL and VIRTIO_VIDEO_CMD_SET_CONTROL.
> +\begin{lstlisting}
> +struct virtio_video_control_val_level {
> +        le32 level;
> +        u8 padding[4];
> +};
> +\end{lstlisting}
> +\end{description}
> +
> +\subsubsection{Buffer lifecycle}
> +\label{sec:Device Types / Video Device / Device Operation / Buffer
> +  lifecycle}
> +
> +The state machine in Figure~\ref{fig:buffer-lifecycle} shows the life
> +cycle of a video buffer.
> +
> +\begin{figure}[h]
> +  \centering
> +  \includegraphics[width=\textwidth]{images/generated/video-buffer-lifecycle.png}
> +  \caption{Lifecycle of a buffer}
> +  \label{fig:buffer-lifecycle}
> +\end{figure}
> +
> +\drivernormative{\subparagraph}{Buffer lifecycle}{Device Types / Video
> +  Device / Device Operation / Device Operation: Buffer lifecycle}
> +
> +The following table shows whether the driver can read or write each
> +buffer in each state in Figure~\ref{fig:buffer-lifecycle}. The driver
> +MUST not read or write buffers in the state that doesn't permit.
> +\begin{center}
> +  \begin{tabular}{|c|c|c|}
> +    \hline
> +    State & Input buffers & Output buffers \\
> +    \hline
> +    Created  & Read / Write & Read \\
> +    Queued   & -            & -    \\
> +    Dequeued & Read / Write & Read \\
> +    \hline
> +  \end{tabular}
> +\end{center}
> +
> +\devicenormative{\subparagraph}{Buffer lifecycle}{Device Types / Video
> +  Device / Device Operation / Device Operation: Buffer lifecycle}
> +
> +The following table shows whether the device can read or write each
> +buffer in each state in Figure~\ref{fig:buffer-lifecycle}. The device
> +MUST not read or write buffers in the state that doesn't permit.
> +\begin{center}
> +  \begin{tabular}{ |c|c|c| }
> +    \hline
> +    State & Input buffers & Output buffers \\
> +    \hline
> +    Created  & -    & - \\
> +    Queued   & Read & Read / Write \\
> +    Dequeued & -    & Read \\
> +    \hline
> +  \end{tabular}
> +\end{center}
> +
> +\subsubsection{Event Virtqueue}
> +
> +While processing buffers, the device can send asynchronous event
> +notifications to the driver. The behaviour depends on the exact
> +stream. For example, the decoder device sends a resolution change
> +event when it encounters new resolution metadata in the stream.
> +
> +The device can report events on the event queue. The driver initially
> +populates the queue with device-writeable buffers. When the device
> +needs to report an event, it fills a buffer and notifies the driver.
> +The driver consumes the report and adds a new buffer to the virtqueue.
> +
> +\begin{lstlisting}
> +enum virtio_video_event_type {
> +        /* For all devices */
> +        VIRTIO_VIDEO_EVENT_ERROR = 0x0100,
> +
> +        /* For decoder only */
> +        VIRTIO_VIDEO_EVENT_DECODER_RESOLUTION_CHANGED = 0x0200,
> +};
> +
> +struct virtio_video_event {
> +        le32 event_type; /* One of VIRTIO_VIDEO_EVENT_* types */
> +        le32 stream_id;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{event_type}] type of the triggered event .
> +\item[\field{stream_id}] id of the source stream.
> +\end{description}
> +
> +The device MUST send VIRTIO_VIDEO_EVENT_DECODER_RESOLUTION_CHANGED
> +whenever it encounters new resolution data in the stream. This
> +includes the case of the initial device configuration after metadata
> +has been parsed and the case of dynamic resolution change.
> --
> 2.25.0.341.g760bfbb309-goog
>

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

[Dmitry Sepp]

Hi Keiichi,

On Montag, 18. Mai 2020 07:17:53 CEST Keiichi Watanabe wrote:
> > +struct virtio_video_stream_create {
> > +        struct virtio_video_cmd_hdr hdr;
> > +        le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > +        le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > +        le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > +        u8 padding[4];
> > +        u8 tag[64];
> > +};
> > +\end{lstlisting}
> > +\begin{description}
> > +\item[\field{in_mem_type, out_mem_type}] is a type of buffer
> > +  management for input /output buffers. The driver MUST set a value in
> > +  \field{enum virtio_video_mem_type} that the device reported a
> > +  corresponding feature bit.
> > +\begin{description}
> > +\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
> > +\end{description}
> > +\item[\field{coded_format}] is the encoded format that will be
> > +  processed.
> > +\item[\field{tag}] is the name associated with this stream. The tag
> > +  MUST be encoded in UTF-8 and NUL-terminated.
> 
> I wonder why we need this "tag" field. I have kept this field from
> Dmitry's first proposal, where this was called "char debug_name[64]".
> However, on second thought, I have no idea what is the necessity to
> have this field. Our VMM implementation in ChromeOS simply ignores
> this field.
> If OpenSynergy's implementation relies on this field, I'm curious
> about the usage. We might want to have an enum value instead of this
> field where arbitrary values can be stored.
> 

The use of this field is not so clear because it was renamed. In fact, one can 
have an idea how it is used by simply looking at the driver code: the field is 
useful to know about the guest client app that uses the context. If someone 
wants to store arbitrary values, they have 64 bytes to do so with this so-
called tag.

> > +\end{description}
> > +
> > +The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
> > +to an integer that is not used before. If a used value is passed as
> > +\field{stream_id}, the device MUST reports an error with
> > +VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.
> 
> I'm wondering if we can't generate stream_id in the host side so that
> we will have less error control code. In the current design, both the
> device and the driver have error checks; the device must check that a
> given ID is available and the driver must check if the device didn't
> return the INVALID_STREAM_ID error. Instead, by generating IDs in the
> device, we will be free from this type of failure. Same for
> resource_id in RESOURCE_CREATE.
> 
> I guess this design originally came from the virtio-gpu protocol.
> However, I couldn't find a benefit of adopting the same design here.
> 

Honestly I don't see too much difference: device still needs to check whether 
the id provided by the driver within some particular command is correct. If it 
is not, it will return an error. The driver needs to check (or skip checking) 
for an error either way as long as it is possible for the driver code to send 
a wrong number.

Best regards,
Dmitry.

> Any feedback is welcome.
> 
> Best regards,
> Keiichi
> 

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

[Keiichi Watanabe]

Hi Dmitry,

On Wed, May 27, 2020 at 9:12 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
>
> Hi Keiichi,
>
> On Montag, 18. Mai 2020 07:17:53 CEST Keiichi Watanabe wrote:
> > > +struct virtio_video_stream_create {
> > > +        struct virtio_video_cmd_hdr hdr;
> > > +        le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > +        le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > +        le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > > +        u8 padding[4];
> > > +        u8 tag[64];
> > > +};
> > > +\end{lstlisting}
> > > +\begin{description}
> > > +\item[\field{in_mem_type, out_mem_type}] is a type of buffer
> > > +  management for input /output buffers. The driver MUST set a value in
> > > +  \field{enum virtio_video_mem_type} that the device reported a
> > > +  corresponding feature bit.
> > > +\begin{description}
> > > +\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
> > > +\end{description}
> > > +\item[\field{coded_format}] is the encoded format that will be
> > > +  processed.
> > > +\item[\field{tag}] is the name associated with this stream. The tag
> > > +  MUST be encoded in UTF-8 and NUL-terminated.
> >
> > I wonder why we need this "tag" field. I have kept this field from
> > Dmitry's first proposal, where this was called "char debug_name[64]".
> > However, on second thought, I have no idea what is the necessity to
> > have this field. Our VMM implementation in ChromeOS simply ignores
> > this field.
> > If OpenSynergy's implementation relies on this field, I'm curious
> > about the usage. We might want to have an enum value instead of this
> > field where arbitrary values can be stored.
> >
>
> The use of this field is not so clear because it was renamed. In fact, one can
> have an idea how it is used by simply looking at the driver code: the field is
> useful to know about the guest client app that uses the context. If someone
> wants to store arbitrary values, they have 64 bytes to do so with this so-
> called tag.

Hmm, though I understand this can be useful for you, I don't think we
should support it in the standard.
For the first example, I feel something is not abstracted well if you
want to send some information from a user app to the host device. User
applications shouldn't have a way to send messages to hardware
directly.
For the second example, who is "someone"? Driver or device? In any
case, I don't think it's the right way. They should extend existing
structs or add commands or feature flags, I think. Also, if arbitrary
values are allowed, the field won't be used correctly except in cases
where both driver implementation and device implementation are
available. This is against what the spec should be: virtio protocol
must work independently from the implementations.
Of course, it's obviously okay to have it as a downstream extension in
your product's local repository.

>
> > > +\end{description}
> > > +
> > > +The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
> > > +to an integer that is not used before. If a used value is passed as
> > > +\field{stream_id}, the device MUST reports an error with
> > > +VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.
> >
> > I'm wondering if we can't generate stream_id in the host side so that
> > we will have less error control code. In the current design, both the
> > device and the driver have error checks; the device must check that a
> > given ID is available and the driver must check if the device didn't
> > return the INVALID_STREAM_ID error. Instead, by generating IDs in the
> > device, we will be free from this type of failure. Same for
> > resource_id in RESOURCE_CREATE.
> >
> > I guess this design originally came from the virtio-gpu protocol.
> > However, I couldn't find a benefit of adopting the same design here.
> >
>
> Honestly I don't see too much difference: device still needs to check whether
> the id provided by the driver within some particular command is correct. If it
> is not, it will return an error. The driver needs to check (or skip checking)
> for an error either way as long as it is possible for the driver code to send
> a wrong number.

I'm talking about creation commands only. So, other commands won't be affected.

Let me try to explain my idea in a different way. The relationship
between the driver and the device can be seen as a client-server
model.
The client (driver) sends a request and the server (device) sends a
response by processing or generating some data.
Thus, I feel it's more natural that new data, including IDs, are
generated and provided by the device.

Best regards,
Keiichi

>
> Best regards,
> Dmitry.
>
> > Any feedback is welcome.
> >
> > Best regards,
> > Keiichi
> >
>
>

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

[Alexandre Courbot]

On Fri, May 29, 2020 at 11:22 PM Keiichi Watanabe <keiichiw@chromium.org> wrote:
>
> Hi Dmitry,
>
> On Wed, May 27, 2020 at 9:12 PM Dmitry Sepp <dmitry.sepp@opensynergy.com> wrote:
> >
> > Hi Keiichi,
> >
> > On Montag, 18. Mai 2020 07:17:53 CEST Keiichi Watanabe wrote:
> > > > +struct virtio_video_stream_create {
> > > > +        struct virtio_video_cmd_hdr hdr;
> > > > +        le32 in_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > > +        le32 out_mem_type; /* One of VIRTIO_VIDEO_MEM_TYPE_* types */
> > > > +        le32 coded_format; /* One of VIRTIO_VIDEO_FORMAT_* types */
> > > > +        u8 padding[4];
> > > > +        u8 tag[64];
> > > > +};
> > > > +\end{lstlisting}
> > > > +\begin{description}
> > > > +\item[\field{in_mem_type, out_mem_type}] is a type of buffer
> > > > +  management for input /output buffers. The driver MUST set a value in
> > > > +  \field{enum virtio_video_mem_type} that the device reported a
> > > > +  corresponding feature bit.
> > > > +\begin{description}
> > > > +\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
> > > > +\end{description}
> > > > +\item[\field{coded_format}] is the encoded format that will be
> > > > +  processed.
> > > > +\item[\field{tag}] is the name associated with this stream. The tag
> > > > +  MUST be encoded in UTF-8 and NUL-terminated.
> > >
> > > I wonder why we need this "tag" field. I have kept this field from
> > > Dmitry's first proposal, where this was called "char debug_name[64]".
> > > However, on second thought, I have no idea what is the necessity to
> > > have this field. Our VMM implementation in ChromeOS simply ignores
> > > this field.
> > > If OpenSynergy's implementation relies on this field, I'm curious
> > > about the usage. We might want to have an enum value instead of this
> > > field where arbitrary values can be stored.
> > >
> >
> > The use of this field is not so clear because it was renamed. In fact, one can
> > have an idea how it is used by simply looking at the driver code: the field is
> > useful to know about the guest client app that uses the context. If someone
> > wants to store arbitrary values, they have 64 bytes to do so with this so-
> > called tag.
>
> Hmm, though I understand this can be useful for you, I don't think we
> should support it in the standard.
> For the first example, I feel something is not abstracted well if you
> want to send some information from a user app to the host device. User
> applications shouldn't have a way to send messages to hardware
> directly.

I am also a bit uncomfortable with having fields whose usage is not
clearly defined in the kernel. How would user-space specify the value
it wants to set there?

> For the second example, who is "someone"? Driver or device? In any
> case, I don't think it's the right way. They should extend existing
> structs or add commands or feature flags, I think. Also, if arbitrary
> values are allowed, the field won't be used correctly except in cases
> where both driver implementation and device implementation are
> available. This is against what the spec should be: virtio protocol
> must work independently from the implementations.
> Of course, it's obviously okay to have it as a downstream extension in
> your product's local repository.
>
> >
> > > > +\end{description}
> > > > +
> > > > +The driver MUST set \field{stream_id} in \field{virtio_video_cmd_hdr}
> > > > +to an integer that is not used before. If a used value is passed as
> > > > +\field{stream_id}, the device MUST reports an error with
> > > > +VIRTIO_VIDEO_RESP_ERR_INVALID_STREAM_ID.
> > >
> > > I'm wondering if we can't generate stream_id in the host side so that
> > > we will have less error control code. In the current design, both the
> > > device and the driver have error checks; the device must check that a
> > > given ID is available and the driver must check if the device didn't
> > > return the INVALID_STREAM_ID error. Instead, by generating IDs in the
> > > device, we will be free from this type of failure. Same for
> > > resource_id in RESOURCE_CREATE.
> > >
> > > I guess this design originally came from the virtio-gpu protocol.
> > > However, I couldn't find a benefit of adopting the same design here.
> > >
> >
> > Honestly I don't see too much difference: device still needs to check whether
> > the id provided by the driver within some particular command is correct. If it
> > is not, it will return an error. The driver needs to check (or skip checking)
> > for an error either way as long as it is possible for the driver code to send
> > a wrong number.
>
> I'm talking about creation commands only. So, other commands won't be affected.
>
> Let me try to explain my idea in a different way. The relationship
> between the driver and the device can be seen as a client-server
> model.
> The client (driver) sends a request and the server (device) sends a
> response by processing or generating some data.
> Thus, I feel it's more natural that new data, including IDs, are
> generated and provided by the device.

This also results in one less check on the device side: when creating
a stream it just needs to pick the first available ID, instead of
checking whether the client provided something valid. This also
removes the burden of managing stream IDs from the driver, since the
device is entirely in charge of it (something it would have to do
anyway in both cases since it would still need to check that the
client-provided ID is valid).

[PATCH v3 2/2] virtio-video: Define a feature for exported objects from different virtio devices

[Keiichi Watanabe]

Define a new feature in the virtio-video protocol to use objects
exported from different virtio devices as video buffers.

Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
---
 virtio-video.tex | 58 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/virtio-video.tex b/virtio-video.tex
index 2eeee53..5d7451f 100644
--- a/virtio-video.tex
+++ b/virtio-video.tex
@@ -30,12 +30,16 @@ \subsection{Feature bits}
   non-contiguous memories for video buffers. Without this flag, the
   driver and device MUST use video buffers that are contiguous in the
   device-side.
+\item[VIRTIO_VIDEO_F_RESOURCE_VIRTIO_OBJECT (2)] Object exported by
+  another virtio device can be used for video buffers.
 \end{description}
 
 \devicenormative{\subsubsection}{Feature bits}{Device Types / Video
   Device / Feature bits}
 
-The device MUST offer VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES.
+The device MUST offer at least either
+VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES or
+VIRTIO_VIDEO_F_RESOURCE_VIRTIO_OBJECT.
 
 \subsection{Device configuration layout}
 \label{sec:Device Types / Video Device / Device configuration layout}
@@ -182,8 +186,9 @@ \subsubsection{Command Virtqueue}
 The device MUST return its capability with \field{
   virtio_video_capability_resp} that includes the following fields:
 \begin{description}
-\item[\field{num_descs}] is a number of \field{virtio_video_format_desc}
-  that follow. The value MUST not exceed 64.
+\item[\field{num_descs}] is a number of
+  \field{virtio_video_format_desc} that follow. The value MUST not
+  exceed 64.
 \end{description}
 
 The format description \field{virtio_video_format_desc} is defined as
@@ -299,6 +304,7 @@ \subsubsection{Command Virtqueue}
 \begin{lstlisting}
 enum virtio_video_mem_type {
         VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES,
+        VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT,
 };
 
 struct virtio_video_stream_create {
@@ -310,13 +316,19 @@ \subsubsection{Command Virtqueue}
         u8 tag[64];
 };
 \end{lstlisting}
+
 \begin{description}
 \item[\field{in_mem_type, out_mem_type}] is a type of buffer
-  management for input /output buffers. The driver MUST set a value in
+  management for input/output buffers. The driver MUST set a value in
   \field{enum virtio_video_mem_type} that the device reported a
   corresponding feature bit.
 \begin{description}
-\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages.
+\item[\field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES}] Use guest pages. The
+  driver MUST not set this value if a feature bit
+  VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES is not set.
+\item[\field{VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT}] Use object exported
+  by another virtio device. The driver MUST not set this value if a
+  feature bit VIRTIO_VIDEO_F_RESOURCE_VIRTIO_OBJECT is not set.
 \end{description}
 \item[\field{coded_format}] is the encoded format that will be
   processed.
@@ -365,7 +377,13 @@ \subsubsection{Command Virtqueue}
         le32 num_planes;
         le32 plane_offsets[VIRTIO_VIDEO_MAX_PLANES];
         le32 num_entries[VIRTIO_VIDEO_MAX_PLANES];
-        /* Followed by struct virtio_video_mem_entry entries[] */
+        /*
+         * Followed by either
+         * - struct virtio_video_mem_entry entries[]
+         *   for VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES
+         * - struct virtio_video_object_entry entries[]
+         *   for VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT
+         */
 };
 \end{lstlisting}
 \begin{description}
@@ -387,8 +405,13 @@ \subsubsection{Command Virtqueue}
   used.
 \end{description}
 
-The \field{virtio_video_resource_create} is followed by an array of
-\field{virtio_video_mem_entry} defined as follows:
+The data following \field{virtio_video_resource_create} depend on a
+type of \field{virtio_video_mem_type} specified in
+\field{virtio_video_stream_create}.
+\begin{itemize*}
+\item When \field{VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES} was specified,
+  \field{virtio_video_resource_create} is followed by an array of
+  \field{virtio_video_mem_entry} defined as follows:
 \begin{lstlisting}
 struct virtio_video_mem_entry {
         le64 addr;
@@ -403,6 +426,25 @@ \subsubsection{Command Virtqueue}
 The number of \field{virtio_video_mem_entry} MUST be equal to the sum
 of integers in the array \field{num_entries}.
 
+\item When \field{VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT} was specified,
+  \field{virtio_video_resource_create} is followed by an array of
+  \field{virtio_video_object_entry} defined as follows:
+
+\begin{lstlisting}
+struct virtio_video_object_entry {
+        le64 uuid_low;
+        le64 uuid_high;
+};
+\end{lstlisting}
+\begin{description}
+\item[\field{uuid_low, uuid_high}] is a UUID of the resource.
+\end{description}
+If \field{planes_layout} is VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER,
+the number of \field{virtio_video_object_entry} MUST be one. If it is
+VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE, the number MUST be equal to
+\field{num_planes}.
+\end{itemize*}
+
 \item[VIRTIO_VIDEO_CMD_RESOURCE_DESTROY_ALL] Invalidate all the
   resource descriptor created so far.
 \begin{lstlisting}
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v3 2/2] virtio-video: Define a feature for exported objects from different virtio devices

[Gerd Hoffmann]

  Hi,

> +        /*
> +         * Followed by either
> +         * - struct virtio_video_mem_entry entries[]
> +         *   for VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES
> +         * - struct virtio_video_object_entry entries[]
> +         *   for VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT

Wouldn't that be a single virtio_video_object_entry?
Or could it be one per plane?

cheers,
  Gerd

Re: [PATCH v3 2/2] virtio-video: Define a feature for exported objects from different virtio devices

[Keiichi Watanabe]

Hi,

On Tue, Feb 25, 2020 at 7:01 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > +        /*
> > +         * Followed by either
> > +         * - struct virtio_video_mem_entry entries[]
> > +         *   for VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES
> > +         * - struct virtio_video_object_entry entries[]
> > +         *   for VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT
>
> Wouldn't that be a single virtio_video_object_entry?
> Or could it be one per plane?

It depends on the value of "planes_layout" in virtio_video_resource_create.
I have documented this restriction as follows:

> +If \field{planes_layout} is VIRTIO_VIDEO_PLANES_LAYOUT_SINGLE_BUFFER,
> +the number of \field{virtio_video_object_entry} MUST be one. If it is
> +VIRTIO_VIDEO_PLANES_LAYOUT_PER_PLANE, the number MUST be equal to
> +\field{num_planes}.

Best regards,
Keiichi


>
> cheers,
>   Gerd
>