Message-ID: <b5ae0ebe-de90-5ebb-5e69-ea66ae0e0639@collabora.com>
Date:   Fri, 27 Aug 2021 13:35:45 +0200
From:   Benjamin Gaignard <benjamin.gaignard@...labora.com>
To:     John Cox <jc@...esim.co.uk>
Cc:     Nicolas Dufresne <nicolas@...fresne.ca>, mchehab@...nel.org,
        hverkuil-cisco@...all.nl, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH] media: hevc: fix pictures lists type


Le 27/08/2021 à 12:10, John Cox a écrit :
>> Le 26/08/2021 à 18:09, Nicolas Dufresne a écrit :
>>> Le lundi 23 août 2021 à 12:35 +0100, John Cox a écrit :
>>>> Hi
>>>>
>>>>> Le 23/08/2021 à 11:50, John Cox a écrit :
>>>>>>> The lists embedded Picture Order Count values which are s32 so their type
>>>>>>> most be s32 and not u8.
>>>>>> I'm not convinced that you can't calculate all of those lists from the
>>>>>> info already contained in the DPB array so this is probably redundant
>>>>>> info though I grant that having the list pre-calced might make your life
>>>>>> easier, and the userland side will have calculated the lists to
>>>>>> calculate other required things so it isn't much extra work for it.
>>>>> Yes the userland have already compute these lists and the number of items
>>>>> in each of them.
>>>>> Build them in the kernel would means to also compute the values of NumPocStCurrBefore,
>>>>> NumPocStCurrAfter, NumPocLtCurr, NumPocStCurrAfter, NumPocStCurrBefore and NumPocLtCurr
>>>>> and that requires information (NumNegativePics, NumPositivePics...) not provided to the kernel.
>>>>> Since it have to be done in userland anyway, I'm reluctant to modify the API to redo in the kernel.
>>>> Well, fair enough, I'm not going to argue
>>>>
>>>>>> Even if you do need the lists wouldn't it be a better idea to have them
>>>>>> as indices into the DPB (you can't have a frame in any of those lists
>>>>>> that isn't in the DPB) which already contains POCs then it will still
>>>>>> fit into u8 and be smaller?
>>>>> Hantro HW works with indexes but I think it is more simple to send PoC rather than indexes.
>>>> I'd disagree but as I don't use the info I'm not concerned. Though I
>>>> think I should point out that when Hantro converts the POCs to indicies
>>>> it compares the now s32 POC in these lists with the u16 POC in the DPB
>>>> so you might need to fix that too; by std (8.3.1) no POC diff can be
>>>> outside s16 so you can mask & compare or use u16 POCs in the lists or
>>>> s32 in the DPB.
>>> Fun fact, my interpretation with the API when I drafted GStreamer support was
>>> that it was DPB indexes:
>>>
>>> https://gitlab.freedesktop.org/ndufresne/gst-plugins-bad/-/blob/hevc_wip/sys/v4l2codecs/gstv4l2codech265dec.c#L850
>>>
>>> It felt quite natural to be, since this is also how we pass references for l0/l1
>>> (unused by hantro I guess).
>>>
>>> Looking at old rkvdec code as a refresher:
>>>
>>>     for (j = 0; j < run->num_slices; j++) {
>>>                   sl_params = &run->slices_params[j];
>>>                   dpb = sl_params->dpb;
>>>
>>>                   hw_ps = &priv_tbl->rps[j];
>>>                   memset(hw_ps, 0, sizeof(*hw_ps));
>>>
>>>                   for (i = 0; i <= sl_params->num_ref_idx_l0_active_minus1; i++) {
>>>                           WRITE_RPS(!!(dpb[sl_params->ref_idx_l0[i]].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR),
>>>                                     REF_PIC_LONG_TERM_L0(i));
>>>                           WRITE_RPS(sl_params->ref_idx_l0[i], REF_PIC_IDX_L0(i));
>>>                   }
>>>
>>>                   for (i = 0; i <= sl_params->num_ref_idx_l1_active_minus1; i++) {
>>>                           WRITE_RPS(!!(dpb[sl_params->ref_idx_l1[i]].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR),
>>>                                     REF_PIC_LONG_TERM_L1(i));
>>>                           WRITE_RPS(sl_params->ref_idx_l1[i], REF_PIC_IDX_L1(i));
>>>                   }
>>>
>>>
>>> This is code is clearly unsafe, but now I remember that dpb_entry has a flag
>>> "rps". So we know from the DPB in which of the list the reference lives, if any.
>>> In the case of RKVDEC the HW only cares to know if this is long term or not.
>>>
>>> So without looking at the spec, is that dpb represention enough to reconstruct
>>> these array ? If we pass these array, shall we keep the rps flag ? I think a
>>> little step back and cleanup will be needed. I doubt there is a single answer,
>>> perhaps list what others do (VA, DXVA, NVDEC, Khronos, etc) and we can
>>> collectively decide were we want V4L2 to sit ?
>> I have done some tests with Hantro driver and look at the spec, the order of the PoC
>> in the reference lists matters. You can deducted the order for DPB rps flags.
>> I would suggest to remove rps flags to avoid information duplication.
> I want the DPB rps member for long term reference marking.  I don't care
> about before / after, but LTR can't be deduced from PoC and if you are
> going to keep the member you might as well keep before / after.

Ok so keep like it is.
In this case my patch is enough, right ?

Benjamin

>
> John Cox
>
>> Benjamin
>>
>>>> Regards
>>>>
>>>> John Cox
>>>>
>>>>> Benjamin
>>>>>
>>>>>> Full disclosure: Pi decode doesn't use this info at all so I'm only
>>>>>> arguing from a theoretical point of view - I think it is only relevant
>>>>>> if your h/w is parsing the reference list setups.
>>>>>>
>>>>>> Regards
>>>>>>
>>>>>> John Cox
>>>>>>
>>>>>>> Reported-by: John Cox <jc@...esim.co.uk>
>>>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
>>>>>>> ---
>>>>>>> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 +++---
>>>>>>> include/media/hevc-ctrls.h                                | 6 +++---
>>>>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>>> index 976d34445a24..db9859ddc8b2 100644
>>>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>>>>>>> @@ -3323,15 +3323,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>>>>>>        * - __u8
>>>>>>>          - ``num_poc_lt_curr``
>>>>>>>          - The number of reference pictures in the long-term set.
>>>>>>> -    * - __u8
>>>>>>> +    * - __s32
>>>>>>>          - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>>>>>>>          - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference
>>>>>>>            picture set.
>>>>>>> -    * - __u8
>>>>>>> +    * - __s32
>>>>>>>          - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>>>>>>>          - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference
>>>>>>>            picture set.
>>>>>>> -    * - __u8
>>>>>>> +    * - __s32
>>>>>>>          - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>>>>>>>          - PocLtCurr as described in section 8.3.2 "Decoding process for reference
>>>>>>>            picture set.
>>>>>>> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
>>>>>>> index 781371bff2ad..04cd62e77f25 100644
>>>>>>> --- a/include/media/hevc-ctrls.h
>>>>>>> +++ b/include/media/hevc-ctrls.h
>>>>>>> @@ -219,9 +219,9 @@ struct v4l2_ctrl_hevc_decode_params {
>>>>>>> 	__u8	num_poc_st_curr_before;
>>>>>>> 	__u8	num_poc_st_curr_after;
>>>>>>> 	__u8	num_poc_lt_curr;
>>>>>>> -	__u8	poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>>> -	__u8	poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>>> -	__u8	poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>>> +	__s32	poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>>> +	__s32	poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>>> +	__s32	poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>>>>>>> 	__u64	flags;
>>>>>>> };
>>>>>>>

Powered by blists - more mailing lists