| Message ID | 20200715202233.185680-7-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Clean H264 stateless uAPI | expand |
Hi! Dne sreda, 15. julij 2020 ob 22:22:29 CEST je Ezequiel Garcia napisal(a): > As discussed recently, the current interface for the > Decoded Picture Buffer is not enough to properly > support field coding. > > This commit introduces enough semantics to support > frame and field coding, and to signal how DPB entries > are "used for reference". > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- > drivers/media/v4l2-core/v4l2-h264.c | 4 +- > drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- > include/media/h264-ctrls.h | 8 +++- > 4 files changed, 42 insertions(+), 24 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > dd8e5a2e8986..46d4c8c6ad47 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > * - __s32 > - ``bottom_field_order_cnt`` > - > + * - enum :c:type:`v4l2_h264_dpb_reference` > + - ``reference`` > + - Specifies how the DPB entry is referenced. > * - __u32 > - ``flags`` > - See :ref:`DPB Entry Flags <h264_dpb_flags>` > > +.. c:type:: v4l2_h264_dpb_reference > + > +.. cssclass:: longtable > + > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + :widths: 1 1 2 > + > + * - ``V4L2_H264_DPB_TOP_REF`` > + - 0x1 > + - The top field in field pair is used for > + short-term reference. > + * - ``V4L2_H264_DPB_BOTTOM_REF`` > + - 0x2 > + - The bottom field in field pair is used for > + short-term reference. > + * - ``V4L2_H264_DPB_FRAME_REF`` > + - 0x3 > + - The frame (or the top/bottom fields, if it's a field pair) > + is used for short-term reference. > + > .. _h264_dpb_flags: > > ``DPB Entries Flags`` > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` > - 0x00000001 > - - The DPB entry is valid and should be considered > + - The DPB entry is valid (non-empty) and should be considered. > * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` I'm still not sure that we actually need both flags. Technically, if entry is not used for reference then doesn't need to be present. Am I missing something? Best regards, Jernej > - 0x00000002 > - - The DPB entry is currently being used as a reference frame > + - The DPB entry is used for reference. > * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` > - 0x00000004 > - - The DPB entry is a long term reference frame > + - The DPB entry is used for long-term reference. > * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` > - 0x00000008 > - - The DPB entry is a field reference, which means only one of the > field - will be used when decoding the new frame/field. When not set > the DPB - entry is a frame reference (both fields will be used). > Note that this - flag does not say anything about the number of > fields contained in the - reference frame, it just describes the one > used to decode the new - field/frame > - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` > - - 0x00000010 > - - The DPB entry is a bottom field reference (only the bottom field of > the - reference frame is needed to decode the new frame/field). Only > valid if - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but > - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the > - DPB entry is a top field reference > + - The DPB entry is a single field or a complementary field pair. > > ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` > Specifies the decoding mode to use. Currently exposes slice-based and > diff --git a/drivers/media/v4l2-core/v4l2-h264.c > b/drivers/media/v4l2-core/v4l2-h264.c index edf6225f0522..306a51683606 > 100644 > --- a/drivers/media/v4l2-core/v4l2-h264.c > +++ b/drivers/media/v4l2-core/v4l2-h264.c > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct > v4l2_h264_reflist_builder *b, else > b->refs[i].frame_num = dpb[i].frame_num; > > - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) > pic_order_count = min(dpb[i].top_field_order_cnt, > dpb[i].bottom_field_order_cnt); > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > pic_order_count = dpb[i].bottom_field_order_cnt; > else > pic_order_count = dpb[i].top_field_order_cnt; > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c > b/drivers/staging/media/rkvdec/rkvdec-h264.c index > 7b66e2743a4f..57539c630422 100644 > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, > RKVDEC_COLMV_USED_FLAG_REF; > > if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > - refer_addr |= RKVDEC_TOPFIELD_USED_REF | > - RKVDEC_BOTFIELD_USED_REF; > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > + refer_addr |= RKVDEC_FIELD_REF; > + > + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) > refer_addr |= RKVDEC_BOTFIELD_USED_REF; > - else > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > refer_addr |= RKVDEC_TOPFIELD_USED_REF; > > writel_relaxed(dpb[i].top_field_order_cnt, > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > index 620ee8863d74..52f3976b986c 100644 > --- a/include/media/h264-ctrls.h > +++ b/include/media/h264-ctrls.h > @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params { > #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE 0x02 > #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM 0x04 > #define V4L2_H264_DPB_ENTRY_FLAG_FIELD 0x08 > -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD 0x10 > + > +enum v4l2_h264_dpb_reference { > + V4L2_H264_DPB_TOP_REF = 0x1, > + V4L2_H264_DPB_BOTTOM_REF = 0x2, > + V4L2_H264_DPB_FRAME_REF = 0x3, > +}; > > struct v4l2_h264_dpb_entry { > __u64 reference_ts; > @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry { > /* Note that field is indicated by v4l2_buffer.field */ > __s32 top_field_order_cnt; > __s32 bottom_field_order_cnt; > + enum v4l2_h264_dpb_reference reference; > __u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */ > };
On Wed, 2020-07-22 at 18:09 +0200, Jernej Škrabec wrote: > Hi! > > Dne sreda, 15. julij 2020 ob 22:22:29 CEST je Ezequiel Garcia napisal(a): > > As discussed recently, the current interface for the > > Decoded Picture Buffer is not enough to properly > > support field coding. > > > > This commit introduces enough semantics to support > > frame and field coding, and to signal how DPB entries > > are "used for reference". > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- > > drivers/media/v4l2-core/v4l2-h264.c | 4 +- > > drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- > > include/media/h264-ctrls.h | 8 +++- > > 4 files changed, 42 insertions(+), 24 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index > > dd8e5a2e8986..46d4c8c6ad47 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > * - __s32 > > - ``bottom_field_order_cnt`` > > - > > + * - enum :c:type:`v4l2_h264_dpb_reference` > > + - ``reference`` > > + - Specifies how the DPB entry is referenced. > > * - __u32 > > - ``flags`` > > - See :ref:`DPB Entry Flags <h264_dpb_flags>` > > > > +.. c:type:: v4l2_h264_dpb_reference > > + > > +.. cssclass:: longtable > > + > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + :widths: 1 1 2 > > + > > + * - ``V4L2_H264_DPB_TOP_REF`` > > + - 0x1 > > + - The top field in field pair is used for > > + short-term reference. > > + * - ``V4L2_H264_DPB_BOTTOM_REF`` > > + - 0x2 > > + - The bottom field in field pair is used for > > + short-term reference. > > + * - ``V4L2_H264_DPB_FRAME_REF`` > > + - 0x3 > > + - The frame (or the top/bottom fields, if it's a field pair) > > + is used for short-term reference. > > + > > .. _h264_dpb_flags: > > > > ``DPB Entries Flags`` > > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` > > - 0x00000001 > > - - The DPB entry is valid and should be considered > > + - The DPB entry is valid (non-empty) and should be considered. > > * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` > > I'm still not sure that we actually need both flags. Technically, if entry is > not used for reference then doesn't need to be present. Am I missing > something? > Indeed, that's exactly what I raised during the review of these flags. However, note that the Cedrus driver seem to do something with VALID but not ACTIVE DPB entries, although maybe that's not really needed. If you could check the code and drop the VALID usage, and confirm ACTIVE is the only flag needed, that would be nice. Now, OTOH, having an extra flag is not hurting us. It might be more consistent for applications, as it would mean simpler setup of the DPB, and more consistent mapping between application kept DPB and kernel DPB. Thanks, Ezequiel > Best regards, > Jernej > > > - 0x00000002 > > - - The DPB entry is currently being used as a reference frame > > + - The DPB entry is used for reference. > > * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` > > - 0x00000004 > > - - The DPB entry is a long term reference frame > > + - The DPB entry is used for long-term reference. > > * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` > > - 0x00000008 > > - - The DPB entry is a field reference, which means only one of the > > field - will be used when decoding the new frame/field. When not set > > the DPB - entry is a frame reference (both fields will be used). > > Note that this - flag does not say anything about the number of > > fields contained in the - reference frame, it just describes the one > > used to decode the new - field/frame > > - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` > > - - 0x00000010 > > - - The DPB entry is a bottom field reference (only the bottom field of > > the - reference frame is needed to decode the new frame/field). Only > > valid if - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When > > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but > > - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the > > - DPB entry is a top field reference > > + - The DPB entry is a single field or a complementary field pair. > > > > ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` > > Specifies the decoding mode to use. Currently exposes slice-based and > > diff --git a/drivers/media/v4l2-core/v4l2-h264.c > > b/drivers/media/v4l2-core/v4l2-h264.c index edf6225f0522..306a51683606 > > 100644 > > --- a/drivers/media/v4l2-core/v4l2-h264.c > > +++ b/drivers/media/v4l2-core/v4l2-h264.c > > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct > > v4l2_h264_reflist_builder *b, else > > b->refs[i].frame_num = dpb[i].frame_num; > > > > - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > > + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) > > pic_order_count = > min(dpb[i].top_field_order_cnt, > > > dpb[i].bottom_field_order_cnt); > > - else if (dpb[i].flags & > V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > > pic_order_count = > dpb[i].bottom_field_order_cnt; > > else > > pic_order_count = dpb[i].top_field_order_cnt; > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c > > b/drivers/staging/media/rkvdec/rkvdec-h264.c index > > 7b66e2743a4f..57539c630422 100644 > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, > > RKVDEC_COLMV_USED_FLAG_REF; > > > > if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > > - refer_addr |= RKVDEC_TOPFIELD_USED_REF | > > - RKVDEC_BOTFIELD_USED_REF; > > - else if (dpb[i].flags & > V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > > + refer_addr |= RKVDEC_FIELD_REF; > > + > > + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) > > refer_addr |= RKVDEC_BOTFIELD_USED_REF; > > - else > > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > > refer_addr |= RKVDEC_TOPFIELD_USED_REF; > > > > writel_relaxed(dpb[i].top_field_order_cnt, > > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > > index 620ee8863d74..52f3976b986c 100644 > > --- a/include/media/h264-ctrls.h > > +++ b/include/media/h264-ctrls.h > > @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params { > > #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE 0x02 > > #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM 0x04 > > #define V4L2_H264_DPB_ENTRY_FLAG_FIELD 0x08 > > -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD 0x10 > > + > > +enum v4l2_h264_dpb_reference { > > + V4L2_H264_DPB_TOP_REF = 0x1, > > + V4L2_H264_DPB_BOTTOM_REF = 0x2, > > + V4L2_H264_DPB_FRAME_REF = 0x3, > > +}; > > > > struct v4l2_h264_dpb_entry { > > __u64 reference_ts; > > @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry { > > /* Note that field is indicated by v4l2_buffer.field */ > > __s32 top_field_order_cnt; > > __s32 bottom_field_order_cnt; > > + enum v4l2_h264_dpb_reference reference; > > __u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */ > > }; > > >
On 2020-07-15 22:22, Ezequiel Garcia wrote: > As discussed recently, the current interface for the > Decoded Picture Buffer is not enough to properly > support field coding. > > This commit introduces enough semantics to support > frame and field coding, and to signal how DPB entries > are "used for reference". > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- > drivers/media/v4l2-core/v4l2-h264.c | 4 +- > drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- > include/media/h264-ctrls.h | 8 +++- > 4 files changed, 42 insertions(+), 24 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > index dd8e5a2e8986..46d4c8c6ad47 100644 > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > * - __s32 > - ``bottom_field_order_cnt`` > - > + * - enum :c:type:`v4l2_h264_dpb_reference` > + - ``reference`` > + - Specifies how the DPB entry is referenced. > * - __u32 > - ``flags`` > - See :ref:`DPB Entry Flags <h264_dpb_flags>` > > +.. c:type:: v4l2_h264_dpb_reference > + > +.. cssclass:: longtable > + > +.. flat-table:: > + :header-rows: 0 > + :stub-columns: 0 > + :widths: 1 1 2 > + > + * - ``V4L2_H264_DPB_TOP_REF`` > + - 0x1 > + - The top field in field pair is used for > + short-term reference. > + * - ``V4L2_H264_DPB_BOTTOM_REF`` > + - 0x2 > + - The bottom field in field pair is used for > + short-term reference. > + * - ``V4L2_H264_DPB_FRAME_REF`` > + - 0x3 > + - The frame (or the top/bottom fields, if it's a field pair) > + is used for short-term reference. > + > .. _h264_dpb_flags: > > ``DPB Entries Flags`` > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` > - 0x00000001 > - - The DPB entry is valid and should be considered > + - The DPB entry is valid (non-empty) and should be considered. > * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` > - 0x00000002 > - - The DPB entry is currently being used as a reference frame > + - The DPB entry is used for reference. > * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` > - 0x00000004 > - - The DPB entry is a long term reference frame > + - The DPB entry is used for long-term reference. > * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` > - 0x00000008 > - - The DPB entry is a field reference, which means only one of the field > - will be used when decoding the new frame/field. When not set the DPB > - entry is a frame reference (both fields will be used). Note that this > - flag does not say anything about the number of fields contained in the > - reference frame, it just describes the one used to decode the new > - field/frame > - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` > - - 0x00000010 > - - The DPB entry is a bottom field reference (only the bottom field of the > - reference frame is needed to decode the new frame/field). Only valid if > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but > - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the > - DPB entry is a top field reference > + - The DPB entry is a single field or a complementary field pair. > > ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` > Specifies the decoding mode to use. Currently exposes slice-based and > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c > index edf6225f0522..306a51683606 100644 > --- a/drivers/media/v4l2-core/v4l2-h264.c > +++ b/drivers/media/v4l2-core/v4l2-h264.c > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b, > else > b->refs[i].frame_num = dpb[i].frame_num; > > - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) This looks wrong, should probably use ==, dpb[i].reference == V4L2_H264_DPB_FRAME_REF else this would match any reference value. > pic_order_count = min(dpb[i].top_field_order_cnt, > dpb[i].bottom_field_order_cnt); > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > pic_order_count = dpb[i].bottom_field_order_cnt; > else > pic_order_count = dpb[i].top_field_order_cnt; > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > index 7b66e2743a4f..57539c630422 100644 > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, > RKVDEC_COLMV_USED_FLAG_REF; > > if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > - refer_addr |= RKVDEC_TOPFIELD_USED_REF | > - RKVDEC_BOTFIELD_USED_REF; > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > + refer_addr |= RKVDEC_FIELD_REF; > + > + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) > refer_addr |= RKVDEC_BOTFIELD_USED_REF; > - else > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF seems to be mixed up. I have only taken a quick look so far, I will update ffmpeg and runtime test later this weekend, will get back with result and full review on Sunday evening. Regards, Jonas > refer_addr |= RKVDEC_TOPFIELD_USED_REF; > > writel_relaxed(dpb[i].top_field_order_cnt, > diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h > index 620ee8863d74..52f3976b986c 100644 > --- a/include/media/h264-ctrls.h > +++ b/include/media/h264-ctrls.h > @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params { > #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE 0x02 > #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM 0x04 > #define V4L2_H264_DPB_ENTRY_FLAG_FIELD 0x08 > -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD 0x10 > + > +enum v4l2_h264_dpb_reference { > + V4L2_H264_DPB_TOP_REF = 0x1, > + V4L2_H264_DPB_BOTTOM_REF = 0x2, > + V4L2_H264_DPB_FRAME_REF = 0x3, > +}; > > struct v4l2_h264_dpb_entry { > __u64 reference_ts; > @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry { > /* Note that field is indicated by v4l2_buffer.field */ > __s32 top_field_order_cnt; > __s32 bottom_field_order_cnt; > + enum v4l2_h264_dpb_reference reference; > __u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */ > }; > >
Hello Jonas, On Wed, 2020-07-22 at 21:52 +0000, Jonas Karlman wrote: > On 2020-07-15 22:22, Ezequiel Garcia wrote: > > As discussed recently, the current interface for the > > Decoded Picture Buffer is not enough to properly > > support field coding. > > > > This commit introduces enough semantics to support > > frame and field coding, and to signal how DPB entries > > are "used for reference". > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- > > drivers/media/v4l2-core/v4l2-h264.c | 4 +- > > drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- > > include/media/h264-ctrls.h | 8 +++- > > 4 files changed, 42 insertions(+), 24 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > index dd8e5a2e8986..46d4c8c6ad47 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > * - __s32 > > - ``bottom_field_order_cnt`` > > - > > + * - enum :c:type:`v4l2_h264_dpb_reference` > > + - ``reference`` > > + - Specifies how the DPB entry is referenced. > > * - __u32 > > - ``flags`` > > - See :ref:`DPB Entry Flags <h264_dpb_flags>` > > > > +.. c:type:: v4l2_h264_dpb_reference > > + > > +.. cssclass:: longtable > > + > > +.. flat-table:: > > + :header-rows: 0 > > + :stub-columns: 0 > > + :widths: 1 1 2 > > + > > + * - ``V4L2_H264_DPB_TOP_REF`` > > + - 0x1 > > + - The top field in field pair is used for > > + short-term reference. > > + * - ``V4L2_H264_DPB_BOTTOM_REF`` > > + - 0x2 > > + - The bottom field in field pair is used for > > + short-term reference. > > + * - ``V4L2_H264_DPB_FRAME_REF`` > > + - 0x3 > > + - The frame (or the top/bottom fields, if it's a field pair) > > + is used for short-term reference. > > + > > .. _h264_dpb_flags: > > > > ``DPB Entries Flags`` > > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` > > - 0x00000001 > > - - The DPB entry is valid and should be considered > > + - The DPB entry is valid (non-empty) and should be considered. > > * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` > > - 0x00000002 > > - - The DPB entry is currently being used as a reference frame > > + - The DPB entry is used for reference. > > * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` > > - 0x00000004 > > - - The DPB entry is a long term reference frame > > + - The DPB entry is used for long-term reference. > > * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` > > - 0x00000008 > > - - The DPB entry is a field reference, which means only one of the field > > - will be used when decoding the new frame/field. When not set the DPB > > - entry is a frame reference (both fields will be used). Note that this > > - flag does not say anything about the number of fields contained in the > > - reference frame, it just describes the one used to decode the new > > - field/frame > > - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` > > - - 0x00000010 > > - - The DPB entry is a bottom field reference (only the bottom field of the > > - reference frame is needed to decode the new frame/field). Only valid if > > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When > > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but > > - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the > > - DPB entry is a top field reference > > + - The DPB entry is a single field or a complementary field pair. > > > > ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` > > Specifies the decoding mode to use. Currently exposes slice-based and > > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c > > index edf6225f0522..306a51683606 100644 > > --- a/drivers/media/v4l2-core/v4l2-h264.c > > +++ b/drivers/media/v4l2-core/v4l2-h264.c > > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b, > > else > > b->refs[i].frame_num = dpb[i].frame_num; > > > > - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > > + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) > > This looks wrong, should probably use ==, > > dpb[i].reference == V4L2_H264_DPB_FRAME_REF > > else this would match any reference value. > > > pic_order_count = min(dpb[i].top_field_order_cnt, > > dpb[i].bottom_field_order_cnt); > > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > > pic_order_count = dpb[i].bottom_field_order_cnt; > > else > > pic_order_count = dpb[i].top_field_order_cnt; > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > > index 7b66e2743a4f..57539c630422 100644 > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, > > RKVDEC_COLMV_USED_FLAG_REF; > > > > if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > > - refer_addr |= RKVDEC_TOPFIELD_USED_REF | > > - RKVDEC_BOTFIELD_USED_REF; > > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > > + refer_addr |= RKVDEC_FIELD_REF; > > + > > + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) > > refer_addr |= RKVDEC_BOTFIELD_USED_REF; > > - else > > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > > This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF > seems to be mixed up. > > I have only taken a quick look so far, I will update ffmpeg and runtime test > later this weekend, will get back with result and full review on Sunday evening. > Thanks that would be useful. However, keep in mind this series is specifically concerned with the uAPI review. This is not supposed to fix the field coded support, or anything else in any driver. IMO, at this stage, fixing drivers is somewhat lower priority than discussing and stabilizing the uAPI. Thanks, Ezequiel
Hi, On 2020-07-24 21:08, Ezequiel Garcia wrote: > Hello Jonas, > > On Wed, 2020-07-22 at 21:52 +0000, Jonas Karlman wrote: >> On 2020-07-15 22:22, Ezequiel Garcia wrote: >>> As discussed recently, the current interface for the >>> Decoded Picture Buffer is not enough to properly >>> support field coding. >>> >>> This commit introduces enough semantics to support >>> frame and field coding, and to signal how DPB entries >>> are "used for reference". >>> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- >>> drivers/media/v4l2-core/v4l2-h264.c | 4 +- >>> drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- >>> include/media/h264-ctrls.h | 8 +++- >>> 4 files changed, 42 insertions(+), 24 deletions(-) >>> >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>> index dd8e5a2e8986..46d4c8c6ad47 100644 >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst >>> @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - >>> * - __s32 >>> - ``bottom_field_order_cnt`` >>> - >>> + * - enum :c:type:`v4l2_h264_dpb_reference` >>> + - ``reference`` >>> + - Specifies how the DPB entry is referenced. >>> * - __u32 >>> - ``flags`` >>> - See :ref:`DPB Entry Flags <h264_dpb_flags>` >>> >>> +.. c:type:: v4l2_h264_dpb_reference >>> + >>> +.. cssclass:: longtable >>> + >>> +.. flat-table:: >>> + :header-rows: 0 >>> + :stub-columns: 0 >>> + :widths: 1 1 2 >>> + >>> + * - ``V4L2_H264_DPB_TOP_REF`` >>> + - 0x1 >>> + - The top field in field pair is used for >>> + short-term reference. >>> + * - ``V4L2_H264_DPB_BOTTOM_REF`` >>> + - 0x2 >>> + - The bottom field in field pair is used for >>> + short-term reference. >>> + * - ``V4L2_H264_DPB_FRAME_REF`` >>> + - 0x3 >>> + - The frame (or the top/bottom fields, if it's a field pair) >>> + is used for short-term reference. >>> + >>> .. _h264_dpb_flags: >>> >>> ``DPB Entries Flags`` >>> @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - >>> >>> * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` >>> - 0x00000001 >>> - - The DPB entry is valid and should be considered >>> + - The DPB entry is valid (non-empty) and should be considered. >>> * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` >>> - 0x00000002 >>> - - The DPB entry is currently being used as a reference frame >>> + - The DPB entry is used for reference. >>> * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` >>> - 0x00000004 >>> - - The DPB entry is a long term reference frame >>> + - The DPB entry is used for long-term reference. >>> * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` >>> - 0x00000008 >>> - - The DPB entry is a field reference, which means only one of the field >>> - will be used when decoding the new frame/field. When not set the DPB >>> - entry is a frame reference (both fields will be used). Note that this >>> - flag does not say anything about the number of fields contained in the >>> - reference frame, it just describes the one used to decode the new >>> - field/frame >>> - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` >>> - - 0x00000010 >>> - - The DPB entry is a bottom field reference (only the bottom field of the >>> - reference frame is needed to decode the new frame/field). Only valid if >>> - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When >>> - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but >>> - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the >>> - DPB entry is a top field reference >>> + - The DPB entry is a single field or a complementary field pair. >>> >>> ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` >>> Specifies the decoding mode to use. Currently exposes slice-based and >>> diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c >>> index edf6225f0522..306a51683606 100644 >>> --- a/drivers/media/v4l2-core/v4l2-h264.c >>> +++ b/drivers/media/v4l2-core/v4l2-h264.c >>> @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b, >>> else >>> b->refs[i].frame_num = dpb[i].frame_num; >>> >>> - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) >>> + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) >> >> This looks wrong, should probably use ==, >> >> dpb[i].reference == V4L2_H264_DPB_FRAME_REF >> >> else this would match any reference value. >> >>> pic_order_count = min(dpb[i].top_field_order_cnt, >>> dpb[i].bottom_field_order_cnt); >>> - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) >>> + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) >>> pic_order_count = dpb[i].bottom_field_order_cnt; >>> else >>> pic_order_count = dpb[i].top_field_order_cnt; >>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c >>> index 7b66e2743a4f..57539c630422 100644 >>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c >>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c >>> @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, >>> RKVDEC_COLMV_USED_FLAG_REF; >>> >>> if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) >>> - refer_addr |= RKVDEC_TOPFIELD_USED_REF | >>> - RKVDEC_BOTFIELD_USED_REF; >>> - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) >>> + refer_addr |= RKVDEC_FIELD_REF; >>> + >>> + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) >>> refer_addr |= RKVDEC_BOTFIELD_USED_REF; >>> - else >>> + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) >> >> This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF >> seems to be mixed up. >> >> I have only taken a quick look so far, I will update ffmpeg and runtime test >> later this weekend, will get back with result and full review on Sunday evening. >> > > Thanks that would be useful. > > However, keep in mind this series is specifically concerned > with the uAPI review. > > This is not supposed to fix the field coded support, or anything > else in any driver. > > IMO, at this stage, fixing drivers is somewhat lower priority > than discussing and stabilizing the uAPI. I have now tested rkvdec on a RK3328 device and needed to do 3 fixups, see [1]. Initial ffmpeg update using update h264 uapi is located at [2], the ffmpeg update still needs to be tested with cedrus and hantro. So far I have not seen any issue with the uapi changes. Q: ffmpeg will not try to set SLICE_PARAMS or PRED_WEIGHT ctrls for DECODE_MODE_SLICE_BASED, should userspace check if ctrl exists or is using DECODE_MODE value okay? I have also pushed an updated WIP branch at [3] containing high 10, field encoded and hevc work. [1] https://github.com/Kwiboo/linux-rockchip/compare/b6b91f27c0cb33520e954e7bb2550e0e07ed4d85...b82b6e93feb9ca44d2c677f25416cf6345f0114d [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3.1 [3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-work-in-progress Best regards, Jonas > > Thanks, > Ezequiel > >
Hi Jonas, On Mon, 2020-07-27 at 23:39 +0000, Jonas Karlman wrote: > Hi, > > On 2020-07-24 21:08, Ezequiel Garcia wrote: > > Hello Jonas, > > > > On Wed, 2020-07-22 at 21:52 +0000, Jonas Karlman wrote: > > > On 2020-07-15 22:22, Ezequiel Garcia wrote: > > > > As discussed recently, the current interface for the > > > > Decoded Picture Buffer is not enough to properly > > > > support field coding. > > > > > > > > This commit introduces enough semantics to support > > > > frame and field coding, and to signal how DPB entries > > > > are "used for reference". > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > --- > > > > .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- > > > > drivers/media/v4l2-core/v4l2-h264.c | 4 +- > > > > drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- > > > > include/media/h264-ctrls.h | 8 +++- > > > > 4 files changed, 42 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > index dd8e5a2e8986..46d4c8c6ad47 100644 > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst > > > > @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > * - __s32 > > > > - ``bottom_field_order_cnt`` > > > > - > > > > + * - enum :c:type:`v4l2_h264_dpb_reference` > > > > + - ``reference`` > > > > + - Specifies how the DPB entry is referenced. > > > > * - __u32 > > > > - ``flags`` > > > > - See :ref:`DPB Entry Flags <h264_dpb_flags>` > > > > > > > > +.. c:type:: v4l2_h264_dpb_reference > > > > + > > > > +.. cssclass:: longtable > > > > + > > > > +.. flat-table:: > > > > + :header-rows: 0 > > > > + :stub-columns: 0 > > > > + :widths: 1 1 2 > > > > + > > > > + * - ``V4L2_H264_DPB_TOP_REF`` > > > > + - 0x1 > > > > + - The top field in field pair is used for > > > > + short-term reference. > > > > + * - ``V4L2_H264_DPB_BOTTOM_REF`` > > > > + - 0x2 > > > > + - The bottom field in field pair is used for > > > > + short-term reference. > > > > + * - ``V4L2_H264_DPB_FRAME_REF`` > > > > + - 0x3 > > > > + - The frame (or the top/bottom fields, if it's a field pair) > > > > + is used for short-term reference. > > > > + > > > > .. _h264_dpb_flags: > > > > > > > > ``DPB Entries Flags`` > > > > @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - > > > > > > > > * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` > > > > - 0x00000001 > > > > - - The DPB entry is valid and should be considered > > > > + - The DPB entry is valid (non-empty) and should be considered. > > > > * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` > > > > - 0x00000002 > > > > - - The DPB entry is currently being used as a reference frame > > > > + - The DPB entry is used for reference. > > > > * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` > > > > - 0x00000004 > > > > - - The DPB entry is a long term reference frame > > > > + - The DPB entry is used for long-term reference. > > > > * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` > > > > - 0x00000008 > > > > - - The DPB entry is a field reference, which means only one of the field > > > > - will be used when decoding the new frame/field. When not set the DPB > > > > - entry is a frame reference (both fields will be used). Note that this > > > > - flag does not say anything about the number of fields contained in the > > > > - reference frame, it just describes the one used to decode the new > > > > - field/frame > > > > - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` > > > > - - 0x00000010 > > > > - - The DPB entry is a bottom field reference (only the bottom field of the > > > > - reference frame is needed to decode the new frame/field). Only valid if > > > > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When > > > > - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but > > > > - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the > > > > - DPB entry is a top field reference > > > > + - The DPB entry is a single field or a complementary field pair. > > > > > > > > ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` > > > > Specifies the decoding mode to use. Currently exposes slice-based and > > > > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c > > > > index edf6225f0522..306a51683606 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-h264.c > > > > +++ b/drivers/media/v4l2-core/v4l2-h264.c > > > > @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b, > > > > else > > > > b->refs[i].frame_num = dpb[i].frame_num; > > > > > > > > - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > > > > + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) > > > > > > This looks wrong, should probably use ==, > > > > > > dpb[i].reference == V4L2_H264_DPB_FRAME_REF > > > > > > else this would match any reference value. > > > > > > > pic_order_count = min(dpb[i].top_field_order_cnt, > > > > dpb[i].bottom_field_order_cnt); > > > > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > > > > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > > > > pic_order_count = dpb[i].bottom_field_order_cnt; > > > > else > > > > pic_order_count = dpb[i].top_field_order_cnt; > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c > > > > index 7b66e2743a4f..57539c630422 100644 > > > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c > > > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c > > > > @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, > > > > RKVDEC_COLMV_USED_FLAG_REF; > > > > > > > > if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) > > > > - refer_addr |= RKVDEC_TOPFIELD_USED_REF | > > > > - RKVDEC_BOTFIELD_USED_REF; > > > > - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) > > > > + refer_addr |= RKVDEC_FIELD_REF; > > > > + > > > > + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) > > > > refer_addr |= RKVDEC_BOTFIELD_USED_REF; > > > > - else > > > > + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) > > > > > > This should probably be if and not else if, and BOTFIELD/TOPFIELD_USED_REF > > > seems to be mixed up. > > > > > > I have only taken a quick look so far, I will update ffmpeg and runtime test > > > later this weekend, will get back with result and full review on Sunday evening. > > > > > > > Thanks that would be useful. > > > > However, keep in mind this series is specifically concerned > > with the uAPI review. > > > > This is not supposed to fix the field coded support, or anything > > else in any driver. > > > > IMO, at this stage, fixing drivers is somewhat lower priority > > than discussing and stabilizing the uAPI. > > I have now tested rkvdec on a RK3328 device and needed to do 3 fixups, see [1]. > Initial ffmpeg update using update h264 uapi is located at [2], the ffmpeg > update still needs to be tested with cedrus and hantro. > > So far I have not seen any issue with the uapi changes. > Great, thanks for the test. > Q: ffmpeg will not try to set SLICE_PARAMS or PRED_WEIGHT ctrls for > DECODE_MODE_SLICE_BASED, You mean it will not try to set those controls for DECODE_MODE_FRAME_BASED? I think that's correct, as we've discussed multiple times, frame-based drivers shouldn't need to use those controls, by definition. > should userspace check if ctrl exists or is using > DECODE_MODE value okay? > If a driver supporting DECODE_MODE_SLICE_BASED doesn't support SLICE_PARAMS, that can probably be considered a failure from the application side. If it doesn't support PRED_WEIGHTS controls, then strictly speaking it won't be able to decode slices that have a prediction weight table, i.e.: ((weighted_pred_flag && (slice_type == P || slice_type == SP)) || \ (weighted_bipred_idc == 1 && slice_type == B)) I doubt we'll ever counter such case, so probably if the controls aren't supported, applications can just safely fail (and e.g. fallback to software). > I have also pushed an updated WIP branch at [3] containing high 10, > field encoded and hevc work. > > [1] https://github.com/Kwiboo/linux-rockchip/compare/b6b91f27c0cb33520e954e7bb2550e0e07ed4d85...b82b6e93feb9ca44d2c677f25416cf6345f0114d > [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3.1 > [3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-work-in-progress > > Best regards, > Jonas > > > Thanks, > > Ezequiel > > > >
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst index dd8e5a2e8986..46d4c8c6ad47 100644 --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst @@ -2058,10 +2058,35 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - * - __s32 - ``bottom_field_order_cnt`` - + * - enum :c:type:`v4l2_h264_dpb_reference` + - ``reference`` + - Specifies how the DPB entry is referenced. * - __u32 - ``flags`` - See :ref:`DPB Entry Flags <h264_dpb_flags>` +.. c:type:: v4l2_h264_dpb_reference + +.. cssclass:: longtable + +.. flat-table:: + :header-rows: 0 + :stub-columns: 0 + :widths: 1 1 2 + + * - ``V4L2_H264_DPB_TOP_REF`` + - 0x1 + - The top field in field pair is used for + short-term reference. + * - ``V4L2_H264_DPB_BOTTOM_REF`` + - 0x2 + - The bottom field in field pair is used for + short-term reference. + * - ``V4L2_H264_DPB_FRAME_REF`` + - 0x3 + - The frame (or the top/bottom fields, if it's a field pair) + is used for short-term reference. + .. _h264_dpb_flags: ``DPB Entries Flags`` @@ -2075,29 +2100,16 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type - * - ``V4L2_H264_DPB_ENTRY_FLAG_VALID`` - 0x00000001 - - The DPB entry is valid and should be considered + - The DPB entry is valid (non-empty) and should be considered. * - ``V4L2_H264_DPB_ENTRY_FLAG_ACTIVE`` - 0x00000002 - - The DPB entry is currently being used as a reference frame + - The DPB entry is used for reference. * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM`` - 0x00000004 - - The DPB entry is a long term reference frame + - The DPB entry is used for long-term reference. * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD`` - 0x00000008 - - The DPB entry is a field reference, which means only one of the field - will be used when decoding the new frame/field. When not set the DPB - entry is a frame reference (both fields will be used). Note that this - flag does not say anything about the number of fields contained in the - reference frame, it just describes the one used to decode the new - field/frame - * - ``V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD`` - - 0x00000010 - - The DPB entry is a bottom field reference (only the bottom field of the - reference frame is needed to decode the new frame/field). Only valid if - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set. When - V4L2_H264_DPB_ENTRY_FLAG_FIELD is set but - V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD is not, that means the - DPB entry is a top field reference + - The DPB entry is a single field or a complementary field pair. ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)`` Specifies the decoding mode to use. Currently exposes slice-based and diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-core/v4l2-h264.c index edf6225f0522..306a51683606 100644 --- a/drivers/media/v4l2-core/v4l2-h264.c +++ b/drivers/media/v4l2-core/v4l2-h264.c @@ -66,10 +66,10 @@ v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b, else b->refs[i].frame_num = dpb[i].frame_num; - if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) + if (dpb[i].reference & V4L2_H264_DPB_FRAME_REF) pic_order_count = min(dpb[i].top_field_order_cnt, dpb[i].bottom_field_order_cnt); - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) pic_order_count = dpb[i].bottom_field_order_cnt; else pic_order_count = dpb[i].top_field_order_cnt; diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c index 7b66e2743a4f..57539c630422 100644 --- a/drivers/staging/media/rkvdec/rkvdec-h264.c +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c @@ -953,11 +953,11 @@ static void config_registers(struct rkvdec_ctx *ctx, RKVDEC_COLMV_USED_FLAG_REF; if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_FIELD)) - refer_addr |= RKVDEC_TOPFIELD_USED_REF | - RKVDEC_BOTFIELD_USED_REF; - else if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD) + refer_addr |= RKVDEC_FIELD_REF; + + if (dpb[i].reference & V4L2_H264_DPB_TOP_REF) refer_addr |= RKVDEC_BOTFIELD_USED_REF; - else + else if (dpb[i].reference & V4L2_H264_DPB_BOTTOM_REF) refer_addr |= RKVDEC_TOPFIELD_USED_REF; writel_relaxed(dpb[i].top_field_order_cnt, diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h index 620ee8863d74..52f3976b986c 100644 --- a/include/media/h264-ctrls.h +++ b/include/media/h264-ctrls.h @@ -202,7 +202,12 @@ struct v4l2_ctrl_h264_slice_params { #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE 0x02 #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM 0x04 #define V4L2_H264_DPB_ENTRY_FLAG_FIELD 0x08 -#define V4L2_H264_DPB_ENTRY_FLAG_BOTTOM_FIELD 0x10 + +enum v4l2_h264_dpb_reference { + V4L2_H264_DPB_TOP_REF = 0x1, + V4L2_H264_DPB_BOTTOM_REF = 0x2, + V4L2_H264_DPB_FRAME_REF = 0x3, +}; struct v4l2_h264_dpb_entry { __u64 reference_ts; @@ -211,6 +216,7 @@ struct v4l2_h264_dpb_entry { /* Note that field is indicated by v4l2_buffer.field */ __s32 top_field_order_cnt; __s32 bottom_field_order_cnt; + enum v4l2_h264_dpb_reference reference; __u32 flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */ };
As discussed recently, the current interface for the Decoded Picture Buffer is not enough to properly support field coding. This commit introduces enough semantics to support frame and field coding, and to signal how DPB entries are "used for reference". Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- .../media/v4l/ext-ctrls-codec.rst | 46 ++++++++++++------- drivers/media/v4l2-core/v4l2-h264.c | 4 +- drivers/staging/media/rkvdec/rkvdec-h264.c | 8 ++-- include/media/h264-ctrls.h | 8 +++- 4 files changed, 42 insertions(+), 24 deletions(-)