[Libva] [PATCH 1/3] jpeg_enc: Avoid integer overflow while doing quality factor scaling
Sreerenj
sreerenj.balachandran at intel.com
Thu Mar 5 01:47:01 PST 2015
On 05.03.2015 09:12, Gwenole Beauchesne wrote:
> 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).
The calculations based on 5000 is something copied from libjpeg library
i think.
The quality factors should be in the range of 1 to 100 only. And yes,
there is code to do the clipping.
But as you know, it is wrong to do uint8_t quality = 5000/1, will simply
overflow and doing clipping on top of that.
IMO, there are other fixes also needed to make the quality calculation
correct.
I have only fixed couple of basic cases .
>
>> 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,
--
Thanks
Sree
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
More information about the Libva
mailing list