* [PATCH] media: hevc: fix pictures lists type @ 2021-08-23 8:29 Benjamin Gaignard 2021-08-23 9:50 ` John Cox 0 siblings, 1 reply; 12+ messages in thread From: Benjamin Gaignard @ 2021-08-23 8:29 UTC (permalink / raw) To: mchehab, hverkuil-cisco, jc, nicolas Cc: linux-media, linux-kernel, kernel, Benjamin Gaignard The lists embedded Picture Order Count values which are s32 so their type most be s32 and not u8. Reported-by: John Cox <jc@kynesim.co.uk> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-23 8:29 [PATCH] media: hevc: fix pictures lists type Benjamin Gaignard @ 2021-08-23 9:50 ` John Cox 2021-08-23 11:17 ` Benjamin Gaignard 0 siblings, 1 reply; 12+ messages in thread From: John Cox @ 2021-08-23 9:50 UTC (permalink / raw) To: Benjamin Gaignard Cc: mchehab, hverkuil-cisco, nicolas, linux-media, linux-kernel, kernel, Benjamin Gaignard >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. 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? 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@kynesim.co.uk> >Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; > }; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-23 9:50 ` John Cox @ 2021-08-23 11:17 ` Benjamin Gaignard 2021-08-23 11:35 ` John Cox 0 siblings, 1 reply; 12+ messages in thread From: Benjamin Gaignard @ 2021-08-23 11:17 UTC (permalink / raw) To: John Cox Cc: mchehab, hverkuil-cisco, nicolas, linux-media, linux-kernel, kernel 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. > > 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. 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@kynesim.co.uk> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; >> }; >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-23 11:17 ` Benjamin Gaignard @ 2021-08-23 11:35 ` John Cox 2021-08-26 16:09 ` Nicolas Dufresne 0 siblings, 1 reply; 12+ messages in thread From: John Cox @ 2021-08-23 11:35 UTC (permalink / raw) To: Benjamin Gaignard Cc: mchehab, hverkuil-cisco, nicolas, linux-media, linux-kernel, kernel 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. 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@kynesim.co.uk> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; >>> }; >>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-23 11:35 ` John Cox @ 2021-08-26 16:09 ` Nicolas Dufresne 2021-08-27 8:55 ` Benjamin Gaignard 0 siblings, 1 reply; 12+ messages in thread From: Nicolas Dufresne @ 2021-08-26 16:09 UTC (permalink / raw) To: John Cox, Benjamin Gaignard Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel, kernel 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 ? > > 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@kynesim.co.uk> > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; > > > > }; > > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-26 16:09 ` Nicolas Dufresne @ 2021-08-27 8:55 ` Benjamin Gaignard 2021-08-27 10:10 ` John Cox 0 siblings, 1 reply; 12+ messages in thread From: Benjamin Gaignard @ 2021-08-27 8:55 UTC (permalink / raw) To: Nicolas Dufresne, John Cox Cc: mchehab, hverkuil-cisco, linux-media, linux-kernel, kernel 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. 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@kynesim.co.uk> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; >>>>> }; >>>>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-27 8:55 ` Benjamin Gaignard @ 2021-08-27 10:10 ` John Cox 2021-08-27 11:35 ` Benjamin Gaignard 0 siblings, 1 reply; 12+ messages in thread From: John Cox @ 2021-08-27 10:10 UTC (permalink / raw) To: Benjamin Gaignard Cc: Nicolas Dufresne, mchehab, hverkuil-cisco, linux-media, linux-kernel, kernel >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. 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@kynesim.co.uk> >>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; >>>>>> }; >>>>>> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-27 10:10 ` John Cox @ 2021-08-27 11:35 ` Benjamin Gaignard 2021-08-27 12:36 ` John Cox 0 siblings, 1 reply; 12+ messages in thread From: Benjamin Gaignard @ 2021-08-27 11:35 UTC (permalink / raw) To: John Cox Cc: Nicolas Dufresne, mchehab, hverkuil-cisco, linux-media, linux-kernel, kernel 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@kynesim.co.uk> >>>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; >>>>>>> }; >>>>>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-27 11:35 ` Benjamin Gaignard @ 2021-08-27 12:36 ` John Cox 2021-08-27 12:40 ` Ezequiel Garcia 0 siblings, 1 reply; 12+ messages in thread From: John Cox @ 2021-08-27 12:36 UTC (permalink / raw) To: Benjamin Gaignard Cc: Nicolas Dufresne, mchehab, hverkuil-cisco, linux-media, linux-kernel, kernel >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 ? I still think there are better and smaller ways of constructing the lists Hantro wants (e.g. using an index into the DPB as the L0/L1 lists do) but this patch is capable of delivering the result you need. Regards John Cox >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@kynesim.co.uk> >>>>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.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; >>>>>>>> }; >>>>>>>> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-27 12:36 ` John Cox @ 2021-08-27 12:40 ` Ezequiel Garcia 2021-08-27 15:26 ` Benjamin Gaignard 0 siblings, 1 reply; 12+ messages in thread From: Ezequiel Garcia @ 2021-08-27 12:40 UTC (permalink / raw) To: John Cox Cc: Benjamin Gaignard, Nicolas Dufresne, Mauro Carvalho Chehab, Hans Verkuil, linux-media, Linux Kernel Mailing List, Collabora Kernel ML On Fri, 27 Aug 2021 at 09:36, John Cox <jc@kynesim.co.uk> wrote: > > >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 ? > The problem with the patch is that it breaks existing userspace. Currently, there's no upstreamed userspace so this is not a huge deal. However, it's definitely not a good practice. Even if these are staging controls, I think a proper action item is to start discussing what's missing on the HEVC interface as a whole, so it can be moved to stable. Otherwise, it almost feels like bikeshading. Thanks, Ezequiel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-27 12:40 ` Ezequiel Garcia @ 2021-08-27 15:26 ` Benjamin Gaignard 2021-08-27 18:46 ` Ezequiel Garcia 0 siblings, 1 reply; 12+ messages in thread From: Benjamin Gaignard @ 2021-08-27 15:26 UTC (permalink / raw) To: Ezequiel Garcia, John Cox Cc: Nicolas Dufresne, Mauro Carvalho Chehab, Hans Verkuil, linux-media, Linux Kernel Mailing List, Collabora Kernel ML Le 27/08/2021 à 14:40, Ezequiel Garcia a écrit : > On Fri, 27 Aug 2021 at 09:36, John Cox <jc@kynesim.co.uk> wrote: >>> 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 ? > The problem with the patch is that it breaks existing userspace. > Currently, there's no upstreamed userspace so this is not a huge > deal. > > However, it's definitely not a good practice. Even if these are > staging controls, I think a proper action item is to start discussing > what's missing on the HEVC interface as a whole, so it can be > moved to stable. I do agree I think it could the time to talk about moving the API to stable. My plan is to get this patch merge before sending a RFC to move the API. Benjamin > > Otherwise, it almost feels like bikeshading. > > Thanks, > Ezequiel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] media: hevc: fix pictures lists type 2021-08-27 15:26 ` Benjamin Gaignard @ 2021-08-27 18:46 ` Ezequiel Garcia 0 siblings, 0 replies; 12+ messages in thread From: Ezequiel Garcia @ 2021-08-27 18:46 UTC (permalink / raw) To: Benjamin Gaignard Cc: John Cox, Nicolas Dufresne, Mauro Carvalho Chehab, Hans Verkuil, linux-media, Linux Kernel Mailing List, Collabora Kernel ML, Daniel Almeida On Fri, 27 Aug 2021 at 12:26, Benjamin Gaignard <benjamin.gaignard@collabora.com> wrote: > > > Le 27/08/2021 à 14:40, Ezequiel Garcia a écrit : > > On Fri, 27 Aug 2021 at 09:36, John Cox <jc@kynesim.co.uk> wrote: > >>> 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 ? > > The problem with the patch is that it breaks existing userspace. > > Currently, there's no upstreamed userspace so this is not a huge > > deal. > > > > However, it's definitely not a good practice. Even if these are > > staging controls, I think a proper action item is to start discussing > > what's missing on the HEVC interface as a whole, so it can be > > moved to stable. > > I do agree I think it could the time to talk about moving the API to stable. > My plan is to get this patch merge before sending a RFC to move the API. > I'd rather not merge this patch, and instead merge all the changes in one go, so we avoid making further changes to the API. The patch iself might look good, but I'd rather have the discussion on the big picture, and make sure we can all review the interface against the spec. BTW, this should probably mean we have a plan to extend APIs if we need to. Daniel Almeida (Cc) has the details for my proposal on that front, if you want to push it forward. Thanks, Ezequiel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-08-27 18:47 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-23 8:29 [PATCH] media: hevc: fix pictures lists type Benjamin Gaignard 2021-08-23 9:50 ` John Cox 2021-08-23 11:17 ` Benjamin Gaignard 2021-08-23 11:35 ` John Cox 2021-08-26 16:09 ` Nicolas Dufresne 2021-08-27 8:55 ` Benjamin Gaignard 2021-08-27 10:10 ` John Cox 2021-08-27 11:35 ` Benjamin Gaignard 2021-08-27 12:36 ` John Cox 2021-08-27 12:40 ` Ezequiel Garcia 2021-08-27 15:26 ` Benjamin Gaignard 2021-08-27 18:46 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).