Re: [RFC] Request API and V4L2 capabilities

Paul Kocialkowski
2018-08-06T11:37:57Z

[-- Attachment #1: Type: text/plain, Size: 6049 bytes --]

On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > in each request.
> > > > >
> > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > >
> > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > the API, but it has to be documented together with the pixel format documentation.
> > > >
> > > > I think this is affected by considerations about codec profile/level
> > > > support. More specifically, some controls will only be required for
> > > > supporting advanced codec profiles/levels, so they can only be
> > > > explicitly marked with appropriate flags by the driver when the target
> > > > profile/level is known. And I don't think it would be sane for userspace
> > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > don't think we can explicitly mark controls as required or optional.
> > > >
> > > > I also like the idea that it should instead be implicit and that the
> > > > documentation should detail which specific stateless metadata controls
> > > > are required for a given profile/level.
> > > >
> > > > As for controls validation, the approach followed in the Cedrus driver
> > > > is to check that the most basic controls are filled and allow having
> > > > missing controls for those that match advanced profiles.
> > > >
> > > > Since this approach feels somewhat generic enough to be applied to all
> > > > stateless VPU drivers, maybe this should be made a helper in the
> > > > framework?
> > >
> > > Sounds reasonable. Not sure if it will be in the first version, but it is
> > > easy to add later.
> >
> > Definitely, I don't think this is such a high priority for now either.
> >
> > > > In addition, I see a need for exposing the maximum profile/level that
> > > > the driver supports for decoding. I would suggest reusing the already-
> > > > existing dedicated controls used for encoding for this purpose. For
> > > > decoders, they would be used to expose the (read-only) maximum
> > > > profile/level that is supported by the hardware and keep using them as a
> > > > settable value in a range (matching the level of support) for encoders.
> > > >
> > > > This is necessary for userspace to determine whether a given video can
> > > > be decoded in hardware or not. Instead of half-way decoding the video
> > > > (ending up in funky results), this would easily allow skipping hardware
> > > > decoding and e.g. falling back on software decoding.
> > >
> > > I think it might be better to expose this through new read-only bitmask
> > > controls: i.e. a bitmask containing the supported profiles and levels.
> >
> > It seems that this is more or less what the coda driver is doing for
> > decoding actually, although it uses a menu control between min/max
> > supported profile/levels, with a mask to "blacklist" the unsupported
> > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> > control read-only.
> >
> > > Reusing the existing controls for a decoder is odd since there is not
> > > really a concept of a 'current' value since you just want to report what
> > > is supported. And I am not sure if all decoders can report the profile
> > > or level that they detect.
> >
> > Is that really a problem when the READ_ONLY flag is set? I thought it
> > was designed to fit this specific case, when the driver reports a value
> > that userspace cannot affect.
>
> Well, for read-only menu controls the current value of the control would
> have to indicate what the current profile/level is that is being decoded.
>
> That's not really relevant since what you want is just to query the
> supported profiles/levels. A read-only bitmask control is the fastest
> method (if only because using a menu control requires the application to
> enumerate all possibilities with QUERYMENU).

Ah yes, I finally understand the issue with what the current control
value represents here. Since I don't think the driver should have to
bother with figuring out the profile in use (as expressed earlier, I
think it should be implicit, through the codec metadata controls and
features used), I no longer believe it's best to have the same control
for both encoding and decoding.

> > Otherwise, I agree that having a bitmask type would be a better fit, but
> > I think it would be beneficial to keep the already-defined control and
> > associated values, which implies using the menu control type for both
> > encoders and decoders.
> >
> > If this is not an option, I would be in favour of adding per-codec read-
> > only bitmask controls (e.g. for H264 something like
> > V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
> > existing profile/level definitions as bit identifiers (a bit like coda
> > is using them to craft a mask for the menu items to blacklist) for
> > decoding only.
>
> That's what I have in mind, yes. I'd like Tomasz' input as well, though.

Okay, so this sounds good to me at this point!

Cheers,

Paul

--
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]