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: Tue, 24 Mar 2020 00:48:14 +0900
Message-ID: <CAD90VcZ2zzzwveYgDs5UdjfZUt=yX4wNX-f_-3V18xz93_fpvQ@mail.gmail.com> (raw)
In-Reply-To: <8121654.T7Z3S40VBb@os-lin-dmo>

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

  reply	other threads:[~2020-03-23 15:48 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 [this message]
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
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='CAD90VcZ2zzzwveYgDs5UdjfZUt=yX4wNX-f_-3V18xz93_fpvQ@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