Le dimanche 16 mai 2021 à 20:04 -0300, Ezequiel Garcia a écrit : > Hi Hans, > > On Thu, 2021-05-06 at 14:50 +0200, Hans Verkuil wrote: > > On 05/05/2021 17:20, Benjamin Gaignard wrote: > > > > > > Le 05/05/2021 à 16:55, Hans Verkuil a écrit : > > > > On 20/04/2021 14:10, Benjamin Gaignard wrote: > > > > > The HEVC HANTRO driver needs to know the number of bits to skip at > > > > > the beginning of the slice header. > > > > > That is a hardware specific requirement so create a dedicated control > > > > > for this purpose. > > > > > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx> > > > > > --- > > > > > .../userspace-api/media/drivers/hantro.rst | 19 +++++++++++++++++++ > > > > > .../userspace-api/media/drivers/index.rst | 1 + > > > > > include/media/hevc-ctrls.h | 13 +++++++++++++ > > > > > 3 files changed, 33 insertions(+) > > > > > create mode 100644 Documentation/userspace-api/media/drivers/hantro.rst > > > > > > > > > > diff --git a/Documentation/userspace-api/media/drivers/hantro.rst b/Documentation/userspace-api/media/drivers/hantro.rst > > > > > new file mode 100644 > > > > > index 000000000000..cd9754b4e005 > > > > > --- /dev/null > > > > > +++ b/Documentation/userspace-api/media/drivers/hantro.rst > > > > > @@ -0,0 +1,19 @@ > > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > > + > > > > > +Hantro video decoder driver > > > > > +=========================== > > > > > + > > > > > +The Hantro video decoder driver implements the following driver-specific controls: > > > > > + > > > > > +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)`` > > > > > + Specifies to Hantro HEVC video decoder driver the number of data (in bits) to > > > > > + skip in the slice segment header. > > > > > + If non-IDR, the bits to be skipped go from syntax element "pic_output_flag" > > > > > + to before syntax element "slice_temporal_mvp_enabled_flag". > > > > > + If IDR, the skipped bits are just "pic_output_flag" > > > > > + (separate_colour_plane_flag is not supported). > > > > I'm not very keen on this. Without this information the video data cannot be > > > > decoded, or will it just be suboptimal? > > > > > > Without that information the video can't be decoded. > > > > > > > > > > > The problem is that a generic decoder would have to know that the HW is a hantro, > > > > and then call this control. If they don't (and are testing on non-hantro HW), then > > > > it won't work, thus defeating the purpose of the HW independent decoder API. > > > > > > > > Since hantro is widely used, and if there is no other way to do this beside explitely > > > > setting this control, then perhaps this should be part of the standard HEVC API. > > > > Non-hantro drivers that do not need this can just skip it. > > > > > > Even if I put this parameter in decode_params structure that would means that a generic > > > userland decoder will have to know how the compute this value for hantro HW since it > > > isn't something that could be done on kernel side. > > > > But since hantro is very common, any userland decoder will need to calculate this anyway. > > So perhaps it is better to have this as part of the decode_params? > > > > I'd like to know what others think about this. > > > > As you know, I'm not a fan of carrying these "unstable" APIs around. > I know it's better than nothing, but I feel they create the illusion > of the interface being supported in mainline. Since it's unstable, > it's difficult for applications to adopt them. > > As Nicolas mentioned, this means neither FFmpeg nor GStreamer will adopt > these APIs, which worries me, as that means we lose two major user bases. > > My personal take from this, is that we need to find ways to stabilize > our stateless codec APIs in less time and perhaps with less effort. > > IMO, a less stiff interface could help us here, and that's why I think > having hardware-specific controls can be useful. Hardware designers > can be so creative :) > > I'm not against introducing this specific parameter in > v4l2_ctrl_hevc_codec_params, arguing that Hantro is widely used, > but I'd like us to be open to hardware-specific controls as a way > to extend the APIs seamlessly. > > Applications won't have to _know_ what hardware they are running on, > they can just use VIDIOC_QUERYCTRL to find out which controls are needed. Can you extend on this, perhaps we need an RFC for this specific mechanism. I don't immediatly see how I could enumerate controls and figure-out which one are needed. Perhaps we need to add new control flags for mandatory control ? This way userspace could detect unsupported HW if it finds a mandatory control that it does not know about ? > > Thanks, > Ezequiel >
