[Libva] [PATCH v3 intel-driver 2/9] decoder: h264: allocate reference frames earlier, if needed.

[Gwenole Beauchesne]

2014-06-04 10:53 GMT+02:00 Xiang, Haihao <haihao.xiang at intel.com>:
> On Tue, 2014-06-03 at 18:43 +0200, Gwenole Beauchesne wrote:
>> Sometimes, a dummy frame comes from the codec layer and it is used
>> as a reference, per the comment in the existing code. Even though
>> this looks suspicious, keep this criterion but make sure to try
>> allocating the VA surface, if needed, earlier in the function that
>> sanity checks the parameters for decoding the current frame.
>>
>> This makes it possible to fail at a much earlier time, and actually
>> make it possible to return a sensible error code to the upper layer.
>>
>> Also fix the reference_objects[] array elements to be an exact 1:1
>> match for ReferenceFrames[] array elements, including possible but
>> unlikely holes in it. The former array holds object_surface structs
>> corresponding to the VA surfaces present in the ReferenceFrames[]
>> array and identified by VAPictureH264.picture_id.
>>
>> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
>> ---
>>  src/i965_decoder_utils.c |   46 +++++++++++++++++++++++-----------------------
>>  1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/i965_decoder_utils.c b/src/i965_decoder_utils.c
>> index 9ae7083..2570be9 100644
>> --- a/src/i965_decoder_utils.c
>> +++ b/src/i965_decoder_utils.c
>> @@ -477,12 +477,6 @@ intel_update_avc_frame_store_index(VADriverContextP ctx,
>>              int slot_found;
>>              struct object_surface *obj_surface = decode_state->reference_objects[i];
>>
>> -            /*
>> -             * Sometimes a dummy frame comes from the upper layer library, call i965_check_alloc_surface_bo()
>> -             * to ake sure the store buffer is allocated for this reference frame
>> -             */
>> -            avc_ensure_surface_bo(ctx, decode_state, obj_surface, pic_param);
>> -
>>              slot_found = 0;
>>              frame_idx = -1;
>>              /* Find a free frame store index */
>> @@ -606,6 +600,7 @@ intel_decoder_check_avc_parameter(VADriverContextP ctx,
>>  {
>>      struct i965_driver_data *i965 = i965_driver_data(ctx);
>>      VAPictureParameterBufferH264 *pic_param = (VAPictureParameterBufferH264 *)decode_state->pic_param->buffer;
>> +    VAStatus va_status;
>>      struct object_surface *obj_surface;
>>      int i;
>>
>> @@ -629,28 +624,33 @@ intel_decoder_check_avc_parameter(VADriverContextP ctx,
>>         }
>>      }
>>
>> -    for (i = 0; i < 16; i++) {
>> -        if (pic_param->ReferenceFrames[i].flags & VA_PICTURE_H264_INVALID ||
>> -            pic_param->ReferenceFrames[i].picture_id == VA_INVALID_SURFACE)
>> -            break;
>> -        else {
>> -            obj_surface = SURFACE(pic_param->ReferenceFrames[i].picture_id);
>> -            assert(obj_surface);
>> +    /* Fill in the reference objects array with the actual VA surface
>> +       objects with 1:1 correspondance with any entry in ReferenceFrames[],
>> +       i.e. including "holes" for invalid entries, that are expanded
>> +       to NULL in the reference_objects[] array */
>> +    for (i = 0; i < ARRAY_ELEMS(pic_param->ReferenceFrames); i++) {
>> +        const VAPictureH264 * const va_pic = &pic_param->ReferenceFrames[i];
>>
>> +        obj_surface = NULL;
>> +        if (!(va_pic->flags & VA_PICTURE_H264_INVALID) &&
>> +            va_pic->picture_id != VA_INVALID_ID) {
>> +            obj_surface = SURFACE(pic_param->ReferenceFrames[i].picture_id);
>>              if (!obj_surface)
>> -                goto error;
>> +                return VA_STATUS_ERROR_INVALID_SURFACE;
>>
>> -            if (!obj_surface->bo) { /* a reference frame  without store buffer */
>> -                WARN_ONCE("Invalid reference frame!!!\n");
>> -            }
>> -
>> -            decode_state->reference_objects[i] = obj_surface;
>> +            /*
>> +             * Sometimes a dummy frame comes from the upper layer
>> +             * library, call i965_check_alloc_surface_bo() to make
>> +             * sure the store buffer is allocated for this reference
>> +             * frame
>> +             */
>> +            va_status = avc_ensure_surface_bo(ctx, decode_state, obj_surface,
>> +                pic_param);
>
> Just noticed avc_ensure_surface_bo() always allocates the buffer store
> with tiled flag if necessary. It is wrong for ironlake.

Ah, OK. I thought it was only for MPEG-2 case. I will provide a fix to that.

Thanks,
Gwenole.

>
>
>> +            if (va_status != VA_STATUS_SUCCESS)
>> +                return va_status;
>>          }
>> +        decode_state->reference_objects[i] = obj_surface;
>>      }
>> -
>> -    for ( ; i < 16; i++)
>> -        decode_state->reference_objects[i] = NULL;
>> -
>>      return VA_STATUS_SUCCESS;
>>
>>  error:
>
>