On 12/17/18 6:23 PM, Dafna Hirschfeld wrote: > > > On Sun, Dec 16, 2018 at 2:24 PM Hans Verkuil <hverkuil@xxxxxxxxx <mailto:hverkuil@xxxxxxxxx>> wrote: > > On 12/15/18 12:54 PM, Dafna Hirschfeld wrote: > > Add support for the selection api for the crop target. > > Mention that this is added for the encoder only. > > > The driver rounds up the width and height such that > > all planes dimesnsions are multiple of 8. > > dimesnsions -> dimensions > > > The userspace client should also read and write > > the raw frames according the padding. > > Since we're not changing the userspace client here, you can drop > that paragraph. > > > > > Signed-off-by: Dafna Hirschfeld <dafna3@xxxxxxxxx <mailto:dafna3@xxxxxxxxx>> > > --- > > drivers/media/platform/vicodec/codec-fwht.c | 45 ++-- > > drivers/media/platform/vicodec/codec-fwht.h | 5 +- > > .../media/platform/vicodec/codec-v4l2-fwht.c | 18 +- > > .../media/platform/vicodec/codec-v4l2-fwht.h | 2 + > > drivers/media/platform/vicodec/vicodec-core.c | 219 ++++++++++++++---- > > 5 files changed, 213 insertions(+), 76 deletions(-) > > > > diff --git a/drivers/media/platform/vicodec/codec-fwht.c b/drivers/media/platform/vicodec/codec-fwht.c > > index a678a716580c..ab59f34e9818 100644 > > --- a/drivers/media/platform/vicodec/codec-fwht.c > > +++ b/drivers/media/platform/vicodec/codec-fwht.c > > @@ -659,7 +659,7 @@ static void add_deltas(s16 *deltas, const u8 *ref, int stride) > > } > > > > static u32 encode_plane(u8 *input, u8 *refp, __be16 **rlco, __be16 *rlco_max, > > - struct fwht_cframe *cf, u32 height, u32 width, > > + struct fwht_cframe *cf, u32 height, u32 width, u32 stride, > > unsigned int input_step, > > bool is_intra, bool next_is_intra) > > { > > @@ -679,9 +679,9 @@ static u32 encode_plane(u8 *input, u8 *refp, __be16 **rlco, __be16 *rlco_max, > > > > if (!is_intra) > > blocktype = decide_blocktype(input, refp, > > - deltablock, width, input_step); > > + deltablock, stride, input_step); > > if (blocktype == IBLOCK) { > > - fwht(input, cf->coeffs, width, input_step, 1); > > + fwht(input, cf->coeffs, stride, input_step, 1); > > quantize_intra(cf->coeffs, cf->de_coeffs, > > cf->i_frame_qp); > > } else { > > @@ -722,7 +722,7 @@ static u32 encode_plane(u8 *input, u8 *refp, __be16 **rlco, __be16 *rlco_max, > > } > > last_size = size; > > } > > - input += width * 7 * input_step; > > + input += (stride - width/8) * 8 * input_step; > > width/8 -> width / 8 > > Hmm, I think this calculation is confusing. > > A better solution would be to set the input pointer just before the > 'for (i = 0; i < width / 8; i++) {' loop. E.g.: > > for (j = 0; j < height / 8; j++) { > input = input_start + j * 8 * stride * input_step; > for (i = 0; i < width / 8; i++) { > > (I hope I got the calculation right) > > > } > > > > exit_loop: > > @@ -756,7 +756,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > > > rlco_max = rlco + size / 2 - 256; > > encoding = encode_plane(frm->luma, ref_frm->luma, &rlco, rlco_max, cf, > > - frm->height, frm->width, > > + frm->height, frm->width, frm->stride, > > frm->luma_alpha_step, is_intra, next_is_intra); > > if (encoding & FWHT_FRAME_UNENCODED) > > encoding |= FWHT_LUMA_UNENCODED; > > @@ -765,11 +765,12 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > if (frm->components_num >= 3) { > > u32 chroma_h = frm->height / frm->height_div; > > u32 chroma_w = frm->width / frm->width_div; > > + u32 stride = frm->stride / frm->width_div; > > Call this chroma_stride to stay consistent with chroma_h/w etc. > > > unsigned int chroma_size = chroma_h * chroma_w; > > > > rlco_max = rlco + chroma_size / 2 - 256; > > encoding |= encode_plane(frm->cb, ref_frm->cb, &rlco, rlco_max, > > - cf, chroma_h, chroma_w, > > + cf, chroma_h, chroma_w, stride, > > frm->chroma_step, > > is_intra, next_is_intra); > > if (encoding & FWHT_FRAME_UNENCODED) > > @@ -777,7 +778,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > encoding &= ~FWHT_FRAME_UNENCODED; > > rlco_max = rlco + chroma_size / 2 - 256; > > encoding |= encode_plane(frm->cr, ref_frm->cr, &rlco, rlco_max, > > - cf, chroma_h, chroma_w, > > + cf, chroma_h, chroma_w, stride, > > frm->chroma_step, > > is_intra, next_is_intra); > > if (encoding & FWHT_FRAME_UNENCODED) > > @@ -789,7 +790,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > rlco_max = rlco + size / 2 - 256; > > encoding |= encode_plane(frm->alpha, ref_frm->alpha, &rlco, > > rlco_max, cf, frm->height, frm->width, > > - frm->luma_alpha_step, > > + frm->stride, frm->luma_alpha_step, > > is_intra, next_is_intra); > > if (encoding & FWHT_FRAME_UNENCODED) > > encoding |= FWHT_ALPHA_UNENCODED; > > @@ -801,7 +802,7 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > } > > > > static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref, > > - u32 height, u32 width, bool uncompressed) > > + u32 height, u32 width, u32 stride, bool uncompressed) > > { > > unsigned int copies = 0; > > s16 copy[8 * 8]; > > @@ -822,13 +823,13 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref, > > */ > > for (j = 0; j < height / 8; j++) { > > for (i = 0; i < width / 8; i++) { > > - u8 *refp = ref + j * 8 * width + i * 8; > > + u8 *refp = ref + j * 8 * stride + i * 8; > > > > if (copies) { > > memcpy(cf->de_fwht, copy, sizeof(copy)); > > if (stat & PFRAME_BIT) > > - add_deltas(cf->de_fwht, refp, width); > > - fill_decoder_block(refp, cf->de_fwht, width); > > + add_deltas(cf->de_fwht, refp, stride); > > + fill_decoder_block(refp, cf->de_fwht, stride); > > copies--; > > continue; > > } > > @@ -847,35 +848,37 @@ static void decode_plane(struct fwht_cframe *cf, const __be16 **rlco, u8 *ref, > > if (copies) > > memcpy(copy, cf->de_fwht, sizeof(copy)); > > if (stat & PFRAME_BIT) > > - add_deltas(cf->de_fwht, refp, width); > > - fill_decoder_block(refp, cf->de_fwht, width); > > + add_deltas(cf->de_fwht, refp, stride); > > + fill_decoder_block(refp, cf->de_fwht, stride); > > } > > } > > } > > > > void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref, > > - u32 hdr_flags, unsigned int components_num) > > + u32 hdr_flags, unsigned int components_num, unsigned int stride) > > { > > const __be16 *rlco = cf->rlc_data; > > > > - decode_plane(cf, &rlco, ref->luma, cf->height, cf->width, > > + decode_plane(cf, &rlco, ref->luma, cf->height, cf->width, stride, > > hdr_flags & FWHT_FL_LUMA_IS_UNCOMPRESSED); > > > > if (components_num >= 3) { > > u32 h = cf->height; > > u32 w = cf->width; > > - > > + u32 s = stride; > > Add an empty line after declaring the variables. > > > if (!(hdr_flags & FWHT_FL_CHROMA_FULL_HEIGHT)) > > h /= 2; > > - if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH)) > > + if (!(hdr_flags & FWHT_FL_CHROMA_FULL_WIDTH)) { > > w /= 2; > > - decode_plane(cf, &rlco, ref->cb, h, w, > > + s /= 2; > > + } > > + decode_plane(cf, &rlco, ref->cb, h, w, s, > > hdr_flags & FWHT_FL_CB_IS_UNCOMPRESSED); > > - decode_plane(cf, &rlco, ref->cr, h, w, > > + decode_plane(cf, &rlco, ref->cr, h, w, s, > > hdr_flags & FWHT_FL_CR_IS_UNCOMPRESSED); > > } > > > > if (components_num == 4) > > - decode_plane(cf, &rlco, ref->alpha, cf->height, cf->width, > > + decode_plane(cf, &rlco, ref->alpha, cf->height, cf->width, stride, > > hdr_flags & FWHT_FL_ALPHA_IS_UNCOMPRESSED); > > } > > diff --git a/drivers/media/platform/vicodec/codec-fwht.h b/drivers/media/platform/vicodec/codec-fwht.h > > index 90ff8962fca7..86c38e873f69 100644 > > --- a/drivers/media/platform/vicodec/codec-fwht.h > > +++ b/drivers/media/platform/vicodec/codec-fwht.h > > @@ -81,6 +81,8 @@ > > #define FWHT_FL_COMPONENTS_NUM_MSK GENMASK(17, 16) > > #define FWHT_FL_COMPONENTS_NUM_OFFSET 16 > > > > +#define vic_round_dim(dim, div) (round_up(dim / div, 8) * div) > > This macro needs a comment explaining what it does. > > > + > > struct fwht_cframe_hdr { > > u32 magic1; > > u32 magic2; > > @@ -112,6 +114,7 @@ struct fwht_raw_frame { > > unsigned int luma_alpha_step; > > unsigned int chroma_step; > > unsigned int components_num; > > + unsigned int stride; > > u8 *luma, *cb, *cr, *alpha; > > }; > > > > @@ -127,6 +130,6 @@ u32 fwht_encode_frame(struct fwht_raw_frame *frm, > > struct fwht_cframe *cf, > > bool is_intra, bool next_is_intra); > > void fwht_decode_frame(struct fwht_cframe *cf, struct fwht_raw_frame *ref, > > - u32 hdr_flags, unsigned int components_num); > > + u32 hdr_flags, unsigned int components_num, unsigned int stride); > > > > #endif > > diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c > > index 8cb0212df67f..3eef6bbe5c06 100644 > > --- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c > > +++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c > > @@ -56,7 +56,7 @@ const struct v4l2_fwht_pixfmt_info *v4l2_fwht_get_pixfmt(u32 idx) > > > > int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out) > > { > > - unsigned int size = state->width * state->height; > > + unsigned int size; > > const struct v4l2_fwht_pixfmt_info *info = state->info; > > struct fwht_cframe_hdr *p_hdr; > > struct fwht_cframe cf; > > @@ -66,8 +66,11 @@ int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out) > > > > if (!info) > > return -EINVAL; > > + > > + size = state->stride * state->padded_height; > > Throughout this patch you talk about 'stride', but I think calling it 'padded_width' is a > much better name. > > There are actually three values here: > > width: this is the visible width in pixels > padded_width: this is the visible width in pixels rounded up to the next macroblock alignment > stride: this is the distance in bytes between two lines. > > width <= padded_width <= stride / input_step > > Currently q_data->width contain none of those, it is the value set by the S_FMT request so the stride can be calculated from it. > This is actually also the visible width for the decoder, since the COMPOSE selection is not implemented yet. Right ? Correct. For the decoder width == padded_width == stride / input_step > > state->width is the cropped width for the encoder and it is the same as q_data->width for the decoder. > You think I should change the field name 'state->width' to 'state->visible_width' ? I've been wondering about that myself. I think we should rename it, yes. Or perhaps to 'vis_width' if 'visible_width' becomes too cumbersome in the code. Being explicit about which 'width' we're talking about is probably helpful in understanding the code. > > I think that when I implement the COMPOSE for the decoder I could change the field ''cropped_width" with "visible_width" > in ""vicodec_q_data" and it will be the cropped width for the encoder and the composed width for the decoder. > Also, when I implement the COMPOSE for the decoder, then q_data->width could be changed to q_data->coded_width and > will contain the coded resolution (that is "vic_round_up(width,info->width_div)") You can rename it right now, I don't think you need to wait until COMPOSE support is added. The decoder already has those different 'width's already, they just always have the same value :-) But that will change once COMPOSE support is added. Regards, Hans
