[Libva] [PATCH intel-driver 1/4] decoder: h264: don't deallocate surface storage of older frames.

[Gwenole Beauchesne]

Hi,

2014-05-12 7:21 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
> On Sun, 2014-05-11 at 22:35 -0600, Gwenole Beauchesne wrote:
>> Hi,
>>
>> 2014-05-12 2:55 GMT+02:00 Zhao Yakui <yakui.zhao at intel.com>:
>> > On Fri, 2014-05-09 at 08:34 -0600, Gwenole Beauchesne wrote:
>> >> Drop the optimization whereby surfaces that are no longer marked as
>> >> reference and that were already displayed are to be destroyed. This
>> >> is wrong mainly for two reasons:
>> >>
>> >> 1. The surface was displayed... once but it may still be needed for
>> >>    subsequent operations like displaying it again, using it for a
>> >>    transcode pipeline (encode) for instance, etc.
>> >>
>> >> 2. The new set of ReferenceFrames[] correspond to the active set of
>> >>    reference frames used for decoding the current slice. In presence
>> >>    of Multiview Coding (MVC), that could correspond to the current
>> >>    view, in view order index, but the surface may still be needed
>> >>    for decoding the next view with the same view_id, while also
>> >>    decoding other views with another set of reference frames for them.
>> >
>> > Hi, Gwenole
>> >
>> >     Thanks for working on the h264 reference frame list. And I have
>> > some concerns about the patch set:
>> >     1. When will the unused reference surface be destroyed?
>>
>> As it ought to be: at vaDestroySurfaces() time.
>>
>> >     2. How to find one free slot in decode_state->reference_surfaces
>> > array to store the corresponding reference frame?
>>
>> It's the same algorithm. :)
>> At this time, the reference_surfaces[] contents exactly matches
>> ReferenceFrames[], hole for hole, if there is any. The former holds
>> object_surface, the latter holds the associated VAPictureH264. i.e.
>> pointer to object_surface [i] is really the reference_surface[i] that
>> matches VAPictureH264 at ReferenceFrames[i]. Previously, there could
>> be a mismatch between both.
>
> As you know, the current policy in the driver is based on the following
> flowchart:
>     1. the driver maintains one array for the hardware-requirement.
>     2. When one new frame is decoded, it will check whether the array
> element  maintained by the driver is still used by the DPB. If not, the
> internal data structure will be free and then be used for the later
> selection.
>     3. Find the free slot in the array to store the reference frames in
> DPB.
>
> But I don't see how this patch set stores/updates the reference frames
> of DPB into the driver array and how to decide which slot can be
> replaced by the reference frames in new DPB.

I don't understand your concern. It works the same as before, but
replaces ReferenceFrames[] with reference_surface[], which now hold
the same contents, in order. The free_slots[] array is filled in order
too so that to avoid the extraneous iterations again to look for the
same free slot id.

> It will be better that you can describe the keypoint in the patch
> especially when it updates the content required by the hardware.

What contents do you think we need to maintain for the hardware? The
hardware is designed to be stateless, so it normally doesn't keep
anything. This is necessary for context switching performance.

Regards,
Gwenole.

>> >>  src/i965_decoder_utils.c | 13 -------------
>> >>  1 file changed, 13 deletions(-)
>> >>
>> >> diff --git a/src/i965_decoder_utils.c b/src/i965_decoder_utils.c
>> >> index 2533381..9d537ff 100644
>> >> --- a/src/i965_decoder_utils.c
>> >> +++ b/src/i965_decoder_utils.c
>> >> @@ -382,19 +382,6 @@ intel_update_avc_frame_store_index(VADriverContextP ctx,
>> >>
>> >>          /* remove it from the internal DPB */
>> >>          if (!found) {
>> >> -            struct object_surface *obj_surface = frame_store[i].obj_surface;
>> >> -
>> >> -            obj_surface->flags &= ~SURFACE_REFERENCED;
>> >> -
>> >> -            if ((obj_surface->flags & SURFACE_ALL_MASK) == SURFACE_DISPLAYED) {
>> >> -                dri_bo_unreference(obj_surface->bo);
>> >> -                obj_surface->bo = NULL;
>> >> -                obj_surface->flags &= ~SURFACE_REF_DIS_MASK;
>> >> -            }
>> >> -
>> >> -            if (obj_surface->free_private_data)
>> >> -                obj_surface->free_private_data(&obj_surface->private_data);
>> >> -
>> >>              frame_store[i].surface_id = VA_INVALID_ID;
>> >>              frame_store[i].frame_store_id = -1;
>> >>              frame_store[i].obj_surface = NULL;
>> >
>> >
>
>