Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags

[Benjamin Gaignard]

Le 05/01/2022 à 13:26, Ezequiel Garcia a écrit :
> Hi Benjamin,
>
> On Tue, Jan 04, 2022 at 08:38:41AM +0100, Benjamin Gaignard wrote:
> > Marking a picture as long-term reference is valid for DPB but not for RPS.
> > Change flag name to match with it description in HEVC spec chapiter
> > "8.3.2 Decoding process for reference picture set".
> > Remove the other unused RPS flags.
> A change like this, which is affecting a kernel interface and has impact
> on userspace and drivers, requires a lot more attention in the commit description.
>
> If I understand correctly, this change is related to how HEVC was first
> introduced and how the DPB was originally represented in V4L2.
>
> Originally, a DPB entry struct v4l2_hevc_dpb_entry had an rps field
> which can be:
>
>    V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE
>    V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER
>    V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR
>
> Perhaps this idea followed libva, where a VAPictureHEVC has flags field
> which can be:
>
>    VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE,
>    VA_PICTURE_HEVC_RPS_ST_CURR_AFTER,
>    VA_PICTURE_HEVC_RPS_LT_CURR,
>    VA_PICTURE_HEVC_LONG_TERM_REFERENCE
>
> In this representation, the sets PocStCurrBefore, PocStCurrAfter,
> PocLtCurr are implicit, and must be built by the kernel from the DPB
> entries struct v4l2_hevc_dpb_entry, using the information in the rps field.
>
> Last year, we changed this with your commit:
>
> commit d395a78db9eabd12633b39e05c80e803543b6590
> Author: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> Date:   Thu Jun 3 13:49:57 2021 +0200
>
>      media: hevc: Add decode params control
>
> Which added the control v4l2_ctrl_hevc_decode_params, which includes
> the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr explicitly and therefore
> moved the responsability of creating and maintaining the sets to userspace.
>
> This effectively made the rps field in the struct v4l2_hevc_dpb_entry
> useless. The longterm flag is still needed though for a DPB entry.
>
> With this in mind, you could even say this commit is doing actually two
> things:
>
> 1. Removes the unused rps field.
> 2. Adds a flag field, for the longterm DPB entry boolean.
>
> Do you think a single byte of flags will be OK for a DPB entry?
> I think so, but I'd love yours/others input here.

In ITU HEVC spec it is said that:
"When a picture is referred to as being marked as "used for reference",
this collectively refers to the picture being marked as "used
 for
short-term reference" or "used for long-term reference" (but not both)."

So I believe that a single if enough for DPB entry.

> If the above is more or less accurate then, and provided you
> submit a new version with a more detailed commit description:

I will follow your suggestions and improve it in v5

> Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
>
> Also, a small question:
>
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> > ---
> > version 4:
> > - check flags with & and not ==
> >
> >   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++-----
> >   drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
> >   drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
> >   include/media/hevc-ctrls.h                                | 6 ++----
> >   4 files changed, 7 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > index e141f0e4eec9..38da33e61c3d 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> > @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> >   	:c:func:`v4l2_timeval_to_ns()` function to convert the struct
> >   	:c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
> >       * - __u8
> > -      - ``rps``
> > -      - The reference set for the reference frame
> > -        (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE,
> > -        V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or
> > -        V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
> > +      - ``flags``
> > +      - Long term flag for the reference frame
> > +        (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
> Is this longterm flag associated in any way to a syntax element
> or an HEVC process? If so, please document that.

done in v5.

Thanks for your review.

Benjamin

> Thanks,
> Ezequiel