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

[Gwenole Beauchesne]

2014-05-14 11:04 GMT+02:00 Gwenole Beauchesne <gb.devel at gmail.com>:
> Hi,
>
> 2014-05-12 8:17 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
>> On Sun, 2014-05-11 at 23:31 -0600, Gwenole Beauchesne wrote:
>>> 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.
>>
>> My point is that you can describe how you find the free slot for the
>> later update about the reference frame in DPB.
>
> OK, will do but there is really zero change in the algorithm beyond
> the natural and higher order optimization.
>
>>> > 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.
>>
>> As I said in the previous email, the array of reference surface is one
>> content required by the hardware.
>
> I think there was a misunderstanding. The reference_surface[] is not
> changed (yet), only the reference_objects[]. That's totally different.
> The former is the frame store to maintain for the HW (and internal
> info we have in the DMV buffers actually); the latter should be a 1:1
> correspondence with VA surface ids in ReferenceFrames[] to the
> associated object_surface objects (stored in reference_surface).

Ping. Is there still something you don't understand in the patches?

> I will provide another patch set with a couple more patches:
> 1. Definitely fix MVC decode use cases ;
> 2. Optimize the process for Haswell and newer.

I will make this MVC specific. This patch series is a prerequisite,
and also applies to AVC decoding.

>>> >> >>  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;
>>> >> >
>>> >> >
>>> >
>>> >
>>
>>