Re: [RFC PATCH v3] media: mediatek: vcodec: support stateless AV1 decoder
Dear Daniel,
Thanks for your good suggestion!
I have updated v4 to fix
your comment.
Changes from v3:
- modify comment for struct vdec_av1_slice_slot
- add define SEG_LVL_ALT_Q
- change use_lr/use_chroma_lr parse from av1 spec
- use ARRAY_SIZE to replace size for loop_filter_level and
loop_filter_mode_deltas
- change array size of loop_filter_mode_deltas from 4 to 2
- add define SECONDARY_FILTER_STRENGTH_NUM_BITS
- change some hex values from upper case to lower case
- change *dpb_sz equal to V4L2_AV1_TOTAL_REFS_PER_FRAME + 1
- convert vb2_find_timestamp to vb2_find_buffer
- test by av1 fluster, result is 173/239
detail in link:
https://patchwork.kernel.org/project/linux-mediatek/patch/20220930033000.22579-1-xiaoyong...@mediatek.com/
thanks !
Xiaoyong Lu
On Thu, 2022-09-22 at 13:36 -0300, Daniel Almeida wrote:
> Hi Xiaoyong.
>
> Comments below (other code removed for brevity)
>
> +/**
> + * struct vdec_av1_slice_slot - slot info need save in global
> instance
> + * @frame_info: frame info for each slot
> + * @timestamp: time stamp info
> + */
> +struct vdec_av1_slice_slot {
> + struct vdec_av1_slice_frame_info
> frame_info[AV1_MAX_FRAME_BUF_COUNT];
> + u64 timestamp[AV1_MAX_FRAME_BUF_COUNT];
> +};
>
> nit: slot info that needs to be saved in the global instance
>
> +static int vdec_av1_slice_get_qindex(struct
> vdec_av1_slice_uncompressed_header *uh,
> + int segmentation_id)
> +{
> + struct vdec_av1_slice_seg *seg = >seg;
> + struct vdec_av1_slice_quantization *quant = >quant;
> + int data = 0, qindex = 0;
> +
> + if (seg->segmentation_enabled &&
> + (seg->feature_enabled_mask[segmentation_id] & BIT(0))) {
> + data = seg->feature_data[segmentation_id][0];
>
>
> Maybe you should replace the 0 above by SEG_LVL_ALT_Q to be more
> explicit. Same goes for BIT(0).
>
> +static void vdec_av1_slice_setup_lr(struct vdec_av1_slice_lr *lr,
> + struct
> v4l2_av1_loop_restoration *ctrl_lr)
> +{
> + int i;
> +
> + for (i = 0; i < V4L2_AV1_NUM_PLANES_MAX; i++) {
> + lr->frame_restoration_type[i] = ctrl_lr-
> >frame_restoration_type[i];
> + lr->loop_restoration_size[i] = ctrl_lr-
> >loop_restoration_size[i];
> + }
> + lr->use_lr = !!lr->frame_restoration_type[0];
> + lr->use_chroma_lr = !!lr->frame_restoration_type[1];
> +}
>
> From a first glance, this looks a bit divergent from the spec?
>
> for ( i = 0; i < NumPlanes; i++ ) {
> lr_type
> FrameRestorationType[i] = Remap_Lr_Type[lr_type]
> if ( FrameRestorationType[i] != RESTORE_NONE ) {
> UsesLr = 1
> if ( i > 0 ) {
> usesChromaLr = 1
> }
> }
> }
>
> I will include these two variables in the next iteration of the uapi
> if
> computing them in the driver is problematic.
>
> +static void vdec_av1_slice_setup_lf(struct
> vdec_av1_slice_loop_filter *lf,
> + struct v4l2_av1_loop_filter
> *ctrl_lf)
> +{
> + int i;
> +
> + for (i = 0; i < 4; i++)
> + lf->loop_filter_level[i] = ctrl_lf->level[i];
> +
> + for (i = 0; i < V4L2_AV1_TOTAL_REFS_PER_FRAME; i++)
> + lf->loop_filter_ref_deltas[i] = ctrl_lf->ref_deltas[i];
> +
> + for (i = 0; i < 2; i++)
> + lf->loop_filter_mode_deltas[i] = ctrl_lf-
> >mode_deltas[i];
> +
> + lf->loop_filter_sharpness = ctrl_lf->sharpness;
> + lf->loop_filter_delta_enabled =
> +BIT_FLAG(ctrl_lf,
> V4L2_AV1_LOOP_FILTER_FLAG_DELTA_ENABLED);
> +}
>
> Maybe ARRAY_SIZE can be of use in the loop indices here?
>
> +static void vdec_av1_slice_setup_cdef(struct vdec_av1_slice_cdef
> *cdef,
> + struct v4l2_av1_cdef *ctrl_cdef)
> +{
> + int i;
> +
> + cdef->cdef_damping = ctrl_cdef->damping_minus_3 + 3;
> + cdef->cdef_bits = ctrl_cdef->bits;
> +
> + for (i = 0; i < V4L2_AV1_CDEF_MAX; i++) {
> + if (ctrl_cdef->y_sec_strength[i] == 4)
> + ctrl_cdef->y_sec_strength[i] -= 1;
> +
> + if (ctrl_cdef->uv_sec_strength[i] == 4)
> + ctrl_cdef->uv_sec_strength[i] -= 1;
> +
> + cdef->cdef_y_strength[i] = ctrl_cdef->y_pri_strength[i]
> << 2 |
> +ctrl_cdef-
> >y_sec_strength[i];
> + cdef->cdef_uv_strength[i] = ctrl_cdef-
> >uv_pri_strength[i] << 2 |
> + ctrl_cdef-
> >uv_sec_strength[i];
> + }
> +}
>
> Maybe:
>
> #define SECONDARY_FILTER_STRENGTH_NUM_BITS 2
>
> + cdef->cdef_y_strength[i] = ctrl_cdef->y_pri_strength[i]
> <<
> SECONDARY_FILTER_STRENGTH_NUM_BITS |
> +ctrl_cdef-
> >y_sec_strength[i];
> + cdef->cdef_uv_strength[i] = ctrl_cdef-
> >uv_pri_strength[i] <<
> SECONDARY_FILTER_STRENGTH_NUM_BITS |
> +
Re: [RFC PATCH v3] media: mediatek: vcodec: support stateless AV1 decoder
Hi Xiaoyong.
Comments below (other code removed for brevity)
+/**
+ * struct vdec_av1_slice_slot - slot info need save in global instance
+ * @frame_info: frame info for each slot
+ * @timestamp: time stamp info
+ */
+struct vdec_av1_slice_slot {
+ struct vdec_av1_slice_frame_info frame_info[AV1_MAX_FRAME_BUF_COUNT];
+ u64 timestamp[AV1_MAX_FRAME_BUF_COUNT];
+};
nit: slot info that needs to be saved in the global instance
+static int vdec_av1_slice_get_qindex(struct
vdec_av1_slice_uncompressed_header *uh,
+int segmentation_id)
+{
+ struct vdec_av1_slice_seg *seg = >seg;
+ struct vdec_av1_slice_quantization *quant = >quant;
+ int data = 0, qindex = 0;
+
+ if (seg->segmentation_enabled &&
+ (seg->feature_enabled_mask[segmentation_id] & BIT(0))) {
+ data = seg->feature_data[segmentation_id][0];
Maybe you should replace the 0 above by SEG_LVL_ALT_Q to be more
explicit. Same goes for BIT(0).
+static void vdec_av1_slice_setup_lr(struct vdec_av1_slice_lr *lr,
+ struct v4l2_av1_loop_restoration *ctrl_lr)
+{
+ int i;
+
+ for (i = 0; i < V4L2_AV1_NUM_PLANES_MAX; i++) {
+ lr->frame_restoration_type[i] =
ctrl_lr->frame_restoration_type[i];
+ lr->loop_restoration_size[i] =
ctrl_lr->loop_restoration_size[i];
+ }
+ lr->use_lr = !!lr->frame_restoration_type[0];
+ lr->use_chroma_lr = !!lr->frame_restoration_type[1];
+}
From a first glance, this looks a bit divergent from the spec?
for ( i = 0; i < NumPlanes; i++ ) {
lr_type
FrameRestorationType[i] = Remap_Lr_Type[lr_type]
if ( FrameRestorationType[i] != RESTORE_NONE ) {
UsesLr = 1
if ( i > 0 ) {
usesChromaLr = 1
}
}
}
I will include these two variables in the next iteration of the uapi if
computing them in the driver is problematic.
+static void vdec_av1_slice_setup_lf(struct vdec_av1_slice_loop_filter *lf,
+ struct v4l2_av1_loop_filter *ctrl_lf)
+{
+ int i;
+
+ for (i = 0; i < 4; i++)
+ lf->loop_filter_level[i] = ctrl_lf->level[i];
+
+ for (i = 0; i < V4L2_AV1_TOTAL_REFS_PER_FRAME; i++)
+ lf->loop_filter_ref_deltas[i] = ctrl_lf->ref_deltas[i];
+
+ for (i = 0; i < 2; i++)
+ lf->loop_filter_mode_deltas[i] = ctrl_lf->mode_deltas[i];
+
+ lf->loop_filter_sharpness = ctrl_lf->sharpness;
+ lf->loop_filter_delta_enabled =
+ BIT_FLAG(ctrl_lf, V4L2_AV1_LOOP_FILTER_FLAG_DELTA_ENABLED);
+}
Maybe ARRAY_SIZE can be of use in the loop indices here?
+static void vdec_av1_slice_setup_cdef(struct vdec_av1_slice_cdef *cdef,
+ struct v4l2_av1_cdef *ctrl_cdef)
+{
+ int i;
+
+ cdef->cdef_damping = ctrl_cdef->damping_minus_3 + 3;
+ cdef->cdef_bits = ctrl_cdef->bits;
+
+ for (i = 0; i < V4L2_AV1_CDEF_MAX; i++) {
+ if (ctrl_cdef->y_sec_strength[i] == 4)
+ ctrl_cdef->y_sec_strength[i] -= 1;
+
+ if (ctrl_cdef->uv_sec_strength[i] == 4)
+ ctrl_cdef->uv_sec_strength[i] -= 1;
+
+ cdef->cdef_y_strength[i] = ctrl_cdef->y_pri_strength[i] << 2 |
+ ctrl_cdef->y_sec_strength[i];
+ cdef->cdef_uv_strength[i] = ctrl_cdef->uv_pri_strength[i] << 2 |
+ ctrl_cdef->uv_sec_strength[i];
+ }
+}
Maybe:
#define SECONDARY_FILTER_STRENGTH_NUM_BITS 2
+ cdef->cdef_y_strength[i] = ctrl_cdef->y_pri_strength[i] <<
SECONDARY_FILTER_STRENGTH_NUM_BITS |
+ ctrl_cdef->y_sec_strength[i];
+ cdef->cdef_uv_strength[i] = ctrl_cdef->uv_pri_strength[i] <<
SECONDARY_FILTER_STRENGTH_NUM_BITS |
+ ctrl_cdef->uv_sec_strength[i];
This should make it clearer.
+ sb_boundary_x_m1 =
+ (tile->mi_col_starts[tile_col + 1] - tile->mi_col_starts[tile_col] -
1) &
+ 0x3F;
+ sb_boundary_y_m1 =
+ (tile->mi_row_starts[tile_row + 1] - tile->mi_row_starts[tile_row] -
1) &
+ 0x1FF;
+
IIRC there's a preference for lower case hex values in the media subsystem.
+static void vdec_av1_slice_get_dpb_size(struct vdec_av1_slice_instance
*instance, u32 *dpb_sz)
+{
+ /* refer av1 specification */
+ *dpb_sz = 9;
+}
That's actually defined as 8 in the spec, i.e.:
NUM_REF_FRAMES 8 Number of frames that can be stored for future
reference.
It's helpful to indicate the section if you reference the specification,
as it makes it easier for the reviewer to cross check.
+ /* get buffer address from vb2buf */
+ for (i = 0; i < V4L2_AV1_REFS_PER_FRAME; i++) {
+ struct vdec_av1_slice_fb *vref = >ref[i];
+ int idx =
