linux-kernel - Re: [PATCH v2 2/2] media: docs-rst: Document memory-to-memory video encoder interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAFQd5BSvbeRB4XSpaORKmKP1_fhEtqm9Q10M7zJ0Evp0GPMLg@mail.gmail.com>
Date:   Wed, 6 Feb 2019 14:49:02 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Nicolas Dufresne <nicolas@...fresne.ca>
Cc:     Hans Verkuil <hverkuil@...all.nl>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Paweł Ościak <posciak@...omium.org>,
        Alexandre Courbot <acourbot@...omium.org>,
        Kamil Debski <kamil@...as.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Jeongtae Park <jtp.park@...sung.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Tiffany Lin <tiffany.lin@...iatek.com>,
        Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Todor Tomov <todor.tomov@...aro.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        dave.stevenson@...pberrypi.org,
        Ezequiel Garcia <ezequiel@...labora.com>,
        Maxime Jourdan <maxi.jourdan@...adoo.fr>
Subject: Re: [PATCH v2 2/2] media: docs-rst: Document memory-to-memory video
 encoder interface

On Thu, Jan 31, 2019 at 12:06 AM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
>
> Le vendredi 25 janvier 2019 à 12:59 +0900, Tomasz Figa a écrit :
> > On Fri, Jan 25, 2019 at 5:14 AM Nicolas Dufresne <nicolas@...fresne.ca> wrote:
> > > Le mercredi 23 janvier 2019 à 14:04 +0100, Hans Verkuil a écrit :
> > > > > > Does this return the same set of formats as in the 'Querying Capabilities' phase?
> > > > > >
> > > > >
> > > > > It's actually an interesting question. At this point we wouldn't have
> > > > > the OUTPUT resolution set yet, so that would be the same set as in the
> > > > > initial query. If we set the resolution (with some arbitrary
> > > > > pixelformat), it may become a subset...
> > > >
> > > > But doesn't setting the capture format also set the resolution?
> > > >
> > > > To quote from the text above:
> > > >
> > > > "The encoder will derive a new ``OUTPUT`` format from the ``CAPTURE`` format
> > > >  being set, including resolution, colorimetry parameters, etc."
> > > >
> > > > So you set the capture format with a resolution (you know that), then
> > > > ENUM_FMT will return the subset for that codec and resolution.
> > > >
> > > > But see also the comment at the end of this email.
> > >
> > > I'm thinking that the fact that there is no "unset" value for pixel
> > > format creates a certain ambiguity. Maybe we could create a new pixel
> > > format, and all CODEC driver could have that set by default ? Then we
> > > can just fail STREAMON if that format is set.
> >
> > The state on the CAPTURE queue is actually not "unset". The queue is
> > simply not ready (yet) and any operations on it will error out.
>
> My point was that it's just awkward to have this "not ready" state, in
> which you cannot go back. And in which the enum-format will ignore the
> format configured on the other side.
>
> What I wanted to say is that this special case is not really needed.
>

Yeah, I think we may actually end up going in that direction, as you
probably noticed in the discussion over the "venus: dec: make decoder
compliant with stateful codec API" patch [1].

[1] https://patchwork.kernel.org/patch/10768539/#22462703

> >
> > Once the application sets the coded resolution on the OUTPUT queue or
> > the decoder parses the stream information, the CAPTURE queue becomes
> > ready and one can do the ioctls on it.
> >
> > > That being said, in GStreamer, I have split each elements per CODEC,
> > > and now only enumerate the information "per-codec". That makes me think
> > > this "global" enumeration was just a miss-use of the API / me learning
> > > to use it. Not having to implement this rather complex thing in the
> > > driver would be nice. Notably, the new Amlogic driver does not have
> > > this "Querying Capabilities" phase, and with latest GStreamer works
> > > just fine.
> >
> > What do you mean by "doesn't have"? Does it lack an implementation of
> > VIDIOC_ENUM_FMT and VIDIOC_ENUM_FRAMESIZES?
>
> What it does is that it sets a default value for the codec format, so
> if you just open the device and do enum_fmt/framesizes, you get that is
> possible for the default codec that was selected. And I thin it's
> entirely correct, doing ENUM_FMT(capture) without doing an
> S_FMT(output) can easily be documented as undefined behaviour.

Okay.

>
> For proper enumeration would be:
>
> for formats on OUTPUT device:
>   S_FMT(OUTPUT):
>   for formats on CAPTURE device:
>     ...
>
> (the pseudo for look represent an enum operation)

And that's how it's defined in v3. There is no default state without
any codec selected.

Best regards,
Tomasz