LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
To: John Cox <jc@kynesim.co.uk>
Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com>,
	Nicolas Dufresne <nicolas@ndufresne.ca>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	linux-media <linux-media@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Collabora Kernel ML <kernel@collabora.com>
Subject: Re: [PATCH] media: hevc: fix pictures lists type
Date: Fri, 27 Aug 2021 09:40:29 -0300	[thread overview]
Message-ID: <CAAEAJfA68tTeGgRHS2=hs5tQw2_3RhPgdXq6+k4GDX=0LMMBxQ@mail.gmail.com> (raw)
In-Reply-To: <mdmhigh3ubgs6r89061v19iagjs0il9b89@4ax.com>

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

  reply	other threads:[~2021-08-27 12:40 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-23  8:29 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 [this message]
2021-08-27 15:26                   ` Benjamin Gaignard
2021-08-27 18:46                     ` Ezequiel Garcia

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='CAAEAJfA68tTeGgRHS2=hs5tQw2_3RhPgdXq6+k4GDX=0LMMBxQ@mail.gmail.com' \
    --to=ezequiel@vanguardiasur.com.ar \
    --cc=benjamin.gaignard@collabora.com \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=jc@kynesim.co.uk \
    --cc=kernel@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=nicolas@ndufresne.ca \
    --subject='Re: [PATCH] media: hevc: fix pictures lists type' \
    /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: link

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).