Unnamed repository; edit this file 'description' to name the repository.
 help / color / mirror / Atom feed
From: Keiichi Watanabe <keiichiw@chromium.org>
To: Dmitry Sepp <dmitry.sepp@opensynergy.com>
Cc: "Alexandre Courbot" <acourbot@chromium.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	virtio-dev@lists.oasis-open.org,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Alex Lau" <alexlau@chromium.org>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Dylan Reid" <dgreid@chromium.org>,
	"David Staessens" <dstaessens@chromium.org>,
	"Enrico Granata" <egranata@google.com>,
	"Frediano Ziglio" <fziglio@redhat.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	"Stéphane Marchesin" <marcheu@chromium.org>,
	"Pawel Osciak" <posciak@chromium.org>,
	spice-devel@lists.freedesktop.org,
	"David Stevens" <stevensd@chromium.org>,
	"Tomasz Figa" <tfiga@chromium.org>,
	uril@redhat.com,
	"Samiullah Khawaja" <samiullah.khawaja@opensynergy.com>,
	"Kiran Pawar" <kiran.pawar@opensynergy.com>
Subject: Re: [PATCH v3 1/2] virtio-video: Add virtio video device specification
Date: Mon, 20 Apr 2020 18:57:52 +0900
Message-ID: <CAD90VcbgrcMEiYSGZo66qZS8uBznLim3nVPnsmbekstB3Brktg@mail.gmail.com> (raw)
In-Reply-To: <3536507.QJadu78ljV@os-lin-dmo>

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

  reply	other threads:[~2020-04-20  9:58 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-06 10:20 [PATCH v3 0/2] Virtio " Keiichi Watanabe
2020-02-06 10:20 ` [PATCH v3 1/2] virtio-video: Add virtio " Keiichi Watanabe
2020-02-25  9:59   ` Gerd Hoffmann
2020-02-27  7:24     ` Keiichi Watanabe
2020-02-27  9:28       ` Gerd Hoffmann
2020-03-04  4:31         ` Alexandre Courbot
2020-03-04  6:42           ` Gerd Hoffmann
2020-03-04 10:07             ` Alexandre Courbot
2020-03-23 12:07               ` Keiichi Watanabe
2020-03-23 13:28                 ` Dmitry Sepp
2020-03-23 15:48                   ` Keiichi Watanabe
2020-03-25  9:47                     ` Dmitry Sepp
2020-03-27  3:35                       ` Keiichi Watanabe
2020-03-30  9:53                         ` Dmitry Sepp
2020-04-06  9:32                           ` Alexandre Courbot
2020-04-06 11:46                             ` Keiichi Watanabe
2020-04-07  9:21                               ` Dmitry Sepp
2020-04-09 10:46                                 ` Keiichi Watanabe
2020-04-17  8:08                                   ` Dmitry Sepp
2020-04-20  9:57                                     ` Keiichi Watanabe [this message]
2020-04-21  8:38                                       ` Dmitry Sepp
2020-04-24 11:42                                         ` Keiichi Watanabe
2020-04-27 14:28                                           ` Dmitry Sepp
2020-04-07 14:49   ` Dmitry Sepp
2020-04-09 10:46     ` Keiichi Watanabe
2020-04-09 13:13       ` Dmitry Sepp
2020-04-24 11:45         ` Keiichi Watanabe
2020-04-27  9:33           ` Dmitry Sepp
2020-05-18  5:17   ` Keiichi Watanabe
2020-05-27 12:12     ` Dmitry Sepp
2020-05-29 14:21       ` Keiichi Watanabe
2020-06-01  7:19         ` Alexandre Courbot
2020-02-06 10:20 ` [PATCH v3 2/2] virtio-video: Define a feature for exported objects from different virtio devices Keiichi Watanabe
2020-02-25 10:01   ` Gerd Hoffmann
2020-02-27  7:24     ` Keiichi Watanabe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAD90VcbgrcMEiYSGZo66qZS8uBznLim3nVPnsmbekstB3Brktg@mail.gmail.com \
    --to=keiichiw@chromium.org \
    --cc=acourbot@chromium.org \
    --cc=alexlau@chromium.org \
    --cc=daniel@ffwll.ch \
    --cc=dgreid@chromium.org \
    --cc=dmitry.sepp@opensynergy.com \
    --cc=dstaessens@chromium.org \
    --cc=egranata@google.com \
    --cc=fziglio@redhat.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kiran.pawar@opensynergy.com \
    --cc=kraxel@redhat.com \
    --cc=linux-media@vger.kernel.org \
    --cc=marcheu@chromium.org \
    --cc=posciak@chromium.org \
    --cc=samiullah.khawaja@opensynergy.com \
    --cc=spice-devel@lists.freedesktop.org \
    --cc=stevensd@chromium.org \
    --cc=tfiga@chromium.org \
    --cc=uril@redhat.com \
    --cc=virtio-dev@lists.oasis-open.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Unnamed repository; edit this file 'description' to name the repository.

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://archive.lwn.net:8080/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ http://archive.lwn.net:8080/linux-media \
		linux-media@vger.kernel.org lwn-linux-media@archive.lwn.net
	public-inbox-index linux-media

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://archive.lwn.net/lwn.kernel.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git