Hi Daniel,
Few more style related comments.
1. "tab" seems to be used before "descr = ". […] for other cases.
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1441,6 +1441,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_PIX_FMT_MT21C: descr = "Mediatek
Compressed Format"; break;
+ case V4L2_PIX_FMT_AV1_FRAME: descr = "AV1 Frame"; break;
2. nit: s/the AOMedia/the AOMedia/
This patch adds the AOMedia Video 1 (AV1) kernel uAPI.
3. nit: s/AV1 film_gram/AV1 film grain/ ?
+++ b/include/media/v4l2-ctrls.h
+ * @p_av1_film_grain: Pointer to an AV1 film_grain.
I found these issues while reviewing Chen-Yu's change, which backports
this change to 5.10 kernel.
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3650151
Steve
On Wed, May 18, 2022 at 4:26 PM Steve Cho <stevecho@xxxxxxxxxx> wrote:
>
> Hi Daniel,
>
> Don't we also need V4L2_PIX_FMT_AV1 in addition to V4L2_PIX_FMT_AV1_FRAME
> as we do with both VP8 and VP9? I see V4L2_PIX_FMT_AV1 is missing.
>
> I think V4L2_PIX_FMT_VP9 is for compressed video frame
> and V4L2_PIX_FMT_VP9_FRAME is for parsed video frame.
>
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/pixfmt-compressed.html
>
> Thanks,
> Steve
>
> On Wed, May 18, 2022 at 2:42 PM Steve Cho <stevecho@xxxxxxxxxxxx> wrote:
>>
>> Optional: one comment about "v4l2_ctrl_av1_frame_header".
>>
>> struct v4l2_ctrl_av1_frame_header {
>> struct v4l2_av1_tile_info tile_info;
>> struct v4l2_av1_quantization quantization;
>> struct v4l2_av1_segmentation segmentation;
>> struct v4l2_av1_loop_filter loop_filter;
>> struct v4l2_av1_cdef cdef;
>> struct v4l2_av1_loop_restoration loop_restoration;
>> struct v4l2_av1_global_motion global_motion;
>>
>> We used "v4l2_ctrl_vp9_frame" for the similar purpose.
>>
>> I thought "_header" can be confusing in a sense that these are
>> parameters setup from parsing av1 frame header,
>> not necessarily "header" itself.
>>
>> How about making it "v4l2_ctrl_av1_frame" similar to vp9,
>> or "v4l2_ctrl_av1_frame_params"?
>>
>> I don't think this alone justify for another update, but if we need to
>> have an update anyway,
>> then I thought it was worthwhile considering.
>>
>> Steve
>>
>> On Mon, May 16, 2022 at 10:42 AM Steve Cho <stevecho@xxxxxxxxxxxx> wrote:
>> >
>> > Hi Daniel,
>> >
>> > Question about tile info structure.
>> >
>> > struct v4l2_av1_tile_info {
>> > __u8 flags;
>> > __u32 mi_col_starts[V4L2_AV1_MAX_TILE_COLS + 1];
>> > __u32 mi_row_starts[V4L2_AV1_MAX_TILE_ROWS + 1];
>> > __u32 width_in_sbs_minus_1[V4L2_AV1_MAX_TILE_COLS];
>> > __u32 height_in_sbs_minus_1[V4L2_AV1_MAX_TILE_ROWS];
>> >
>> > I see below from the spec and gstreamer implementation
>> > for width_in_sbs_minus_1 and height_in_sbs_minus_1 computation.
>> >
>> > sb_cols = seq_header->use_128x128_superblock ?
>> > ((parser->state.mi_cols + 31) >> 5) : ((parser->state.mi_cols + 15) >> 4);
>> > sb_rows = seq_header->use_128x128_superblock ? ((parser->state.mi_rows +
>> > 31) >> 5) : ((parser->state.mi_rows + 15) >> 4);
>> >
>> > Are we confident that V4L2_AV1_MAX_TILE_COLS is good enough size for
>> > width_in_sbs_minus_1?
>> > Or does it potentially need to be V4L2_AV1_MAX_TILE_COLS+1?
>> >
>> > I am asking to double check because I see V4L2_AV1_MAX_TILE_COLS+1
>> > used for corresponding field in libgav1.
>> > int tile_column_width_in_superblocks[kMaxTileColumns + 1];
>> >
>> > https://source.chromium.org/chromium/chromium/src/+/main:third_party/libgav1/src/src/utils/types.h;l=291
>> >
>> > Steve
>> >
>> > On Wed, May 11, 2022 at 1:59 PM Steve Cho <stevecho@xxxxxxxxxxxx> wrote:
>> > >
>> > > Hi Daniel,
>> > >
>> > > I think the below definition is expected to cause a build error.
>> > >
>> > > +struct v4l2_av1_loop_restoration {
>> > > + u8 flags;
>> > >
>> > > s/u8/__u8/ is needed.
>> > >
>> > > At least, this change was needed to fix this build error on Chromium
>> > > build environment.
>> > >
>> > > Steve
>> > >
>> > > On Tue, May 10, 2022 at 9:30 AM Daniel Almeida
>> > > <daniel.almeida@xxxxxxxxxxxxx> wrote:
>> > > >
>> > > > Hi Steve,
>> > > >
>> > > > > Hi Daniel,
>> > > > >
>> > > > > Found a minor typo.
>> > > > >
>> > > > >> See enum_v4l2_av1_frame_restoration_type.
>> > > > > Assume you meant v4l2_av1_frame_restoration_type instead here.
>> > > >
>> > > >
>> > > > Thanks for the heads up, this will be fixed in v3.
>> > > >
>> > > > -- Daniel
>> > > >
