On 2020-07-10 10:13, Boris Brezillon wrote:
> On Fri, 10 Jul 2020 01:21:07 -0300
> Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
>
>> Hello Jonas,
>>
>> In the context of the uAPI cleanup,
>> I'm revisiting this patch.
>>
>> On Sun, 2019-09-01 at 12:45 +0000, Jonas Karlman wrote:
>>> Add DPB entry flags to help indicate when a reference frame is a field picture
>>> and how the DPB entry is referenced, top or bottom field or full frame.
>>>
>>> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx>
>>> ---
>>> Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 12 ++++++++++++
>>> include/media/h264-ctrls.h | 4 ++++
>>> 2 files changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> index bc5dd8e76567..eb6c32668ad7 100644
>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
>>> @@ -2022,6 +2022,18 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
>>> * - ``V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM``
>>> - 0x00000004
>>> - The DPB entry is a long term reference frame
>>> + * - ``V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE``
>>> + - 0x00000008
>>> + - The DPB entry is a field picture
>>> + * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_TOP``
>>> + - 0x00000010
>>> + - The DPB entry is a top field reference
>>> + * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM``
>>> + - 0x00000020
>>> + - The DPB entry is a bottom field reference
>>> + * - ``V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME``
>>> + - 0x00000030
>>> + - The DPB entry is a reference frame
>>>
>>> ``V4L2_CID_MPEG_VIDEO_H264_DECODE_MODE (enum)``
>>> Specifies the decoding mode to use. Currently exposes slice-based and
>>> diff --git a/include/media/h264-ctrls.h b/include/media/h264-ctrls.h
>>> index e877bf1d537c..76020ebd1e6c 100644
>>> --- a/include/media/h264-ctrls.h
>>> +++ b/include/media/h264-ctrls.h
>>> @@ -185,6 +185,10 @@ struct v4l2_ctrl_h264_slice_params {
>>> #define V4L2_H264_DPB_ENTRY_FLAG_VALID 0x01
>>> #define V4L2_H264_DPB_ENTRY_FLAG_ACTIVE 0x02
>>> #define V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM 0x04
>>> +#define V4L2_H264_DPB_ENTRY_FLAG_FIELD_PICTURE 0x08
>>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_TOP 0x10
>>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_BOTTOM 0x20
>>> +#define V4L2_H264_DPB_ENTRY_FLAG_REF_FRAME 0x30
>>>
>>
>> I've been going thru the H264 spec and I'm unsure,
>> are all these flags semantically needed?
>>
>> For instance, if one of REF_BOTTOM or REF_TOP (or both)
>> are set, doesn't that indicate it's a field picture?
These flags would only indicate how the frame / field pair / field is
referenced and not if the DPB entry was decoded as a frame or field pair.
Both hantro and rkvdec needs to know how the referenced frame / field pair
was decoded (not how it is referenced), my best guess is that MV is stored
differently for a frame (linear) and field pair (buffer split in two).
I think we should be able to track how the buffer was decoded similar to
how VP9 keep track of buffer width/height.
When I played with interlaced decoding of rkvdec a few weeks ago I
reverted flags to something similar as my initial rfc patch, see [1].
I guess it should be possible to keep current flags and track field_pic
in driver, some macro to simplify check for top/bottom ref could be
useful if flags is kept as-is.
I am hoping to find some time next week to revisit hantro interlaced
and refine rkvdec interlaced support.
[1] https://github.com/Kwiboo/linux-rockchip/compare/da52ca6f8d2284aebea2d0b99d254b64922faa2d...c9f04cd9bc65eda0da713f4ce1c77eeb1960bd70
Regards,
Jonas
>>
>> Or conversely, if neither REF_BOTTOM or REF_TOP are set,
>> then it's a frame picture?
>
> I think that's what I was trying to do here [1]
>
> [1]https://patchwork.kernel.org/patch/11392095/
>
