Messages in this thread | | | Subject | Re: [PATCH v7 00/11] VP9 codec V4L2 control interface | From | Andrzej Pietrasiewicz <> | Date | Wed, 17 Nov 2021 11:51:09 +0100 |
| |
Hi again,
W dniu 17.11.2021 o 11:49, Andrzej Pietrasiewicz pisze: > Hi, > > W dniu 17.11.2021 o 10:59, Hans Verkuil pisze: >> 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. >> > > I think it would, but I would slightly change the loop: > > for (i = 0; i < ARRAY_SIZE(v4l2_vp9_kf_uv_mode_prob); i++) {
actually, sizeof(v4l2_vp9_kf_uv_mode_prob)
> const u8 *ptr = (const u8 *)v4l2_vp9_kf_uv_mode_prob; > > rkprobs->intra_mode[i / 23].uv_mode[i % 23] = ptr[i]; > } > > because v4l2_vp9_kf_uv_mode_prob is actually a u8[10][9]. > > I will make such a change locally and test whether it causes regressions. > > Once I confirm it works (and I expect I will) would you like me to post a v9, > only reply to the changed patch with its updated version or do you want to make > this change yourself? > > Andrzej
| |