Re: [PATCH v4 1/2] media: hevc: Remove RPS named flags

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 05/01/2022 à 13:26, Ezequiel Garcia a écrit :
> Hi Benjamin,
>
> On Tue, Jan 04, 2022 at 08:38:41AM +0100, Benjamin Gaignard wrote:
>> 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.
>>
> A change like this, which is affecting a kernel interface and has impact
> on userspace and drivers, requires a lot more attention in the commit description.
>
> If I understand correctly, this change is related to how HEVC was first
> introduced and how the DPB was originally represented in V4L2.
>
> Originally, a DPB entry struct v4l2_hevc_dpb_entry had an rps field
> which can be:
>
>    V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE
>    V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER
>    V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR
>
> Perhaps this idea followed libva, where a VAPictureHEVC has flags field
> which can be:
>
>    VA_PICTURE_HEVC_RPS_ST_CURR_BEFORE,
>    VA_PICTURE_HEVC_RPS_ST_CURR_AFTER,
>    VA_PICTURE_HEVC_RPS_LT_CURR,
>    VA_PICTURE_HEVC_LONG_TERM_REFERENCE
>
> In this representation, the sets PocStCurrBefore, PocStCurrAfter,
> PocLtCurr are implicit, and must be built by the kernel from the DPB
> entries struct v4l2_hevc_dpb_entry, using the information in the rps field.
>
> Last year, we changed this with your commit:
>
> commit d395a78db9eabd12633b39e05c80e803543b6590
> Author: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Date:   Thu Jun 3 13:49:57 2021 +0200
>
>      media: hevc: Add decode params control
>
> Which added the control v4l2_ctrl_hevc_decode_params, which includes
> the sets PocStCurrBefore, PocStCurrAfter, PocLtCurr explicitly and therefore
> moved the responsability of creating and maintaining the sets to userspace.
>
> This effectively made the rps field in the struct v4l2_hevc_dpb_entry
> useless. The longterm flag is still needed though for a DPB entry.
>
> With this in mind, you could even say this commit is doing actually two
> things:
>
> 1. Removes the unused rps field.
> 2. Adds a flag field, for the longterm DPB entry boolean.
>
> Do you think a single byte of flags will be OK for a DPB entry?
> I think so, but I'd love yours/others input here.

In ITU HEVC spec it is said that:
"When a picture is referred to as being marked as "used for reference",
this collectively refers to the picture being marked as "used
  for
short-term reference" or "used for long-term reference" (but not both)."

So I believe that a single if enough for DPB entry.

>
> If the above is more or less accurate then, and provided you
> submit a new version with a more detailed commit description:

I will follow your suggestions and improve it in v5

>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>
> Also, a small question:
>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>> version 4:
>> - check flags with & and not ==
>>
>>   Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++-----
>>   drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
>>   drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
>>   include/media/hevc-ctrls.h                                | 6 ++----
>>   4 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> index e141f0e4eec9..38da33e61c3d 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>>   	:c:func:`v4l2_timeval_to_ns()` function to convert the struct
>>   	: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)
>> +      - ``flags``
>> +      - Long term flag for the reference frame
>> +        (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
> Is this longterm flag associated in any way to a syntax element
> or an HEVC process? If so, please document that.

done in v5.

Thanks for your review.

Benjamin

>
> Thanks,
> Ezequiel