On Fri, 27 Aug 2021 at 09:36, John Cox <jc@xxxxxxxxxxxxx> 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
