[Libva] [PATCH 1/3] jpeg_enc: Avoid integer overflow while doing quality factor scaling

[Gwenole Beauchesne]

Hi,

2015-03-04 19:09 GMT+01:00  <sreerenj.balachandran at intel.com>:
> From: Sreerenj Balachandran <sreerenj.balachandran at intel.com>
>
> For eg: The uint8_t will simple overflow if submitted
> quality factor is 1 (5000/1).

And is the resulting value (5000) a valid one for the HW as well?
The comments below say that it shall be clipped to [ 1 .. 255 ] range.
So, another algorithm that would maintain the quality variable to
uint8_t is probably possible, and that would also maintain increased
(resp. decreased) quality if the input param increases (resp.
decreses).

> Note: Also removed a lot of whitespaces here and there.
> There are even more whitespaces all around the jpeg enc
> source code.

In this case, it's better to separate cosmetics from functional changes.

> ---
>  src/gen8_mfc.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/src/gen8_mfc.c b/src/gen8_mfc.c
> index 314b882..7754b70 100644
> --- a/src/gen8_mfc.c
> +++ b/src/gen8_mfc.c
> @@ -2674,17 +2674,17 @@ gen8_mfc_jpeg_fqm_state(VADriverContextP ctx,
>                          struct intel_encoder_context *encoder_context,
>                          struct encode_state *encode_state)
>  {
> -    uint8_t quality = 0;
> +    unsigned int quality = 0;
>      uint32_t temp, i = 0, j = 0, dword_qm[32];
>      VAEncPictureParameterBufferJPEG *pic_param;
>      VAQMatrixBufferJPEG *qmatrix;
>      unsigned char raster_qm[64];
>      struct gen6_mfc_context *mfc_context = encoder_context->mfc_context;
> -
> +
>      assert(encode_state->pic_param_ext && encode_state->pic_param_ext->buffer);
>      pic_param = (VAEncPictureParameterBufferJPEG *)encode_state->pic_param_ext->buffer;
>      quality = pic_param->quality;
> -
> +
>      //If the app sends the qmatrix, use it, buffer it for using it with the next frames
>      //The app can send qmatrix for the first frame and not send for the subsequent frames
>      if(encode_state->q_matrix && encode_state->q_matrix->buffer) {
> @@ -2705,16 +2705,19 @@ gen8_mfc_jpeg_fqm_state(VADriverContextP ctx,
>          qmatrix = &mfc_context->buffered_qmatrix;
>          qmatrix->load_lum_quantiser_matrix = 1;
>          qmatrix->load_chroma_quantiser_matrix = (pic_param->num_components > 1) ? 1 : 0;
> -    }
> -
> +    }
> +
> +    if (quality > 100)
> +        quality = 100;
> +
>      quality = (quality < 50) ? (5000/quality) : (200 - (quality*2));
>      quality = (quality == 0) ? 1 : quality;
> -
> +
>      //Step 1. Apply Quality factor and clip to range [1, 255] for luma and chroma Quantization matrices
>      //Step 2. HW expects the 1/Q[i] values in the qm sent, so get reciprocals
>      //Step 3. HW also expects 32 dwords, hence combine 2 (1/Q) values into 1 dword
>      //Step 4. Send the Quantization matrix to the HW, use gen8_mfc_fqm_state
> -
> +
>      //For luma (Y or R)
>      if(qmatrix->load_lum_quantiser_matrix) {
>          //apply quality to lum_quantiser_matrix
> @@ -2724,10 +2727,10 @@ gen8_mfc_jpeg_fqm_state(VADriverContextP ctx,
>              temp = (temp > 255) ? 255 : temp;
>              temp = (temp < 1) ? 1 : temp;
>              qmatrix->lum_quantiser_matrix[i] = (unsigned char)temp;
> -        }
> -
> -        //For VAAPI, the VAQMatrixBuffer needs to be in zigzag order.
> -        //The App should send it in zigzag. Now, the driver has to extract the raster from it.
> +        }
> +
> +        //For VAAPI, the VAQMatrixBuffer needs to be in zigzag order.
> +        //The App should send it in zigzag. Now, the driver has to extract the raster from it.
>          for (j = 0; j < 64; j++)
>              raster_qm[zigzag_direct[j]] = qmatrix->lum_quantiser_matrix[j];
>
> @@ -2736,16 +2739,16 @@ gen8_mfc_jpeg_fqm_state(VADriverContextP ctx,
>          //Need to double check if our HW expects col or row raster.
>          for (j = 0; j < 64; j++) {
>              int row = j / 8, col = j % 8;
> -            raster_qm[col * 8 + row] = raster_qm[j];
> +            raster_qm[col * 8 + row] = raster_qm[j];
>          }
> -
> +
>          //Convert to raster QM to reciprocal. HW expects values in reciprocal.
>          get_reciprocal_dword_qm(raster_qm, dword_qm);
> -
> +
>          //send the luma qm to the command buffer
>          gen8_mfc_fqm_state(ctx, MFX_QM_JPEG_LUMA_Y_QUANTIZER_MATRIX, dword_qm, 32, encoder_context);
> -    }
> -
> +    }
> +
>      //For Chroma, if chroma exists (Cb, Cr or G, B)
>      if(qmatrix->load_chroma_quantiser_matrix) {
>          //apply quality to chroma_quantiser_matrix
> @@ -2756,12 +2759,12 @@ gen8_mfc_jpeg_fqm_state(VADriverContextP ctx,
>              temp = (temp < 1) ? 1 : temp;
>              qmatrix->chroma_quantiser_matrix[i] = (unsigned char)temp;
>          }
> -
> -        //For VAAPI, the VAQMatrixBuffer needs to be in zigzag order.
> -        //The App should send it in zigzag. Now, the driver has to extract the raster from it.
> +
> +        //For VAAPI, the VAQMatrixBuffer needs to be in zigzag order.
> +        //The App should send it in zigzag. Now, the driver has to extract the raster from it.
>          for (j = 0; j < 64; j++)
>              raster_qm[zigzag_direct[j]] = qmatrix->chroma_quantiser_matrix[j];
> -
> +
>          //Convert the raster order(row-ordered) to the column-raster (column by column).
>          //To be consistent with the other encoders, send it in column order.
>          //Need to double check if our HW expects col or row raster.
> --
> 1.9.1
>
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva

Thanks,
-- 
Gwenole Beauchesne
Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France
Registration Number (RCS): Nanterre B 302 456 199