Messages in this thread | | | Subject | Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags | From | Benjamin Gaignard <> | Date | Fri, 7 Jan 2022 17:02:41 +0100 |
| |
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@collabora.com> > 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@vanguardiasur.com.ar> > > Also, a small question: > >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> 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
| |