Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1677127 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs Kamil Debski <kamil@wypas.org> - 2017-06-28 22:10 +0200
    Re: [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs Smitha T Murthy <smitha.t@samsung.com> - 2017-06-30 11:10 +0200

#1677127 — 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@samsung.com> wrote:
> Added V4l2 controls for HEVC encoder
>
> Signed-off-by: Smitha T Murthy <smitha.t@samsung.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

[toc] | [next] | [standalone]

#1678616

On Wed, 2017-06-28 at 22:04 +0200, Kamil Debski wrote:
> Hi,
> 
> Please find my comments inline.
> 
> On 19 June 2017 at 07:10, Smitha T Murthy <smitha.t@samsung.com> wrote:
> > Added V4l2 controls for HEVC encoder
> >
> > Signed-off-by: Smitha T Murthy <smitha.t@samsung.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.
> 
I had changed it from HIERARCHICAL to HIER as per suggestion by
Sylwester Nawrocki. Here
https://patchwork.kernel.org/patch/9666129/

> [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.
> 
I will add enum options for the above control.

> > +
> > +``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
> 
> 
Thank you for the review.

Regards,
Smitha

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web