linux-kernel - Re: [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs

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: <CAP3TMiG4PtiqeHhCGkHhdX_uvOhPKd=gTyckczyFKDJwnGfwYA@mail.gmail.com>
Date:   Wed, 28 Jun 2017 22:04:05 +0200
From:   Kamil Debski <kamil@...as.org>
To:     Smitha T Murthy <smitha.t@...sung.com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Kyungmin Park <kyungmin.park@...sung.com>,
        jtp.park@...sung.com, a.hajda@...sung.com, mchehab@...nel.org,
        pankaj.dubey@...sung.com, krzk@...nel.org,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sylwester Nawrocki <s.nawrocki@...sung.com>
Subject: Re: [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs

Hi,

Please find my comments inline.

On 19 June 2017 at 07:10, Smitha T Murthy <smitha.t@...sung.com> wrote:
> Added V4l2 controls for HEVC encoder
>
> Signed-off-by: Smitha T Murthy <smitha.t@...sung.com>
> ---
>  Documentation/media/uapi/v4l/extended-controls.rst | 364 +++++++++++++++++++++
>  1 file changed, 364 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> index abb1057..7767c70 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -1960,6 +1960,370 @@ enum v4l2_vp8_golden_frame_sel -
>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
>
>

[snip]

> +
> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_LAYER (integer)``
> +    Selects the hierarchical coding layer. In normal encoding
> +    (non-hierarchial coding), it should be zero. Possible values are 0 ~ 6.
> +    0 indicates HIERARCHICAL CODING LAYER 0, 1 indicates HIERARCHICAL CODING
> +    LAYER 1 and so on.

I would like the above to be more consistent. If HIER is in the name
then HIER in the description should be used as well. Aside from that,
I would recommend using full HIERARCHICAL instead of HIER in the name
of the control. Why? Because it is HIERARCHICAL in controls already
present in V4L2, such as
V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING_LAYER_QP.

[snip]

> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_LF (boolean)``
> +    Indicates loop filtering. Control value 1 indicates loop filtering
> +    is enabled and when set to 0 indicates loop filtering is disabled.
> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_LF_SLICE_BOUNDARY (boolean)``
> +    Selects whether to apply the loop filter across the slice boundary or not.
> +    If the value is 0, loop filter will not be applied across the slice boundary.
> +    If the value is 1, loop filter will be applied across the slice boundary.

Just a thought. Pretty much the same fucntionality is achieved via the
V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_MODE control. It's an enum having
three states: enabled, disabled and disabled at slice boundary. Maybe
a single control could be introduced? With another legacy define for
API compatibility. Also, I don't like that controls are not consistent
between H264 and HEVC. I would opt for the enum option.

> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_LF_BETA_OFFSET_DIV2 (integer)``
> +    Selects HEVC loop filter beta offset. The valid range is [-6, +6].
> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_LF_TC_OFFSET_DIV2 (integer)``
> +    Selects HEVC loop filter tc offset. The valid range is [-6, +6].
> +
> +.. _v4l2-hevc-refresh-type:
> +
> +``V4L2_CID_MPEG_VIDEO_HEVC_REFRESH_TYPE``
> +    (enum)
> +
[snip]

Best wishes,
Kamil