Messages in this thread | | | Subject | Re: [PATCH 1/2] media: hevc: Remove RPS named flags | From | Benjamin Gaignard <> | Date | Tue, 31 Aug 2021 13:56:36 +0200 |
| |
Le 31/08/2021 à 13:08, John Cox a écrit : >> 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. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 ++---- >> drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +- >> drivers/staging/media/sunxi/cedrus/cedrus_h265.c | 2 +- >> include/media/hevc-ctrls.h | 4 +--- >> 4 files changed, 5 insertions(+), 9 deletions(-) >> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> index 3865acb9e0fd..eff33c511090 100644 >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >> @@ -3138,10 +3138,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - >> :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) >> + - Long term flag for the reference frame >> + (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) >> * - __u8 >> - ``field_pic`` >> - Whether the reference is a field picture or a frame. > If you are going to remove all the RPS values except for Long Term > wouldn't it be better to rename the field too, either to "flags" or a > bool "is_long_term"? If we have a field called RPS it really should be > able to have a value for any of the 5 valid Reference Picture Sets that > a DPB entry can belong to.
I will send a v2 and rename rps into flags.
> > As a side note, it is important to my code that the DPB array contains > all the DPB entries not just the ones that are in use in this frame. I > need them so I can track which frames have left the DPB so I can > reuse/free the MV tables associated with them (yes I could keep one for > every entry in the capture Q but that is generally wasteful on memory > and the Pi is often memory constrained). So maybe update the docn on DPB > to make this explicit please? (I suspect that current code does this > anyway as it is generally easier to do than to not.)
That should be in another patch :-)
Benjamin
> > John Cox > >> diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c >> index 9ea864ca5625..be46b3c28b17 100644 >> --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c >> +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c >> @@ -503,7 +503,7 @@ static int set_ref(struct hantro_ctx *ctx) >> compress_luma_addr = luma_addr + compress_luma_offset; >> compress_chroma_addr = luma_addr + compress_chroma_offset; >> >> - if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) >> + if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) >> dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i); >> >> /* >> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >> index ef0311a16d01..6086cc35e8cc 100644 >> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c >> @@ -169,7 +169,7 @@ static void cedrus_h265_ref_pic_list_write(struct cedrus_dev *dev, >> unsigned int index = list[i]; >> u8 value = list[i]; >> >> - if (dpb[index].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR) >> + if (dpb[index].rps == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE) >> value |= VE_DEC_H265_SRAM_REF_PIC_LIST_LT_REF; >> >> /* Each SRAM word gathers up to 4 references. */ >> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h >> index ef63bc205756..f587448ef495 100644 >> --- a/include/media/hevc-ctrls.h >> +++ b/include/media/hevc-ctrls.h >> @@ -127,9 +127,7 @@ struct v4l2_ctrl_hevc_pps { >> __u64 flags; >> }; >> >> -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE 0x01 >> -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER 0x02 >> -#define V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR 0x03 >> +#define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE 0x01 >> >> #define V4L2_HEVC_DPB_ENTRIES_NUM_MAX 16 >>
| |