Re: [PATCH v7 00/11] VP9 codec V4L2 control interface

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 16/11/2021 14:14, Andrzej Pietrasiewicz wrote:
> Hi,
> 
> W dniu 16.11.2021 o 09:21, Hans Verkuil pisze:
>> On 16/11/2021 09:09, Andrzej Pietrasiewicz wrote:
>>> Hi Hans,
>>>
>>> W dniu 15.11.2021 o 22:16, Hans Verkuil pisze:
>>>> On 15/11/2021 18:14, Andrzej Pietrasiewicz wrote:
>>>>> Hi Hans,
>>>>>
>>>>> W dniu 15.11.2021 o 16:07, Hans Verkuil pisze:
>>>>>> Andrzej,
>>>>>>
>>>>>> Can you rebase this series on top of the master branch of
>>>>>> https://git.linuxtv.org/media_stage.git/ ? Unfortunately this v7 no longer
>>>>>> applies. Specifically "rkvdec: Add the VP9 backend" failed in a non-trivial
>>>>>> manner.
>>>>>
>>>>> This is a branch for you:
>>>>>
>>>>> https://gitlab.collabora.com/linux/for-upstream/-/tree/vp9-uapi
>>>>
>>>> I'm getting a bunch of sparse/smatch warnings:
>>>>
>>>
>>> Thanks for finding this, I will re-create the branch and let you know on irc.
>>> Some of the below are "false positives, namely:
>>>
>>> drivers/media/platform/omap3isp/omap3isp.h
>>> drivers/media/platform/qcom/venus/core.h
>>
>> Ah, sorry, I though I had filtered those out. Obviously you can ignore those.
>>
>> Please post a v8. That way the series is archived on lore. And it works better
>> with patchwork.
> 
> Sure, no problem. Also please see below.
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> which are not touched by the series.
>>>
>>> Regards,
>>>
>>> Andrzej
>>>
>>>> sparse:
>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> SPARSE:hantro/hantro_postproc.c hantro/hantro_postproc.c:37:35: warning: symbol 'hantro_g1_postproc_regs' was not declared. Should it be static?
>>>>
>>>> smatch:
>>>> rkvdec/rkvdec-vp9.c:190:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> rkvdec/rkvdec-vp9.c:245:43: warning: variable 'dec_params' set but not used [-Wunused-but-set-variable]
>>>> rkvdec/rkvdec-vp9.c: rkvdec/rkvdec-vp9.c:236 init_intra_only_probs() error: buffer overflow 'ptr' 90 <= 91
> 
> this looks a false positive.
> 
> A portion of memory pointed to by ptr is indexed with i * 23 + m,
> where i ranges from 0 to 3, inclusive, and m ranges from 0 to 22,
> inclusive if i < 3, otherwise m ranges from 0 to 20, inclusive.
> So the largest index value we compute equals 89 (3 * 23 + 20).
> Because ptr points to something that is at least 90 bytes large,
> 89 is a valid index and no greater index will be ever computed.

But we do need to get rid of this smatch warning, otherwise it will pollute the
list of smatch warnings.

I was looking at the code and wonder if it wouldn't make more sense to
move writing to rkprobs->intra_mode[i].uv_mode[] into a separate for loop:

        for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++)
                rkprobs->intra_mode[i / 23].uv_mode[i % 23] = v4l2_vp9_kf_uv_mode_prob[i];

Wouldn't that do the same as the current code? It looks simpler as well.

Regards,

	Hans