[Libva] [PATCH v3 intel-driver 4/9] decoder: h264: fix submission of AVC_REF_IDX_STATE command.

[Gwenole Beauchesne]

2014-06-05 4:18 GMT+02:00 Xiang, Haihao <haihao.xiang at intel.com>:
> On Tue, 2014-06-03 at 18:43 +0200, Gwenole Beauchesne wrote:
>> If the RefPicListX[] entry has no valid picture_id associated to it,
>> then set the resulting state to 0xff. If that entry has no surface
>> buffer storage either, then compose a valid state that maps to the
>> first item in the reference frames list, as mandated by the PRM.
>>
>> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne at intel.com>
>> ---
>>  src/i965_decoder_utils.c |   35 +++++++++++++++++++----------------
>>  1 file changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/i965_decoder_utils.c b/src/i965_decoder_utils.c
>> index c686235..e0593ea 100644
>> --- a/src/i965_decoder_utils.c
>> +++ b/src/i965_decoder_utils.c
>> @@ -334,34 +334,37 @@ gen5_fill_avc_ref_idx_state(
>>      const GenFrameStore frame_store[MAX_GEN_REFERENCE_FRAMES]
>>  )
>>  {
>> -    unsigned int i, n, frame_idx;
>> -    int found;
>> +    int i, j;
>> +    bool found;
>>
>> -    for (i = 0, n = 0; i < ref_list_count; i++) {
>> +    for (i = 0; i < ref_list_count; i++) {
>>          const VAPictureH264 * const va_pic = &ref_list[i];
>>
>> -        if (va_pic->flags & VA_PICTURE_H264_INVALID)
>> +        if ((va_pic->flags & VA_PICTURE_H264_INVALID) ||
>> +            va_pic->picture_id == VA_INVALID_ID) {
>> +            state[i] = 0xff;
>>              continue;
>> +        }
>>
>> -        found = 0;
>> -        for (frame_idx = 0; frame_idx < MAX_GEN_REFERENCE_FRAMES; frame_idx++) {
>> -            const GenFrameStore * const fs = &frame_store[frame_idx];
>> -            if (fs->surface_id != VA_INVALID_ID &&
>> -                fs->surface_id == va_pic->picture_id) {
>> -                found = 1;
>> +        found = false;
>> +        for (j = 0; j < MAX_GEN_REFERENCE_FRAMES; j++) {
>> +            if ((found = frame_store[j].surface_id == va_pic->picture_id))
>>                  break;
>> -            }
>>          }
>>
>>          if (found) {
>> -            state[n++] = get_ref_idx_state_1(va_pic, frame_idx);
>> -        } else {
>> -            WARN_ONCE("Invalid Slice reference frame list !!!. It is not included in DPB \n");
>> +            const GenFrameStore * const fs = &frame_store[j];
>> +            assert(fs->frame_store_id == j); // Current architecture/assumption
>> +            state[i] = get_ref_idx_state_1(va_pic, fs->frame_store_id);
>> +        }
>> +        else {
>> +            WARN_ONCE("Invalid RefPicListX[] entry!!! It is not included in DPB\n");
>> +            state[i] = get_ref_idx_state_1(va_pic, 0) | 0x80;
>
>
> How to ensure the first item in the reference frames list is valid ? It
> seems it is needed to update the reference frames in
> MFX_PIPE_BUF_ADDR_STATE to make sure the first item is always valid.

e.g. fix intel_decoder_check_avc_parameter() to validate RefPicListX[]
too and error out if there is any frame that is not available in the
ReferenceFrames[] array, or if the set of active ref frames reduces to
nothing despite the need for RefPicList[]. Then, we are sure at this
point that there is at least one entry that is valid.

That's for Fix 1. Another fix would be needed to strictly conform with
the specs, while probably providing improved error concealment at the
same time: always fill a MFX_PIPE_BUF_ADDR_STATE entries (RefAddr[]
array).

I will work on Fix 1, if you don't for the next 3 hours. Fix 2 could
be deferred to a later stage.

Thanks,
Gwenole.

>
>
>
>>          }
>>      }
>>
>> -    for (; n < 32; n++)
>> -        state[n] = 0xff;
>> +    for (; i < 32; i++)
>> +        state[i] = 0xff;
>>  }
>>
>>  /* Emit Reference List Entries (Gen6+: SNB, IVB) */
>
>