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

[Zhao, Yakui]

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.

It will be better that you can describe the keypoint in the patch
especially when it updates the content required by the hardware.
Otherwise too many things are mixed in the patches and it is difficult
to review it.

Thanks.
    Yakui
> 
> 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;
> >
> >