Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <8e6977ac-53bb-53f0-3fdc-75aa2643cb77@collabora.com>
Date:   Tue, 26 Apr 2022 10:50:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v5 04/17] media: uapi: HEVC: Add missing fields in HEVC
 controls
Content-Language: en-US
To:     Sebastian Fricke <sebastian.fricke@collabora.com>
Cc:     mchehab@kernel.org, hverkuil@xs4all.nl,
        ezequiel@vanguardiasur.com.ar, p.zabel@pengutronix.de,
        gregkh@linuxfoundation.org, mripard@kernel.org,
        paul.kocialkowski@bootlin.com, wens@csie.org,
        jernej.skrabec@gmail.com, samuel@sholland.org,
        nicolas.dufresne@collabora.com, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-staging@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev
References: <20220407152940.738159-1-benjamin.gaignard@collabora.com>
 <20220407152940.738159-5-benjamin.gaignard@collabora.com>
 <20220425135449.oapsrqqyq34s2ii3@basti-XPS-13-9310>
 <d1ba4e57-2f13-ab3b-f79a-dc812cd4c2bf@collabora.com>
 <20220426075234.6gk72hzf2bgaxwqw@basti-XPS-13-9310>
From:   Benjamin Gaignard <benjamin.gaignard@collabora.com>
In-Reply-To: <20220426075234.6gk72hzf2bgaxwqw@basti-XPS-13-9310>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Le 26/04/2022 à 09:52, Sebastian Fricke a écrit :
> Hey Benjamin,
>
> On 25.04.2022 18:16, Benjamin Gaignard wrote:
>>
>> Le 25/04/2022 à 15:54, Sebastian Fricke a écrit :
>>> On 07.04.2022 17:29, Benjamin Gaignard wrote:
>>>> Complete the HEVC controls with missing fields from H.265 
>>>> specifications.
>>>> Even if these fields aren't used by the current mainlined drivers
>>>> they will be need for (at least) rkvdec driver.
>>>
>>> s/be need/be required/
>>> or
>>> s/be need/be needed/
>>>
>>> s/rkvdec/the rkvdec/
>>>
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>> .../media/v4l/ext-ctrls-codec.rst             | 19 +++++++++++++++++++
>>>> include/media/hevc-ctrls.h                    |  6 +++++-
>>>> 2 files changed, 24 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git 
>>>> a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst 
>>>> b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> index 4cd7c541fc30..dbb08603217b 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>> @@ -2661,6 +2661,16 @@ enum 
>>>> v4l2_mpeg_video_hevc_size_of_length_field -
>>>>     :stub-columns: 0
>>>>     :widths:       1 1 2
>>>>
>>>> +    * - __u8
>>>> +      - ``video_parameter_set_id``
>>>> +      - Specifies the value of the vps_video_parameter_set_id of 
>>>> the active VPS
>>>> +        as descibed in section "7.4.3.2.1 General sequence 
>>>> parameter set RBSP semantics"
>>>> +        of H.265 specifications.
>>>> +    * - __u8
>>>> +      - ``seq_parameter_set_id``
>>>> +      - Provides an identifier for the SPS for reference by other 
>>>> syntax elements
>>>> +        as descibed in section "7.4.3.2.1 General sequence 
>>>> parameter set RBSP semantics"
>>>> +        of H.265 specifications.
>>>>     * - __u16
>>>>       - ``pic_width_in_luma_samples``
>>>>       -
>>>> @@ -2800,6 +2810,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>     :stub-columns: 0
>>>>     :widths:       1 1 2
>>>>
>>>> +    * - __u8
>>>> +      - ``pic_parameter_set_id``
>>>> +      - Identifies the PPS for reference by other syntax elements.
>>>>     * - __u8
>>>>       - ``num_extra_slice_header_bits``
>>>>       -
>>>> @@ -3026,6 +3039,12 @@ enum 
>>>> v4l2_mpeg_video_hevc_size_of_length_field -
>>>>     * - __u8
>>>>       - ``ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>>>>       - The list of L1 reference elements as indices in the DPB.
>>>> +    * - __u16
>>>> +      - ``short_term_ref_pic_set_size``
>>>> +      - Specifies the size of short-term reference pictures set 
>>>> included in the SPS.
>>>
>>> s/size of/size of the/
>>>
>>> Section 7.4.8 depicts that the st_ref_pic_set syntax
>>> structure can be part of the SPS or the slice header.
>>>
>>> I think we should mention that we talk about the size of the 
>>> st_ref_pic_set
>>> syntax structure from section 7.4.8 of the specification.
>>>> +    * - __u16
>>>> +      - ``long_term_ref_pic_set_size``
>>>> +      - Specifies the size of long-term reference pictures set 
>>>> include in the SPS.
>>>
>>> s/size of/size of the/
>>>
>>> Can we make this a bit more helpful? The specification doesn't contain
>>> a similar structure to `st_ref_pic_set` for long term pictures. So, 
>>> as a
>>> programmer this leaves me guessing:
>>> - Which syntax structure's size are we talking about?
>>> - Does this correlate to any of the existing sections of the
>>> specification?
>>> Because in the end, I feel like this documentation should be able to
>>> help a programmer to provide the correct data for the uABI.
>>
>> I will reword it like that:
>>
>>   * - __u16
>>     - ``short_term_ref_pic_set_size``
>>     - Specifies the size, in bits, of the short-term reference 
>> pictures set, described as st_ref_pic_set()
>
> Just ...
>
> s/reference pictures set/reference picture set/
>
>>      in the specification, included in the slice header (section 
>> 7.3.6.1).
>
> s/slice header/slice header or SPS/
>
>
>>
>> * - __u16
>>    - ``long_term_ref_pic_set_size``
>>    - Specifies the size, in bits, of the long-term reference pictures 
>> set include in the slice header.
>
> s/reference pictures set/reference picture set/
>
> Looking at the documentation it looks like: s/slice header/slice 
> header or SPS/
> (For example in section 7.3.6.1:
> ```
> if( long_term_ref_pics_present_flag ) {
>   if( num_long_term_ref_pics_sps > 0 )
>     num_long_term_sps
>   num_long_term_pics
>   for( i = 0; i < num_long_term_sps + num_long_term_pics; i++ ) {
>     ...
> ```
> )
>
> ... and then it looks very good!

I don't know the syntax to insert this kind of comment inside an array in the documentation
so it will remain as before :-)

Benjamin

>
> Greetings,
> Sebastian
>
>>      It is the number of bits in the conditional block if( 
>> long_term_ref_pics_present_flag ) {...}
>>      in section 7.3.6.1 of the specification.
>>
>> Benjamin
>>
>>
>>>
>>> Greetings,
>>> Sebastian
>>>
>>>>     * - __u8
>>>>       - ``padding``
>>>>       - Applications and drivers must set this to zero.
>>>> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
>>>> index 01ccda48d8c5..a329e086a89a 100644
>>>> --- a/include/media/hevc-ctrls.h
>>>> +++ b/include/media/hevc-ctrls.h
>>>> @@ -58,6 +58,8 @@ enum v4l2_mpeg_video_hevc_start_code {
>>>> /* The controls are not stable at the moment and will likely be 
>>>> reworked. */
>>>> struct v4l2_ctrl_hevc_sps {
>>>>     /* ISO/IEC 23008-2, ITU-T Rec. H.265: Sequence parameter set */
>>>> +    __u8    video_parameter_set_id;
>>>> +    __u8    seq_parameter_set_id;
>>>>     __u16    pic_width_in_luma_samples;
>>>>     __u16    pic_height_in_luma_samples;
>>>>     __u8    bit_depth_luma_minus8;
>>>> @@ -108,6 +110,7 @@ struct v4l2_ctrl_hevc_sps {
>>>>
>>>> struct v4l2_ctrl_hevc_pps {
>>>>     /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture parameter set */
>>>> +    __u8    pic_parameter_set_id;
>>>>     __u8    num_extra_slice_header_bits;
>>>>     __u8    num_ref_idx_l0_default_active_minus1;
>>>>     __u8    num_ref_idx_l1_default_active_minus1;
>>>> @@ -199,7 +202,8 @@ struct v4l2_ctrl_hevc_slice_params {
>>>>     __u32    slice_segment_addr;
>>>>     __u8    ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>     __u8    ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>> -
>>>> +    __u16    short_term_ref_pic_set_size;
>>>> +    __u16    long_term_ref_pic_set_size;
>>>>     __u8    padding;
>>>>
>>>>     /* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction 
>>>> parameter */
>>>> -- 
>>>> 2.32.0
>>>>