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

[Zhao, Yakui]

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.  

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

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