linux-kernel - Re: Re: [PATCH v3 09/14] media: uapi: Add V4L2_CID_STATELESS_HEVC_ENTRY_POINT

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: <1884298.PYKUYFuaPT@kista>
Date:   Sat, 26 Feb 2022 18:59:18 +0100
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     mchehab@...nel.org, ezequiel@...guardiasur.com.ar,
        p.zabel@...gutronix.de, gregkh@...uxfoundation.org,
        mripard@...nel.org, paul.kocialkowski@...tlin.com, wens@...e.org,
        jonas@...boo.se, nicolas@...fresne.ca,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-staging@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
        kernel@...labora.com, knaerzche@...il.com, jc@...esim.co.uk,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>
Subject: Re: Re: [PATCH v3 09/14] media: uapi: Add V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSET control

Hi!

Dne petek, 25. februar 2022 ob 20:30:20 CET je Jernej Škrabec napisal(a):
> Hi!
> 
> Dne petek, 25. februar 2022 ob 17:45:55 CET je Benjamin Gaignard napisal(a):
> > The number of 'entry point offset' could be very variable.
> > Rather than use a large static array define a v4l2 dynamic array
> > of integer control.
> 
> I suggest we should be more specific and say U32 (V4L2_CTRL_TYPE_U32).
> 
> > The number of entry point offsets is reported by the elems field.

I did few more tests and these are my findings:
1. dynamic array can't be set over size, specified in .dims array
2. entry point offsets are per slice, so if we make slices dynamic array, then 
entry points would become two dimensional dynamic array
3. num_entry_point_offsets must be part of slice control, because it can be 
zero, but size of array can't be
4. fortunately, not setting entry points doesn't impact decoding correctness. 
This is in line what John told me about them.

Hans, can you comment points 1-3? I might misunderstand point 1.

In short, it seems like we don't really need entry points, even if they are 
used in BSP library. In both cases, I got fluster score 119/138 (10-bit 
excluded), so we can just drop this patch, although I'm a bit uneasy not 
setting entry points...

Best regards,
Jernej

> > 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-codec.rst          | 9 +++++++++
> >  include/media/hevc-ctrls.h                               | 1 +
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/
> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index 44a268a948c0..71f7dc1c1ccd 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -3128,6 +3128,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >  
> >      \normalsize
> >  
> > +``V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSETS (integer)``
> 
> Here you have OFFSETS (plural) ...
> 
> > +    Specifies the i-th entry point offset in bytes and is represented by
> > +    offset_len_minus1 plus 1 bits.
> 
> You probably mean entry_point_offset_minus1? offset_len_minus1 just tells how 
> much bits need to be read for each element and it's not important for actual 
> decoding.
> 
> > +    This control is a dynamically sized array. The number of entry point
> > +    offsets is reported by the ``elems`` field.
> > +    This bitstream parameter is defined according to :ref:`hevc`.
> > +    They are described in section 7.4.7.1 "General slice segment header
> > +    semantics" of the specification.
> > +
> >  ``V4L2_CID_STATELESS_HEVC_SCALING_MATRIX (struct)``
> >      Specifies the HEVC scaling matrix parameters used for the scaling 
> process
> >      for transform coefficients.
> > diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> > index 3016c1abb1d0..3f8a67924df3 100644
> > --- a/include/media/hevc-ctrls.h
> > +++ b/include/media/hevc-ctrls.h
> > @@ -20,6 +20,7 @@
> >  #define V4L2_CID_STATELESS_HEVC_DECODE_PARAMS	(V4L2_CID_CODEC_BASE + 
> 1012)
> >  #define V4L2_CID_STATELESS_HEVC_DECODE_MODE	(V4L2_CID_CODEC_BASE + 
> 1015)
> >  #define V4L2_CID_STATELESS_HEVC_START_CODE	(V4L2_CID_CODEC_BASE + 
1016)
> > +#define V4L2_CID_STATELESS_HEVC_ENTRY_POINT_OFFSET (V4L2_CID_CODEC_BASE + 
> 1017)
> 
> ... and here you have OFFSET (singlular). I suggest plural form to be used 
in 
> all places, including subject line of this commit.
> 
> Additionally, it would be nice if control is initialized, like so:
> https://github.com/jernejsk/linux-1/commit/
> f938e162cd8dd77c9f6f1b248d80144840a37bce
> 
> Best regards,
> Jernej
> 
> >  
> >  /* enum v4l2_ctrl_type type values */
> >  #define V4L2_CTRL_TYPE_HEVC_SPS 0x0120
> > -- 
> > 2.32.0
> > 
> > 
> 
> 
>