Re: [PATCH] media: coda: add SPS fixup code for frame sizes that are not multiples of 16

2018-07-16 Thread Philipp Zabel
On Mon, 2018-07-02 at 10:53 +0200, Hans Verkuil wrote:
> On 28/06/18 18:47, Philipp Zabel wrote:
> > The CODA firmware does not set the VUI frame cropping fields to properly
> > describe coded h.264 streams with frame sizes that are not a multiple of
> > the macroblock size.
> > This adds RBSP parsing code and a SPS fixup routine to manually replace
> > the cropping information in the headers produced by the firmware with
> > the correct values.
> > 
> > Signed-off-by: Philipp Zabel 
> > ---
> >  drivers/media/platform/coda/coda-bit.c  |  11 +
> >  drivers/media/platform/coda/coda-h264.c | 302 
> >  drivers/media/platform/coda/coda.h  |   2 +
> >  3 files changed, 315 insertions(+)
> > 
> > diff --git a/drivers/media/platform/coda/coda-bit.c 
> > b/drivers/media/platform/coda/coda-bit.c
> > index 94ba3cc3bf14..d6380a10fa2c 100644
> > --- a/drivers/media/platform/coda/coda-bit.c
> > +++ b/drivers/media/platform/coda/coda-bit.c
> > @@ -1197,6 +1197,17 @@ static int coda_start_encoding(struct coda_ctx *ctx)
> > if (ret < 0)
> > goto out;
> >  
> > +   if ((q_data_src->rect.width % 16) ||
> > +   (q_data_src->rect.height % 16)) {
> > +   ret = coda_sps_fixup(ctx, q_data_src->rect.width,
> > +q_data_src->rect.height,
> > +&ctx->vpu_header[0][0],
> > +&ctx->vpu_header_size[0],
> > +sizeof(ctx->vpu_header[0]));
> 
> You need to add a comment here why this is needed.
[...]
> > +int coda_sps_fixup(struct coda_ctx *ctx, int width, int height, char *buf,
> > +  int *size, int max_size)
> 
> Same here: why it is needed and what does it do.

Thank you, I'll send a v2 with this fixed.

This function rewrites the h.264 SPS header to set the crop_right and
crop_bottom fields correctly, as these are always set to 0 by the
CODA7541 firmware.

Setting the crop_* fields is important for frame sizes that are not
multiples of the macroblock size: those fields are used together with
the pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1 fields
to determine the visible frame width and height.

regards
Philipp


Re: [PATCH] media: coda: add SPS fixup code for frame sizes that are not multiples of 16

2018-07-02 Thread Hans Verkuil
On 28/06/18 18:47, Philipp Zabel wrote:
> The CODA firmware does not set the VUI frame cropping fields to properly
> describe coded h.264 streams with frame sizes that are not a multiple of
> the macroblock size.
> This adds RBSP parsing code and a SPS fixup routine to manually replace
> the cropping information in the headers produced by the firmware with
> the correct values.
> 
> Signed-off-by: Philipp Zabel 
> ---
>  drivers/media/platform/coda/coda-bit.c  |  11 +
>  drivers/media/platform/coda/coda-h264.c | 302 
>  drivers/media/platform/coda/coda.h  |   2 +
>  3 files changed, 315 insertions(+)
> 
> diff --git a/drivers/media/platform/coda/coda-bit.c 
> b/drivers/media/platform/coda/coda-bit.c
> index 94ba3cc3bf14..d6380a10fa2c 100644
> --- a/drivers/media/platform/coda/coda-bit.c
> +++ b/drivers/media/platform/coda/coda-bit.c
> @@ -1197,6 +1197,17 @@ static int coda_start_encoding(struct coda_ctx *ctx)
>   if (ret < 0)
>   goto out;
>  
> + if ((q_data_src->rect.width % 16) ||
> + (q_data_src->rect.height % 16)) {
> + ret = coda_sps_fixup(ctx, q_data_src->rect.width,
> +  q_data_src->rect.height,
> +  &ctx->vpu_header[0][0],
> +  &ctx->vpu_header_size[0],
> +  sizeof(ctx->vpu_header[0]));

You need to add a comment here why this is needed.

> + if (ret < 0)
> + goto out;
> + }
> +
>   /*
>* Get PPS in the first frame and copy it to an
>* intermediate buffer.
> diff --git a/drivers/media/platform/coda/coda-h264.c 
> b/drivers/media/platform/coda/coda-h264.c
> index 0e27412e01f5..8ec5a5d076c0 100644
> --- a/drivers/media/platform/coda/coda-h264.c
> +++ b/drivers/media/platform/coda/coda-h264.c
> @@ -111,3 +111,305 @@ int coda_h264_level(int level_idc)
>   default: return -EINVAL;
>   }
>  }
> +
> +struct rbsp {
> + char *buf;
> + int size;
> + int pos;
> +};
> +
> +static inline int rbsp_read_bit(struct rbsp *rbsp)
> +{
> + int shift = 7 - (rbsp->pos % 8);
> + int ofs = rbsp->pos++ / 8;
> +
> + if (ofs >= rbsp->size)
> + return -EINVAL;
> +
> + return (rbsp->buf[ofs] >> shift) & 1;
> +}
> +
> +static inline int rbsp_write_bit(struct rbsp *rbsp, int bit)
> +{
> + int shift = 7 - (rbsp->pos % 8);
> + int ofs = rbsp->pos++ / 8;
> +
> + if (ofs >= rbsp->size)
> + return -EINVAL;
> +
> + rbsp->buf[ofs] &= ~(1 << shift);
> + rbsp->buf[ofs] |= bit << shift;
> +
> + return 0;
> +}
> +
> +static inline int rbsp_read_bits(struct rbsp *rbsp, int num, int *val)
> +{
> + int i, ret;
> + int tmp = 0;
> +
> + if (num > 32)
> + return -EINVAL;
> +
> + for (i = 0; i < num; i++) {
> + ret = rbsp_read_bit(rbsp);
> + if (ret < 0)
> + return ret;
> + tmp |= ret << (num - i - 1);
> + }
> +
> + if (val)
> + *val = tmp;
> +
> + return 0;
> +}
> +
> +static int rbsp_write_bits(struct rbsp *rbsp, int num, int value)
> +{
> + int ret;
> +
> + while (num--) {
> + ret = rbsp_write_bit(rbsp, (value >> num) & 1);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int rbsp_read_uev(struct rbsp *rbsp, unsigned int *val)
> +{
> + int leading_zero_bits = 0;
> + unsigned int tmp = 0;
> + int ret;
> +
> + while ((ret = rbsp_read_bit(rbsp)) == 0)
> + leading_zero_bits++;
> + if (ret < 0)
> + return ret;
> +
> + if (leading_zero_bits > 0) {
> + ret = rbsp_read_bits(rbsp, leading_zero_bits, &tmp);
> + if (ret)
> + return ret;
> + }
> +
> + if (val)
> + *val = (1 << leading_zero_bits) - 1 + tmp;
> +
> + return 0;
> +}
> +
> +static int rbsp_write_uev(struct rbsp *rbsp, unsigned int value)
> +{
> + int i;
> + int ret;
> + int tmp = value + 1;
> + int leading_zero_bits = fls(tmp) - 1;
> +
> + for (i = 0; i < leading_zero_bits; i++) {
> + ret = rbsp_write_bit(rbsp, 0);
> + if (ret)
> + return ret;
> + }
> +
> + return rbsp_write_bits(rbsp, leading_zero_bits + 1, tmp);
> +}
> +
> +static int rbsp_read_sev(struct rbsp *rbsp, int *val)
> +{
> + unsigned int tmp;
> + int ret;
> +
> + ret = rbsp_read_uev(rbsp, &tmp);
> + if (ret)
> + return ret;
> +
> + if (val) {
> + if (tmp & 1)
> + *val = (tmp + 1) / 2;
> + else
> + *val = -(tmp / 2);
> + }
> +
> + return 0;
> +}
> +
> +int coda_sps_fixup(struct coda_ctx *ctx, int width, int he