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
> > >
