From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-08-31 09:49:25
This series aims to clean up Reference Picture Set usage and flags.
Long term flag was named with RPS prefix while it is not used for RPS
but for mark long term references in DBP. Remane it and remove the two
other useless RPS flags.
Clarify documentation about RPS lists content and make sure that Hantro
driver use them correctly (i.e without look up in DBP).
Benjamin
Benjamin Gaignard (2):
media: hevc: Remove RPS named flags
media: hevc: Embedded indexes in RPS
.../media/v4l/ext-ctrls-codec.rst | 12 ++++-----
.../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++++--------------
.../staging/media/sunxi/cedrus/cedrus_h265.c | 2 +-
include/media/hevc-ctrls.h | 4 +--
4 files changed, 14 insertions(+), 31 deletions(-)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-08-31 09:49:18
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(-)
@@ -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.
@@ -169,7 +169,7 @@ static void cedrus_h265_ref_pic_list_write(struct cedrus_dev *dev,unsignedintindex=list[i];u8value=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. */
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-08-31 09:49:22
Reference Picture Set lists provide indexes of short and long term
reference in DBP array.
Fix Hantro to not do a look up in DBP entries.
Make documentation more clear about it.
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
.../media/v4l/ext-ctrls-codec.rst | 6 ++---
.../staging/media/hantro/hantro_g2_hevc_dec.c | 25 +++++--------------
2 files changed, 9 insertions(+), 22 deletions(-)
@@ -3352,15 +3352,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -* - __u8-``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``- PocStCurrBefore as described in section 8.3.2 "Decoding process for reference- picture set.+ picture set": provides the index of the short term before references in DPB array.* - __u8-``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``- PocStCurrAfter as described in section 8.3.2 "Decoding process for reference- picture set.+ picture set": provides the index of the short term after references in DPB array.* - __u8-``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``- PocLtCurr as described in section 8.3.2 "Decoding process for reference- picture set.+ picture set": provides the index of the long term references in DPB array.* - __u64-``flags``- See :ref:`Decode Parameters Flags <hevc_decode_params_flags>`
@@ -367,11 +354,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx)/* List 0 contains: short term before, short term after and long term */j=0;for(i=0;i<decode_params->num_poc_st_curr_before&&j<ARRAY_SIZE(list0);i++)-list0[j++]=find_ref_pic_index(dpb,decode_params->poc_st_curr_before[i]);+list0[j++]=decode_params->poc_st_curr_before[i];for(i=0;i<decode_params->num_poc_st_curr_after&&j<ARRAY_SIZE(list0);i++)-list0[j++]=find_ref_pic_index(dpb,decode_params->poc_st_curr_after[i]);+list0[j++]=decode_params->poc_st_curr_after[i];for(i=0;i<decode_params->num_poc_lt_curr&&j<ARRAY_SIZE(list0);i++)-list0[j++]=find_ref_pic_index(dpb,decode_params->poc_lt_curr[i]);+list0[j++]=decode_params->poc_lt_curr[i];/* Fill the list, copying over and over */i=0;
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(-)
: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.
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.)
John Cox
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. */
From: Benjamin Gaignard <benjamin.gaignard@collabora.com> Date: 2021-08-31 11:56:45
Le 31/08/2021 à 13:08, John Cox a écrit :
quoted
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(-)
: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.)
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. */
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(-)
: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.
OK. I was going to say that you should merge the "field_pic" entry into
a flags bitfield, but then I remembered that H265 doesn't have the
concept of field_pics in the way that H264 does and I believe that both
it and pic_order_count[1] are redundant (i.e. can never be used). But I
guess that is the subject of yet another patch.
Regards
John Cox
quoted
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.)
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. */
From: Nicolas Dufresne <hidden> Date: 2021-08-31 19:19:27
As per discussion with Ezequiel, I think this patch should perhaps be made an
RFC titled "media: uapi: clean and make HEVC uAPI public". Keep it RFC until we
have collected all the needed changes.
Opinion ?
Le mardi 31 août 2021 à 11:48 +0200, Benjamin Gaignard a écrit :
This series aims to clean up Reference Picture Set usage and flags.
Long term flag was named with RPS prefix while it is not used for RPS
but for mark long term references in DBP. Remane it and remove the two
other useless RPS flags.
Clarify documentation about RPS lists content and make sure that Hantro
driver use them correctly (i.e without look up in DBP).
Benjamin
Benjamin Gaignard (2):
media: hevc: Remove RPS named flags
media: hevc: Embedded indexes in RPS
.../media/v4l/ext-ctrls-codec.rst | 12 ++++-----
.../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++++--------------
.../staging/media/sunxi/cedrus/cedrus_h265.c | 2 +-
include/media/hevc-ctrls.h | 4 +--
4 files changed, 14 insertions(+), 31 deletions(-)
Hans, Nicolas,
On Tue, 31 Aug 2021 at 16:19, Nicolas Dufresne [off-list ref] wrote:
As per discussion with Ezequiel, I think this patch should perhaps be made an
RFC titled "media: uapi: clean and make HEVC uAPI public". Keep it RFC until we
have collected all the needed changes.
Opinion ?
I agree.
IMO, it makes no sense to keep merging anything on Hantro G2 HEVC
support, until the BLK-CTRL part is ready [1]. Without that, we can't
test anything
so we may introduce regressions without knowing.
Also, I think it's time to start discussing the HEVC uAPI as a whole,
with the goal is having a stable interface that can be used by userspace
frameworks (GStreamer).
[1] https://www.spinics.net/lists/arm-kernel/msg913881.html
Thanks,
Ezequiel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel