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@xxxxxxxxxxxxx> > > > > --- > > > > .../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 > > > >
