From: Smitha T Murthy <smitha.t@samsung.com> To: Kamil Debski <kamil@wypas.org> Cc: linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Kyungmin Park <kyungmin.park@samsung.com>, jtp.park@samsung.com, a.hajda@samsung.com, mchehab@kernel.org, pankaj.dubey@samsung.com, krzk@kernel.org, Marek Szyprowski <m.szyprowski@samsung.com>, Sylwester Nawrocki <s.nawrocki@samsung.com> Subject: Re: [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs Date: Fri, 30 Jun 2017 08:54:20 +0530 [thread overview] Message-ID: <1498793060.22203.1034.camel@smitha-fedora> (raw) In-Reply-To: <CAP3TMiG4PtiqeHhCGkHhdX_uvOhPKd=gTyckczyFKDJwnGfwYA@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: smitha.t@samsung.com (Smitha T Murthy) To: linux-arm-kernel@lists.infradead.org Subject: [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs Date: Fri, 30 Jun 2017 08:54:20 +0530 [thread overview] Message-ID: <1498793060.22203.1034.camel@smitha-fedora> (raw) In-Reply-To: <CAP3TMiG4PtiqeHhCGkHhdX_uvOhPKd=gTyckczyFKDJwnGfwYA@mail.gmail.com> 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
next prev parent reply other threads:[~2017-06-30 9:08 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <CGME20170619052453epcas1p426ebca32768cee57390f39b53d835e81@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 00/12] Add MFC v10.10 support Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052455epcas5p3b0c71e840af3b9022bf69d1058cc94e5@epcas5p3.samsung.com> 2017-06-19 5:10 ` [Patch v5 01/12] [media] s5p-mfc: Rename IS_MFCV8 macro Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052458epcas5p3ffbe45cb390e87df6bb11980cd7498aa@epcas5p3.samsung.com> 2017-06-19 5:10 ` [Patch v5 02/12] [media] s5p-mfc: Adding initial support for MFC v10.10 Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052500epcas1p477df788aee11a5a0d3e2defac97e9ae3@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 03/12] [media] s5p-mfc: Use min scratch buffer size as provided by F/W Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052502epcas1p41e0ba072755ca63278f2d4cfdc03ed06@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 04/12] [media] s5p-mfc: Support MFCv10.10 buffer requirements Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy 2017-06-27 20:30 ` Kamil Debski 2017-06-27 20:30 ` Kamil Debski 2017-06-28 5:06 ` Smitha T Murthy 2017-06-28 5:06 ` Smitha T Murthy [not found] ` <CGME20170619052505epcas5p33a78ee709263cc9ae33cd9383794ca06@epcas5p3.samsung.com> 2017-06-19 5:10 ` [Patch v5 05/12] [media] videodev2.h: Add v4l2 definition for HEVC Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy 2017-07-07 14:56 ` Stanimir Varbanov 2017-07-07 14:56 ` Stanimir Varbanov 2017-07-17 11:12 ` Smitha T Murthy 2017-07-17 11:12 ` Smitha T Murthy 2017-07-20 13:07 ` Hans Verkuil 2017-07-20 13:07 ` Hans Verkuil 2017-07-24 4:29 ` Smitha T Murthy 2017-07-24 4:29 ` Smitha T Murthy [not found] ` <CGME20170619052507epcas1p406fa9f6d84baa9c11050b1998021788a@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 06/12] [media] v4l2-ioctl: add HEVC format description Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy 2017-07-20 13:07 ` Hans Verkuil 2017-07-20 13:07 ` Hans Verkuil 2017-07-24 4:28 ` Smitha T Murthy 2017-07-24 4:28 ` Smitha T Murthy [not found] ` <CGME20170619052509epcas1p4841f9c3733280a232d35c8624fe80576@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 07/12] Documentation: v4l: Documentation for HEVC v4l2 definition Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052511epcas1p4e79efd3c9f0f8062eeac1ab4884b709e@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 08/12] [media] s5p-mfc: Add support for HEVC decoder Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052514epcas1p45d4b590d673a0ff2a3cf0117d55e656a@epcas1p4.samsung.com> 2017-06-19 5:10 ` [Patch v5 09/12] [media] s5p-mfc: Add VP9 decoder support Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy [not found] ` <CGME20170619052516epcas5p349b080cc6c242444d2db1f3c0e1c6f68@epcas5p3.samsung.com> 2017-06-19 5:10 ` [Patch v5 10/12] [media] v4l2: Add v4l2 control IDs for HEVC encoder Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy 2017-07-20 13:13 ` Hans Verkuil 2017-07-20 13:13 ` Hans Verkuil 2017-07-24 4:30 ` Smitha T Murthy 2017-07-24 4:30 ` Smitha T Murthy [not found] ` <CGME20170619052519epcas5p3bacba6c01bf2f135e5cd628226f8d24b@epcas5p3.samsung.com> 2017-06-19 5:10 ` [Patch v5 11/12] [media] s5p-mfc: Add support " Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy 2017-07-20 13:32 ` Hans Verkuil 2017-07-20 13:32 ` Hans Verkuil 2017-07-24 4:40 ` Smitha T Murthy 2017-07-24 4:40 ` Smitha T Murthy [not found] ` <CGME20170619052521epcas5p36a0bc384d10809dcfe775e6da87ed37b@epcas5p3.samsung.com> 2017-06-19 5:10 ` [Patch v5 12/12] Documention: v4l: Documentation for HEVC CIDs Smitha T Murthy 2017-06-19 5:10 ` Smitha T Murthy 2017-06-28 20:04 ` Kamil Debski 2017-06-28 20:04 ` Kamil Debski 2017-06-30 3:24 ` Smitha T Murthy [this message] 2017-06-30 3:24 ` Smitha T Murthy 2017-07-07 14:59 ` Stanimir Varbanov 2017-07-07 14:59 ` Stanimir Varbanov 2017-07-17 11:18 ` Smitha T Murthy 2017-07-17 11:18 ` Smitha T Murthy 2017-07-20 15:46 ` Stanimir Varbanov 2017-07-20 15:46 ` Stanimir Varbanov 2017-07-25 4:53 ` Smitha T Murthy 2017-07-25 4:53 ` Smitha T Murthy 2017-08-17 14:43 ` Stanimir Varbanov 2017-08-17 14:43 ` Stanimir Varbanov 2017-07-20 14:50 ` Hans Verkuil 2017-07-20 14:50 ` Hans Verkuil 2017-07-25 4:48 ` Smitha T Murthy 2017-07-25 4:48 ` Smitha T Murthy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1498793060.22203.1034.camel@smitha-fedora \ --to=smitha.t@samsung.com \ --cc=a.hajda@samsung.com \ --cc=jtp.park@samsung.com \ --cc=kamil@wypas.org \ --cc=krzk@kernel.org \ --cc=kyungmin.park@samsung.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=m.szyprowski@samsung.com \ --cc=mchehab@kernel.org \ --cc=pankaj.dubey@samsung.com \ --cc=s.nawrocki@samsung.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.