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

[Gwenole Beauchesne]

2014-06-05 6:17 GMT+02:00 Gwenole Beauchesne <gb.devel at gmail.com>:
> 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.

Actually, create an intel_decoder_check_avc_slice_params() for that,
and that is called in the same loop that searches for ILDB?

>
> 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) */
>>
>>